Fix regressions and improve the insert link search field and suggestions list #10838

afercia · 2018-10-20T18:15:26Z

Additionally fixes a few CSS regressions and improves the interaction with the insert link search field and suggestions list.

First commit:

regression: makes the list of suggestions scrollable again when using a keyboard and the list is longer than the max-height (it worked in 2.9.0)
regression: fixes the input padding after 38378c8 which broke the usage of the variable in the padding calculation
regression: fixes the spinner position for the same reason

Worth noting:

I couldn't reproduce the original issue mentioned in Fix issue with link dialog. #8646 because there's now a 1 pixel padding that makes the focus style visible so I just reverted the variable to 8px
re: the padding on the right: seems to me there's nothing to display in that space so I'm not sure why the padding was meant to be 44px; changed to 8px

Second commit:

makes the TAB key populate the search field with the highlighted link URL
this allows keyboard users to select the link (not submit yet) and tab to the Link Settings button; matches the current interaction in the Classic Editor
adds a speak message to announce the link has been selected, matches the current interaction in the Classic Editor
when clicking on a suggested link, moves focus to the search field: matches the current interaction in the Classic Editor and clarifies a further action (submit) is needed
changes the Link added. message to Link inserted to match the Classic Editor message

Third commit:

~~regression: fixes the position of the suggestions popover after Chore - extract link container from rich text #10495~~
the suggestions popover is now nested within another popover (the URLPopover introduced in Chore - extract link container from rich text #10495)
this broke the position calculation, on current master the position looks like in the screenshot below:

Basically, this happens because the "Link Settings" button is now placed outside of the first popover:

Thus, the calculation of the suggestions popover is based on the search field form without the settings button. I can think of two ways to fix this:

either move back the "Link Settings" button within the form (not sure if this is possible / desirable)
or use a custom getAnchorRect function to get the grand parent node

I've opted for the latter, even if a better solution would probably be making the popover component handle these nesting cases. I'd appreciate some eyes form @aduth and @talldan. If there's the need for adjustments / refinements / improvements glad if you can take this PR and make it better.

adds 2 pixels to the suggestions list width to match the search field popover width (it has now an additional 1 pixel padding)
removes width: 100%; from the search field because it made the following "Apply" button smaller than it's supposed to be (the correct width is 36 pixels)
removes a stray list-style: none; applied on a div

Re: the popover position when a popover is nested within another popover, I guess this relates also to #8468.

youknowriad · 2018-10-20T22:33:17Z

packages/editor/src/components/url-input/index.js

+	 * position. As the popover with the list of suggestions is now nested within
+	 * another popover, we need to get the gran parent node.
+	 */
+	return anchor.parentNode.parentNode.getBoundingClientRect();


I understand the logic, but I'd prefer if we find another way as this seems fragile. It used to work properly before without it (even with two popovers) so curious to know why it regressed.

Yeah I agree it's fragile, that's why I've mentioned that maybe the popover component should handle these nesting cases internally. It happened to work before just by coincidence 🙂 If you look at the screenshot posted above, the suggestions list is perfectly centered to the first popover highlighted area. The ellipsis button is now outside of the first popover and doesn't contribute to determine its width, so the position calculation of the second popover takes into consideration only the highlighted area. If you move the ellipsis button back inside the first popover, then the position will work correctly (but just by coincidence).

youknowriad · 2018-10-20T22:34:22Z

packages/editor/src/components/url-input/index.js

 		this.props.onChange( post.url, post );
 		this.setState( {
 			selectedSuggestion: null,
 			showSuggestions: false,
 		} );
+
+		// Announce a link has been selected when tabbing away from the input field.
+		if ( event.keyCode === TAB ) {


Why should we only announce this on "tab" only? I prefer to do it consistently if possible.

Because when pressing Enter, the form is submitted and the link gets inserted. There's another speak message for that, see "Link inserted" in submitLink() in LinkContainer.

Gotcha, instead of passing the event object here, I'd personally just move this to the switch/case above

afercia · 2018-10-21T17:06:13Z

Thanks @youknowriad

tried to use the TAB / ENTER cases and dry up a bit to avoid duplicated code
when a suggested item gets clicked with the mouse, focus must be returned on the search field: using a new handleOnClick() for this

youknowriad

Nice, Thanks for the improvements.

youknowriad

Actually, this broken the url input for the button block.

afercia · 2018-10-21T18:37:05Z

Ah, right. I see the custom getAnchorRect() doesn't work on the Button block because it gets the wrong parent and needs the default behavior instead.
Not sure I see other differences from master (or even previous versions), am I missing something? Aside: the interaction on the Button block would need some work.

afercia · 2018-10-22T06:34:23Z

I've reverted the custom getAnchorRect() as that's not a proper solution. However, the position thing is still a problem, I guess it would be better addressed separately. Also, I'd tend to think the URL input / popover implementation needs some refinement, looking at it holistically. FOr example, I'm not sure I understand why the Button block URL input doesn't have a "Link Settings" button: as a used, I can I set that link to open in a new tab?

youknowriad · 2018-11-08T07:57:38Z

Can you rebase the branch so we can get it in.

…g, add speak message.

afercia · 2018-11-08T08:48:34Z

Rebased. Thank you.

talldan · 2018-11-08T09:00:35Z

I had a test - looks good. A simple solution to the positioning of the autcomplete suggestions might be to make it a child of the editor-url-input div:

Will need to make it a bit narrower, but I think it's still wide enough to cover most post titles.

I think we should tackle that in a separate PR though.

youknowriad

LGTM 👍

afercia · 2018-11-12T15:32:49Z

I think we should tackle that in a separate PR though.

@talldan has a PR been created? I've just noticed one more issue: as the ellipsis button is now placed outside, pressing Escape when it's focused or when its expanded panel is focused, doesn't close the link popover.

talldan · 2018-11-13T09:46:50Z

Hi @afercia - here's a PR tackling that - #11803

I'll also see if I can solve the escape key issue now. (apologies for these regressions)

edit: here's the PR for the escape key issue - #11806

aduth · 2020-01-13T17:20:58Z

makes the TAB key populate the search field with the highlighted link URL [...] adds a speak message to announce the link has been selected, matches the current interaction in the Classic Editor

Was it intentional that we only announce that the link has been selected when explicitly pressing the Tab key, and not when selecting the link via Enter ? I could see an argument being made that since it might not be expected that the link is selected when the focus shifts by pressing Tab, the spoken message helps inform the user about that effect. But if it's unexpected such that it needs to be explained, then it makes me wonder whether it should actually happen at all.

In other words, would you consider either of the following as a reasonable change:

Always announce that the link is selected, regardless of whether it occurs by Tab or by Enter
Or, remove the behavior where a link is selected as a result of pressing Tab

Edit: After spending more time with links (related to #19462), I'm now aware there are expected to be different specific behaviors of Tab and Enter insofar as both "select" a link, but only when pressing Enter is the link actually applied. At least, this is how it works today. It's not necessarily something that the URLInput ought to be opinionated about, considering that it doesn't actually handle the application of the link in response to an Enter press, but I can see how the changes here were modeled after that expected outcome. Technically, since both keys would cause a link to become "selected", it could still be reasonable to expect the spoken message to happen in response to either of the two keys. But I could also see how that might cause some confusion for how the Enter key results not only in the selection of the link, but also the application of that link to the selected text (the submission of the form).

afercia · 2020-01-21T15:20:11Z

Generally, the Tab key should be reserved for keyboard navigation so the most important thing is that it doesn't insert a link.

If I recall correctly, the current behavior is meant to reproduce how wplink always worked in WordPress. That's the behavior keyboard and assistive technology users are used to.

In a way, that's not totally unexpected. Some browsers have the same behavior on some native HTML elements. For example on a <select> element on macOS:

Firefox: pressing Tab selects the highlighted option
Webkit (Chrome, Safari, Opera): pressing Tab doesn't do anything

Worth noting that when pressing Enter, wplink does announce Link inserted. so I'd vote for option 1

Always announce that the link is selected, regardless of whether it occurs by Tab or by Enter

afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release labels Oct 20, 2018

afercia requested review from talldan and aduth October 20, 2018 18:15

youknowriad reviewed Oct 20, 2018

View reviewed changes

youknowriad approved these changes Oct 21, 2018

View reviewed changes

youknowriad requested changes Oct 21, 2018

View reviewed changes

desrosj added this to the WordPress 5.0 milestone Oct 25, 2018

gziolo modified the milestones: WordPress 5.0, 4.4 Nov 5, 2018

youknowriad modified the milestones: 4.4, 4.3 Nov 7, 2018

afercia added 5 commits November 8, 2018 09:20

Fix link search field padding and spinner.

52a24a4

Select the highlighted link when tabbing away, set focus when clickin…

b3a5f1c

…g, add speak message.

Fix links suggestions popover position and size.

d09108e

Avoid events, they're bad.

dc5163a

Revert custom getAnchorRect.

6bd3ae2

afercia force-pushed the fix/link-search-field-and-suggestions-fixes branch from 444e68f to 6bd3ae2 Compare November 8, 2018 08:31

youknowriad approved these changes Nov 8, 2018

View reviewed changes

afercia merged commit e12f2b4 into master Nov 8, 2018

afercia deleted the fix/link-search-field-and-suggestions-fixes branch November 8, 2018 09:04

afercia mentioned this pull request Nov 13, 2018

Fix autocomplete suggestion list positioning #11803

Merged

4 tasks

dimadin mentioned this pull request Dec 3, 2018

Merge similar strings #12540

Merged

aduth mentioned this pull request Jan 13, 2020

Try the link control in the link format #19462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regressions and improve the insert link search field and suggestions list #10838

Fix regressions and improve the insert link search field and suggestions list #10838

afercia commented Oct 20, 2018 •

edited

Loading

youknowriad Oct 20, 2018

afercia Oct 21, 2018

youknowriad Oct 20, 2018

afercia Oct 21, 2018

youknowriad Oct 21, 2018

afercia commented Oct 21, 2018

youknowriad left a comment

youknowriad left a comment

afercia commented Oct 21, 2018

afercia commented Oct 22, 2018

youknowriad commented Nov 8, 2018

afercia commented Nov 8, 2018

talldan commented Nov 8, 2018 •

edited

Loading

youknowriad left a comment

afercia commented Nov 12, 2018

talldan commented Nov 13, 2018 •

edited

Loading

aduth commented Jan 13, 2020 •

edited

Loading

afercia commented Jan 21, 2020

Fix regressions and improve the insert link search field and suggestions list #10838

Fix regressions and improve the insert link search field and suggestions list #10838

Conversation

afercia commented Oct 20, 2018 • edited Loading

youknowriad Oct 20, 2018

Choose a reason for hiding this comment

afercia Oct 21, 2018

Choose a reason for hiding this comment

youknowriad Oct 20, 2018

Choose a reason for hiding this comment

afercia Oct 21, 2018

Choose a reason for hiding this comment

youknowriad Oct 21, 2018

Choose a reason for hiding this comment

afercia commented Oct 21, 2018

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

afercia commented Oct 21, 2018

afercia commented Oct 22, 2018

youknowriad commented Nov 8, 2018

afercia commented Nov 8, 2018

talldan commented Nov 8, 2018 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

afercia commented Nov 12, 2018

talldan commented Nov 13, 2018 • edited Loading

aduth commented Jan 13, 2020 • edited Loading

afercia commented Jan 21, 2020

afercia commented Oct 20, 2018 •

edited

Loading

talldan commented Nov 8, 2018 •

edited

Loading

talldan commented Nov 13, 2018 •

edited

Loading

aduth commented Jan 13, 2020 •

edited

Loading