Fix link popover keyboard accessibility #10983

talldan · 2018-10-24T08:05:30Z

Description

Fixes #8266 by displaying the link popover whenever the caret is positioned within link formatting. As discussed in the issue, the agreed upon approach also involved displaying the toolbar whenever the caret is positioned within any formatting.

Also fixes #3350.

Changes:

Add core/editor state handling for storing whether caret is positioned within formatting.
Adjust toolbar rendering logic to show toolbar when caret is positioned within formatting.
Add e2e tests to ensure regressions don't occur.

How has this been tested?

e2e tests
manual testing

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

ellatrix · 2018-10-24T08:22:15Z

packages/editor/src/components/rich-text/index.js

@@ -426,7 +426,13 @@ export class RichText extends Component {
 			return;
 		}

-		const { start, end } = this.createRecord();
+		const { start, end, formats } = this.createRecord();


createRecord creates a new record form the DOM. getRecord gets the current record from props. Also I think this might be better to do in onChange right before or after you update the global state. You have direct access to the record there. Thanks!

looks like onChange only triggers when typing instead of when using cursor keys or changing selection with the mouse like I need.

The createRecord was there before, should it be getRecord instead?

Oh, never mind then, I only saw the green line. :) Then this looks good, sorry!

No worries - thanks for taking a look. 😄

talldan · 2018-10-24T08:28:20Z

packages/editor/src/components/block-list/block.js

