update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343

BoardJames · 2017-11-06T05:15:49Z

Description

As requested in issue #3329 I have changed the structure for the autocomplete menu to be a div with buttons. This will hopefully improve how screen readers respond so they note that the button is clickable though I did not notice any changed in the behaviour of NVDA.

How Has This Been Tested?

Existing automated tests cover the autocomplete menu. I have also manually verified the changes in Firefox with NVDA.

Screenshots (jpeg or gifs if applicable):

Types of changes

Minor changes to structure and CSS of the autocomplete menu which might be considered a bug fix.

Checklist:

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

…utton" This will hopefully improve how screen readers respond so they note that the button is clickable.

codecov · 2017-11-06T05:15:52Z

Codecov Report

Merging #3343 into master will increase coverage by <.01%.
The diff coverage is 76.66%.

@@            Coverage Diff            @@
##           master   #3343      +/-   ##
=========================================
+ Coverage    33.8%   33.8%   +<.01%     
=========================================
  Files         249     249              
  Lines        6713    6733      +20     
  Branches     1207    1216       +9     
=========================================
+ Hits         2269    2276       +7     
- Misses       3754    3764      +10     
- Partials      690     693       +3

Impacted Files	Coverage Δ
blocks/autocompleters/index.js	`0% <ø> (ø)`	⬆️
components/autocomplete/index.js	`73.26% <76.66%> (-4.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04ef252...bdeff5a. Read the comment docs.

afercia · 2017-11-06T14:16:26Z

From an a11y perspective looks good to me, thanks! Tested with Safari+VoiceOver and Firefox+NVDA. The last combo is also able to announce the item number within the set of results.

It would be great to add a speak message to announce the total number of results, as done for the link and tags suggestions. Something like nn results found, use up and down arrow keys to navigate. Please let me know if you want me to open a separate issue.

I see a small visual glitch, see last screenshot. When username/first name/last name are very long and depending also on the option Display name publicly as, the text in a suggestion can be very long. In Chrome, the text can overflow from the "popover". In Firefox, the images shrink: I guess they need flex: 0 0 auto;.

Images with very long suggestions text in Firefox:

Also, In Safari, the after selecting an user from the suggestion (either by clicking or via the keyboard) the caret position is at the beginning of the inserted user name, before the "at" sign. I think it's unrelated to this PR though (boundaries?).

Safari caret position after inserting an username:

Question: from a functional point of view, what happens when there are hundreds/thousands of users on a site? What is the max number of users returned by the API? Would the API need some adjustments?

afercia

I think at least the CSS issue should be fixed in this PR. I guess the other points would need their own separate issues.

BoardJames · 2017-11-06T23:14:48Z

Hmmm, looks like I need to set flex-shrink to 0.

Question: from a functional point of view, what happens when there are hundreds/thousands of users on a site? What is the max number of users returned by the API? Would the API need some adjustments?

I don't know anything about the collections API but the autocomplete popup only shows 10 items at a time so you would need to filter them by typing.

…vement

BoardJames · 2017-11-07T00:51:55Z

I have made it handle long names.

The profile image will always stay the same size (24px x 24px). If the display name is longer than 200px it will get truncated (by css, I am hoping it will still read it) and if the slug is longer than 100px it will be truncated. Both the display name and the slug have white-space: nowrap set.

…ken; Filtering is now done on typing and load instead of on rendering; List item id is now tied to key instead of index so it will change on filtering which will hopefully lead to screen readers noticing the change more often

BoardJames · 2017-11-07T06:53:05Z

I've added a few more changes.

The list count will now be spoken just like the tags input
Pressing ESC will now suppress the popup (not just hide it until a new character is typed)
Pressing CTRL+SPACE while the popup is suppressed will show it again.
The ID assigned to the list items should now be guaranteed to be different for different list items whereas before it depended on the position in the filtered list which meant that it could stay the same when the options changed due to filtering
Filtering is done on typing instead of render which I think will be more efficient (and it made implementing the spoken count easier).

…vement

BoardJames · 2017-11-07T07:09:12Z

I haven't solved the Safai cursor position problem yet - in theory it shouldn't be happening because the link is inserted before the @query and then the @query is removed so there should be no way for the cursor to end up before the link. I will probably have to set the selection to fix it.

afercia · 2017-11-07T15:56:27Z

The list count will now be spoken just like the tags input

Thanks! The results are nicely announced, tested with Safari+VoiceOver and Firefox+NVDA.

Pressing CTRL+SPACE while the popup is suppressed will show it again.

This doesn't seem to work for me. When previously pressing Escape, the block is not focused any longer so CTRL+SPACE doesn't do anything for me.

From an a11y perspective, everything seems to work fine to me. I'd leave the overall technical review to someone with more technical expertise than me :)

I'd suggest to consider just one small usability improvement: on Twitter, typing @ doesn't do anything and the suggestions list is displayed only after typing at least one character after the @. I'd consider to do the same here.

afercia · 2017-11-07T16:06:30Z

Note: on Safari 11.0.1 and only when using VoiceOver, the boundaries don't work at all for me: no highlighting is displayed on both user names and links. No "link modal" appears. They work on Safari Technology Preview – Release 43 (Safari 11.1, WebKit 12605.1.12). Seems to me something that should be monitored in a separate issue.a

BoardJames · 2017-11-08T06:54:58Z

I'd suggest to consider just one small usability improvement: on Twitter, typing @ doesn't do anything and the suggestions list is displayed only after typing at least one character after the @. I'd consider to do the same here.

Both Facebook and Slack display a list after only typing the '@'. I tried it anyway since it was technically a very easy change and it just feels wrong to not have the pop-up appear so I'm going to not implement that.

Note if you want to try it out then modify line 284-286 of components/autocomplete/index.js to:

			if ( text.substr( pos + 1, triggerPrefix.length ) !== triggerPrefix ||
					text.length <= ( pos + 1 + triggerPrefix.length ) ) {
				return false;
			}

afercia · 2017-11-08T08:16:05Z

Thanks! Fair enough for the first character :) I don't have a strong preference, to me the Twitter behavior seems more intuitive but since it's an easy change, can also be evaluated in the future.

Two more considerations:
Should the "search term" match usernames that contain the search term or usernames that starts with it? At the moment, seems to me it uses "contain", see below searching for "me":

it returns usernames containing "me". Instead, seems to me both Slack and Twitter search for matches that start with the search term. As far as I see, they search for name or last name or "slug" starting with the typed search term. Thoughts? /cc @jasmussen

Minor:
I'd consideer to reset the border radius on the focused search result: all similar menus in Gutenberg (e.g. block switcher menu, edit table menu, use squared borders.

jasmussen · 2017-11-08T08:32:00Z

I'm general for the auto complete stuff, the behavior I'd mimic is that of IRC and slack when you type /. So yes, more like auto complete, less of a search. Nice work though.

@iseulde should also be heard on the syntactic sugar like how to start quotes, lists and headings. She has a ton of experience on how that works best.

Regarding the border radii, that's a regression in master. Okay to fix, okay to ignore also, as I'm working separately on improving all those focus styles in a separate pr.

…vement

BoardJames · 2017-11-09T04:55:22Z

@jasmussen I have changed it to only start matches on word boundaries or after spaces so it won't match things in the middle of words any more.

jasmussen · 2017-11-09T07:22:18Z

Nice, that works well! I'd guess this is ready for final code review. Nice work!

afercia · 2017-11-09T12:17:42Z

Tested again also on Windows and from an accessibility perspective, it works pretty well. Thanks!

Unfortunately, with IE 11 nothing happens 😞 no suggestions list (seems to me it's not rendered in the DOM), no speak message, nothing at all.

IE 11:

For reference: Firefox and NVDA announce everything pretty well:

aduth · 2017-11-09T19:26:48Z

components/autocomplete/index.js

-		if ( options.length === 0 ) {
+		if ( suppress === open.idx ) {
+			const { keyCode, ctrlKey, shiftKey, altKey, metaKey } = event;
+			if ( keyCode === SPACE && ctrlKey && ! ( shiftKey || altKey || metaKey ) ) {


I'm wondering about the SPACE check here; if the user moves focus out of the block after dismissing suggestions and then returns later, shouldn't we expect the dismissal to no longer take effect?

In a future refactor, to simplify the handling of modifier keys, we might consider adding support to <KeyboardShortcuts /> to monitor key events of children, mentioned originally in implementation notes #1944 . Made more difficult here by the fact that we manually create the DOM event handlers.

Yep, that still hidden after move thing is a bug.

The bug should be fixed.

mcsf · 2017-11-09T21:48:16Z

blocks/autocompleters/style.scss

+		width: 24px; // avoid jarring resize by seting the size upfront
+		height: 24px;
+	}
+	.name {


Although they weren't introduced in this PR, these classnames don't adhere to Gutenberg's BEM-like conventions. We should have something along the lines of components-autocomplete__result-name.

Hmmm, reading that document I'd come up with blocks-autocompleters__user-name because the actual CSS is not in the components/autocomplete folder but the blocks/autocompleters folder. @mcsf Does that sound right to you?
I would also have blocks-autocompleters__user-slug and blocks-autocompleters__user-avatar.

I have renamed the classes. I also tweaked where the completers' className is applied in the Autocompleter structure so their css won't care about the Autocompleter's class names.

mcsf · 2017-11-09T21:54:52Z

components/autocomplete/index.js

+		// filter the options we already have
+		const filteredOptions = open ? filterOptions( search, this.state[ 'options_' + open.idx ] ) : [];
+		// check if we should still suppress the popover
+		const suppress = open && wasSuppress === open.idx ? wasSuppress : undefined;


I suggest adding parentheses to make eval order really obvious, instead of relying on precedence rules. One level is enough: ( a && b === c ) ? d : e

…vement

BoardJames · 2017-11-10T06:44:56Z

@aduth It seems that your new test "closes by blur" doesn't like my changes in this branch but I can't figure out why. Any ideas?

…vement

BoardJames · 2017-11-13T02:00:43Z

I figured out that the "closes by blur" test was failing because I had put the withSpokenMessages HOC before the withFocusOutside in the evaluation order and withFocusOutside was depending on calling the handleFocusOutside method of the child component (which withFocusOutside did not have).

BoardJames · 2017-11-13T02:02:06Z

I think I have addressed all review feedback so I will merge.

aduth · 2017-11-13T15:21:02Z

blocks/autocompleters/style.scss

-	color: $dark-gray-100;
-}
+.blocks-autocompleters__user {
+	.blocks-autocompleters__user-avatar {


One of the benefits of using a BEM convention is that it eliminates the need for nesting in many cases, so this could be moved to a top-level style definition.

aduth · 2018-01-08T16:50:12Z

components/autocomplete/index.js

 	withInstanceId,
-	withFocusOutside,
+	withFocusOutside, // this MUST be the innermost HOC as it calls handleFocusOutside


Alternatively we could consider incorporating something like hoist-non-react-statics so we don't depend on a particular order for composing higher-order components.

https://www.npmjs.com/package/hoist-non-react-statics

tiny-james added 2 commits November 6, 2017 14:44

update/3329: Changed autocomplete menu "ul > li > button" to "div > b…

829dfea

…utton" This will hopefully improve how screen readers respond so they note that the button is clickable.

update/3329: Updated selector to be more specific to fix tests

3c6b8f9

BoardJames added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 6, 2017

BoardJames self-assigned this Nov 6, 2017

BoardJames requested a review from afercia November 6, 2017 05:15

afercia requested changes Nov 6, 2017

View reviewed changes

tiny-james added 2 commits November 7, 2017 10:36

update/3329: Fix styling - images don't shrink, text has max-width

570fbee

Merge branch 'master' into update/3329-autocomplete-aria-markup-impro…

73a6231

…vement

tiny-james added 2 commits November 7, 2017 16:36

update/3329: specify profile picture size to avoid resizing after load

c229639

Merge branch 'master' into update/3329-autocomplete-aria-markup-impro…

7d8aedb

…vement

BoardJames mentioned this pull request Nov 7, 2017

Add/2793 hashlink autocompletion #3324

Closed

3 tasks

afercia approved these changes Nov 7, 2017

View reviewed changes

afercia mentioned this pull request Nov 7, 2017

Link boundaries don't work with Safari 11.0.1 when using VoiceOver #3373

Closed

BoardJames requested review from ephox-mogran, tg-ephox, youknowriad, aduth and mcsf and removed request for ephox-mogran and tg-ephox November 8, 2017 04:37

tiny-james added 2 commits November 9, 2017 14:02

Merge branch 'master' into update/3329-autocomplete-aria-markup-impro…

49413dc

…vement

update/3329: Autocompleter matches after word boundaries or space

334ed13

aduth approved these changes Nov 9, 2017

View reviewed changes

aduth mentioned this pull request Nov 9, 2017

Autocomplete: Assign ARIA attributes as combobox, with TinyMCE node mirroring #2789

Closed

mcsf reviewed Nov 9, 2017

View reviewed changes

tiny-james added 2 commits November 10, 2017 15:00

Merge branch 'master' into update/3329-autocomplete-aria-markup-impro…

5c2570c

…vement

update/3329: Fix the list staying suppressed when the cursor is moved

45c3a55

tiny-james added 4 commits November 13, 2017 10:21

Merge branch 'master' into update/3329-autocomplete-aria-markup-impro…

0d0e8f2

…vement

update/3329: fixed withFocusOutside position in call order

b3325c1

update/3329: Renamed classes to follow BEM style guide

5976604

update/3329: Bracketed test of turnary statement to improve readability

bdeff5a

BoardJames merged commit 380fde1 into master Nov 13, 2017

BoardJames deleted the update/3329-autocomplete-aria-markup-improvement branch November 13, 2017 02:02

aduth reviewed Nov 13, 2017

View reviewed changes

aduth reviewed Jan 8, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343

update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343

BoardJames commented Nov 6, 2017

codecov bot commented Nov 6, 2017 •

edited

Loading

afercia commented Nov 6, 2017

afercia left a comment

BoardJames commented Nov 6, 2017

BoardJames commented Nov 7, 2017 •

edited

Loading

BoardJames commented Nov 7, 2017

BoardJames commented Nov 7, 2017 •

edited

Loading

afercia commented Nov 7, 2017

afercia commented Nov 7, 2017

BoardJames commented Nov 8, 2017

afercia commented Nov 8, 2017

jasmussen commented Nov 8, 2017

BoardJames commented Nov 9, 2017

jasmussen commented Nov 9, 2017

afercia commented Nov 9, 2017

aduth Nov 9, 2017

aduth Nov 9, 2017

BoardJames Nov 10, 2017

BoardJames Nov 10, 2017

mcsf Nov 9, 2017

BoardJames Nov 13, 2017

BoardJames Nov 13, 2017

mcsf Nov 9, 2017

BoardJames Nov 13, 2017

BoardJames commented Nov 10, 2017

BoardJames commented Nov 13, 2017

BoardJames commented Nov 13, 2017

aduth Nov 13, 2017

aduth Jan 8, 2018

update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343

update/3329: Changed autocomplete menu "ul > li > button" to "div > button" #3343

Conversation

BoardJames commented Nov 6, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

codecov bot commented Nov 6, 2017 • edited Loading

Codecov Report

afercia commented Nov 6, 2017

afercia left a comment

Choose a reason for hiding this comment

BoardJames commented Nov 6, 2017

BoardJames commented Nov 7, 2017 • edited Loading

BoardJames commented Nov 7, 2017

BoardJames commented Nov 7, 2017 • edited Loading

afercia commented Nov 7, 2017

afercia commented Nov 7, 2017

BoardJames commented Nov 8, 2017

afercia commented Nov 8, 2017

jasmussen commented Nov 8, 2017

BoardJames commented Nov 9, 2017

jasmussen commented Nov 9, 2017

afercia commented Nov 9, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoardJames commented Nov 10, 2017

BoardJames commented Nov 13, 2017

BoardJames commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2017 •

edited

Loading

BoardJames commented Nov 7, 2017 •

edited

Loading

BoardJames commented Nov 7, 2017 •

edited

Loading