@@ -399,7 +400,7 @@ export class BlockListBlock extends Component {
 		// We render block movers and block settings to keep them tabbale even if hidden
 		const shouldRenderMovers = ! isFocusMode && ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
 		const shouldShowBreadcrumb = ! isFocusMode && isHovered && ! isEmptyDefaultBlock;
-		const shouldShowContextualToolbar = ! hasFixedToolbar && ! showSideInserter && ( ( isSelected && ! isTypingWithinBlock ) || isFirstMultiSelected );
+		const shouldShowContextualToolbar = ! hasFixedToolbar && ! showSideInserter && ( ( isSelected && ( ! isTypingWithinBlock || isCaretWithinFormattedText ) ) || isFirstMultiSelected );


These bools scare me - might be worth refactoring them into functions or something?

Definitely yes, I recall them being quite scary the last time I worked on this file.

Might be worth a separate code quality issue as a follow-up, unless you have the time to do it now 😄

tofumatt

Thanks a million, this was the bug that was irking me the most in the merge milestone.

Just one comment about "leave" vs "exit", but after that: 🚢

tofumatt · 2018-10-24T12:41:17Z

docs/data/data-core-editor.md

+
+Returns an action object used in signalling that the caret has entered formatted text.
+
+### leaveFormattedText


Would this be better-named as exitFormattedText? It matches enterFormattedText more, and the docs confused me at first because I read "left" as in "left/right" not "left/leave".

👍 changed this

tofumatt · 2018-10-24T12:42:02Z

test/e2e/specs/links.test.js

@@ -388,4 +388,36 @@ describe( 'Links', () => {
 		await page.keyboard.press( 'Escape' );
 		expect( await page.$( '.editor-url-popover' ) ).toBeNull();
 	} );
+
+	it( 'can be modified using the keyboard once a link has been set', async () => {


❤️ for the test!

tofumatt · 2018-10-24T12:58:43Z

Ah, hold off one second, getting some weird behaviour in Firefox, let me see...

tofumatt · 2018-10-24T13:03:38Z

Never mind, my build was buggy; fixed it.

Confirmed to be working for me with the keyboard. Awesome, thanks, ship it! 👍

chrisvanpatten · 2018-10-24T13:04:13Z

The only slightly weird thing is that it looks like the toolbar shows when your cursor is adjacent to the inline token but the token isn't itself highlighted. Not the end of the world, but it jumped out at me.

Either way this looks like an improvement!

noisysocks · 2018-10-25T01:26:27Z

Can we make the link popover not move around?

I could have sworn it used to anchor itself to the link... maybe #6113 regressed...

talldan · 2018-10-25T01:36:42Z

@noisysocks - I might look at that on a separate PR today. I think it'll be related to this component:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/rich-text/format-toolbar/positioned-at-selection.js

It might have been one my past changes that caused it to regress as I put out a fix to make it recalculate position whenever the selection changes - there was an issue where it didn't change position when clicking between links.

@chrisvanpatten - I see what you mean, and it works differently on the left and right side of the format. I think I can iron out the left/right inconsistency, but the highlighting of formatting is internal to tinymce (they're called inline boundaries), would have to hook into that somehow.

talldan · 2018-10-25T03:34:39Z

Looking some more the inconsistency with how the toolbar is displayed on the leading and trailing edge of formatted text is a bit of a deeper problem.

Currently if the caret is on the trailing edge of formatted text, the toolbar button to toggle the format doesn't work, but on the leading edge it does work. The logic in this PR for displaying the toolbar is consistent with that.

I think it's worth filing a separate issue to look at fixing both things at once.

afercia · 2018-10-25T07:47:48Z

Thanks for working on this! Quickly tested, I've noticed the same thing @chrisvanpatten and @talldan mentioned about the highlighting and inline boundaries (not only an inconsistency between left and right behaviors but it's also different from Classic Editor). Regardless, makes perfectly sense to me to try to address that separately. This PR is a good improvement for keyboard users and I'm all for it. Thank you.

youknowriad · 2018-10-26T10:14:14Z

packages/editor/src/components/block-list/block.js

@@ -372,6 +372,7 @@ export class BlockListBlock extends Component {
 			isPartOfMultiSelection,
 			isFirstMultiSelected,
 			isTypingWithinBlock,
+			isCaretWithinFormattedText,


I don't like that the block's wrapper is aware of something very specific to the implementation of a given block (RichText). It doesn't seem like a generic enough solution for me. All blocks don't use RichText. As I see it the block wrapper should work similarily no matter what's the content of the edit function.

That said, I'm not familiar enough with the issue and the PR to propose an alternative. (which means I won't block this PR either)

That's a good point. I don't have a better solution either off the top of my head, aside from just documenting that it's RichText specific, which still isn't great/is the whole problem. 🤷‍♂️

youknowriad · 2018-10-27T10:19:51Z

Let's rebase and merge.

tofumatt · 2018-10-27T10:55:02Z

Rebased and pushed, just waiting on Travis and then I'll get this merged.

tofumatt · 2018-10-27T10:57:16Z

Urgh, something went wrong with the rebase, looking into it now...

tofumatt · 2018-10-27T11:01:33Z

Rebase fixed… will merge when ready 😄

aduth · 2018-10-29T19:51:26Z

Is this new action expected to be so noisy? This is just in writing a few words in a paragraph:

There's a decent amount of work performed on each action dispatched. We should try to limit this if we can.

ellatrix · 2018-10-29T21:53:13Z

packages/editor/src/components/rich-text/index.js

-		const { start, end } = this.createRecord();
+		const { start, end, formats } = this.createRecord();
+
+		if ( formats[ start ] ) {


Could this be moved in start !== this.state.start || end !== this.state.end you think? Could we check right here which flag is set to avoid calling the prop?

talldan · 2018-10-30T08:54:50Z

Thanks for catching that - I've got a PR here to reduce the frequency of dispatches - #11235

talldan self-assigned this Oct 24, 2018

talldan added this to the 4.2 milestone Oct 24, 2018

talldan requested a review from a team October 24, 2018 08:07

talldan added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Oct 24, 2018

talldan mentioned this pull request Oct 24, 2018

Can't edit an existing link using only the keyboard #8266

Closed

ellatrix reviewed Oct 24, 2018

View reviewed changes

talldan commented Oct 24, 2018

View reviewed changes

talldan requested a review from afercia October 24, 2018 09:15

tofumatt self-requested a review October 24, 2018 12:21

tofumatt approved these changes Oct 24, 2018

View reviewed changes

tofumatt mentioned this pull request Oct 24, 2018

Cannot edit existing link in URLInput #3350

Closed

talldan requested review from karmatosed and mtias October 25, 2018 03:44

talldan force-pushed the fix/link-popover-keyboard-a11y branch from 7b7144a to ac5d270 Compare October 25, 2018 08:45

talldan mentioned this pull request Oct 26, 2018

Highlighted formatting in the toolbar doesn't match MCE inline boundaries #11091

Closed

youknowriad reviewed Oct 26, 2018

View reviewed changes

tofumatt force-pushed the fix/link-popover-keyboard-a11y branch from ac5d270 to 7e4c15f Compare October 27, 2018 10:54

Add state to determine whether the caret is currently within formatting

017601d

talldan and others added 7 commits October 27, 2018 21:11

Show toolbar when the caret is positioned within formatting

0bdbc2b

Add e2e test covering modifying an existing link using the keyboard

45c09f5

Update docs

fd432f0

Change leave to exit

ec5bfb3

chore: Fix rebase issue

861c00c

Fix docs rebase issue

6c0a2b7

fix docs

97dfc04

tofumatt force-pushed the fix/link-popover-keyboard-a11y branch from a20ba53 to 97dfc04 Compare October 27, 2018 20:14

tofumatt merged commit 432bfe5 into master Oct 27, 2018

tofumatt deleted the fix/link-popover-keyboard-a11y branch October 27, 2018 21:15

ellatrix reviewed Oct 29, 2018

View reviewed changes

talldan mentioned this pull request Oct 30, 2018

Reduce frequency of actions updating isCaretWithinFormattedText #11235

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix link popover keyboard accessibility #10983

Fix link popover keyboard accessibility #10983

talldan commented Oct 24, 2018 •

edited by tofumatt

Loading

ellatrix Oct 24, 2018

talldan Oct 24, 2018

ellatrix Oct 24, 2018

talldan Oct 24, 2018

talldan Oct 24, 2018

tofumatt Oct 24, 2018

tofumatt left a comment

tofumatt Oct 24, 2018

talldan Oct 25, 2018

tofumatt Oct 24, 2018

tofumatt commented Oct 24, 2018

tofumatt commented Oct 24, 2018

chrisvanpatten commented Oct 24, 2018

noisysocks commented Oct 25, 2018

talldan commented Oct 25, 2018 •

edited

Loading

talldan commented Oct 25, 2018

afercia commented Oct 25, 2018

youknowriad Oct 26, 2018

tofumatt Oct 26, 2018

youknowriad commented Oct 27, 2018

tofumatt commented Oct 27, 2018

tofumatt commented Oct 27, 2018

tofumatt commented Oct 27, 2018

aduth commented Oct 29, 2018

ellatrix Oct 29, 2018

talldan commented Oct 30, 2018 •

edited

Loading


		Returns an action object used in signalling that the caret has entered formatted text.

		### leaveFormattedText

Fix link popover keyboard accessibility #10983

Fix link popover keyboard accessibility #10983

Conversation

talldan commented Oct 24, 2018 • edited by tofumatt Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Oct 24, 2018

tofumatt commented Oct 24, 2018

chrisvanpatten commented Oct 24, 2018

noisysocks commented Oct 25, 2018

talldan commented Oct 25, 2018 • edited Loading

talldan commented Oct 25, 2018

afercia commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Oct 27, 2018

tofumatt commented Oct 27, 2018

tofumatt commented Oct 27, 2018

tofumatt commented Oct 27, 2018

aduth commented Oct 29, 2018

Choose a reason for hiding this comment

talldan commented Oct 30, 2018 • edited Loading

talldan commented Oct 24, 2018 •

edited by tofumatt

Loading

talldan commented Oct 25, 2018 •

edited

Loading

talldan commented Oct 30, 2018 •

edited

Loading