WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 7 months ago

#33029 reviewing defect (bug)

Dismissable admin notices focus loss

Reported by: afercia Owned by: morganestes
Milestone: Future Release Priority: normal
Severity: normal Version: 4.2
Component: Administration Keywords: needs-testing has-patch
Focuses: ui, accessibility, javascript Cc:

Description

When activating the button to dismiss an admin notice, the currently focused element (the dismiss button itself) disappears hence there's a focus loss. This is more noticeable in some browsers (e.g. Chrome) while other browsers try to be smarter (e.g. Firefox) and keep focus in place.

To reproduce, use Chrome and edit a post, then press the "Update" button and after page refresh a notice will appear. Using your keyboard, tab through the UI elements and focus the dismiss button. Press Enter.

Now tab again and focus will be on the document root. See screenshot:

https://cldup.com/8RQDiHnFkX.png

Worth noting this is not specific to dismissable notices, it happens every time a focused element gets hidden or removed from the DOM. Every time this happens, users will lose context and will be forced to navigate through several elements just to find again the last place from where they left off.

Since it's a general issue in the admin I initially thought to open a general ticket but then feared it would have been too broad. Decided to start with a small, understandable example to start the discussion and hopefully find a nice, simple, solution :)

As a general rule, every time a focused element gets hidden or removed from the DOM, correctly handling the focus and restoring it in the most logical place should be a developer responsibility.
An interesting reading would be what a blind person (and developer, and Mozillian) thinks about this specific issue:
https://www.marcozehe.de/2015/02/05/advanced-aria-tip-2-accessible-modal-dialogs/
while the article focuses on modal dialogs, the part about focus loss is pretty relevant.

Attachments (5)

33029.diff (1.2 KB) - added by McGuive7 2 years ago.
Patch to preserve original focus when dismissing notices.
33029.2.diff (1.1 KB) - added by McGuive7 2 years ago.
Update to simplify (switch from extra mouseup event callback to using click event)
33029.3.diff (1.5 KB) - added by adamsilverstein 2 years ago.
33029.4.alt.diff (2.3 KB) - added by afercia 2 years ago.
33029.5.patch (3.0 KB) - added by afercia 22 months ago.

Download all attachments as: .zip

Change History (26)

#1 @McGuive7
2 years ago

  • Keywords 2nd-opinion added

Curious what other contexts there are in which this is an issue. It seems like there are possibly so many that any sort of general approach wouldn't be possible. Per your comment above:

As a general rule, every time a focused element gets hidden or removed from the DOM, correctly handling the focus and restoring it in the most logical place should be a developer responsibility.

. . . I can at least work on a patch for dismissible notices and we can go from there.

@McGuive7
2 years ago

Patch to preserve original focus when dismissing notices.

#2 @McGuive7
2 years ago

  • Keywords dev-feedback has-patch added; needs-patch removed

Just added a starter patch for this one (and a quick revision). A few things to note:

  1. By the time the click event fires, focus already appears to have been lost, so I'm using the mousedown event to track the previously focused element and add and identifying data attribute. Open to better ways of doing this.
  2. It may make sense to split this out into a more generic JS function that can be applied generally to a wider variety of events for which this behavior is desired. Would love some additional opinions/thoughts on whether or not this is worthwhile, feasible.

@McGuive7
2 years ago

Update to simplify (switch from extra mouseup event callback to using click event)

#3 @adamsilverstein
2 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to morganestes
  • Status changed from new to reviewing

@McGuive7: Thanks for the patch. Tested and this works well if I click the dismiss notice.

While this seems useful in itself, I don't think it resolves the issue raised in the ticket. Using only the keyboard to navigate to the dismiss button, the patch doesn't change the focus behaviour when closing the dismissable notice.

Reading thru the linked document above, the most relevant bit is to set focus back to the element that opened the dialog, or any other useful element from which the user of your app will most likely continue working (emphasis added).

After actioning 'Update' the page refreshes and I am back at the root element. Tabbing back to the dismissable notice, the focus immediately before the dismiss button is the 'View' link which is part of the dismissable notice. Therefore, I think we need to take the approach of focusing back to where the user is likely to continue working - I propose we refocus on the content editor or possibly the title field. I'd still like to keep the click catch return to previous focus in the patch, otherwise we'll wind up tacking focus after click to a possibly different location.

#4 @adamsilverstein
2 years ago

  • Keywords has-patch added; needs-patch removed

In 33029.3.diff:

  • If we don't have a previously focused element captured on mousedown, for example when tabbing to the dismiss button, instead focus back on TinyMCE's active area content and place caret at the end.

Screencast of this in action including key events:

https://cloudup.com/ctxw2K-BbIi

#5 follow-up: @afercia
2 years ago

I'm not sure we can rely on the mousedown event. When an assistive technology (i.e. screen readers) is enabled, it does its things with events and doesn't deliver them the same way as browsers do. Sometimes screen readers don't deliver a keydown or mousedown event at all, see for example this article:
http://unobfuscated.blogspot.it/2013/05/event-handlers-and-screen-readers.html
Though the article focuses on anchor elements, it mentions also buttons. I've struggled a bit looking for other references about screen readers and events, without any luck for now.

Ideally, maybe the best option would be what Firefox does. If you compare Firefox and Chrome behaviors, you will see that after you dismiss the notice and tab (or tab backwards) Firefox will give focus to the next (or previous) focusable element. I love Firefox for this kind of things :) Chrome will move focus to the document root instead.
I'd be interested in understanding how Firefox handle this. It looks like it keeps a sort of focusable elements internal index.

#6 in reply to: ↑ 5 ; follow-up: @adamsilverstein
2 years ago

Interesting - I will test in Firefox!

Thinking we could achieve something similar by leaving a (possible collapsed/empty) element in place, replacing the dismissed element, and keeping the focus there. This would give chrome the behavior you describe firefox having and would likely be a more re-usable pattern.

Replying to afercia:

I'm not sure we can rely on the mousedown event. When an assistive technology (i.e. screen readers) is enabled, it does its things with events and doesn't deliver them the same way as browsers do. Sometimes screen readers don't deliver a keydown or mousedown event at all, see for example this article:
http://unobfuscated.blogspot.it/2013/05/event-handlers-and-screen-readers.html
Though the article focuses on anchor elements, it mentions also buttons. I've struggled a bit looking for other references about screen readers and events, without any luck for now.

Ideally, maybe the best option would be what Firefox does. If you compare Firefox and Chrome behaviors, you will see that after you dismiss the notice and tab (or tab backwards) Firefox will give focus to the next (or previous) focusable element. I love Firefox for this kind of things :) Chrome will move focus to the document root instead.
I'd be interested in understanding how Firefox handle this. It looks like it keeps a sort of focusable elements internal index.

#7 @adamsilverstein
2 years ago

  • Keywords needs-patch added; has-patch removed

#8 @morganestes
2 years ago

I tested @adamsilverstein's patch in Opera on OS X and it works great. I activated the dismiss element with both return and spacebar, and focus was set to the end of the text in TinyMCE.

In Firefox, it works to dismiss with either the enter key or spacebar, then takes me to the editor, but after the post is updated, focus returns to root so I have to tab through all the menus before I can get to the editor again. That seems broken and not at all what @afercia describes.

Last edited 2 years ago by morganestes (previous) (diff)

#9 in reply to: ↑ 6 @afercia
2 years ago

Replying to adamsilverstein:

Thinking we could achieve something similar by leaving a (possible collapsed/empty) element in place, replacing the dismissed element, and keeping the focus there.

@adamsilverstein I think it's a very interesting idea, worth experimenting a bit.

@afercia
2 years ago

#10 follow-up: @afercia
2 years ago

First, very rough, pass for @adamsilverstein idea. Still experimental.
Was thinking this sort of injected "focus placeholder" element can be used to dispatch a confirmation message (optional). Just passing a "Notice dismissed." string to be used as the placeholder content: when focus is moved to the placeholder, the message will be read out by screen readers. Without a message, screen readers may read out "blank", probably not a big deal and maybe someway appropriate. On blur, the placeholder should be removed.

Many things to check, first of all if jQuery insertBefore() is the right thing to use, checking also what it does internally. Initially I thought it could lose the reference node, since we're deleting it but seems to work in latest Chrome, Firefox and also IE 8.

Temporary styling just to make things visible for debugging. All this should be hidden with screen-reader-text of course.
Would appreciate any help, thoughts, feedback and testing :)

#11 @afercia
2 years ago

The focus placeholder inserted in place of the notice:

https://cldup.com/omj-jReMN4.png

#12 in reply to: ↑ 10 @adamsilverstein
2 years ago

  • Keywords needs-testing has-patch added; 2nd-opinion needs-patch removed

Nice! I tested this in Chrome and it works as expected. I'm left with focus right where the dismissed element was - tabbing forward sends me to the title and tabbing backwards sends me to the add new button.

Here is a screencast with key events captured for the record:
https://cloudup.com/cLbKgq5atLv

I looked at the JavaScript, everything looks good to me... I think the use of insertBefore is appropriate here. I like your constructor for focusPlaceHolder!

I haven't tested with a screen reader; the way it works feels great.

Replying to afercia:

First, very rough, pass for @adamsilverstein idea. Still experimental.
Was thinking this sort of injected "focus placeholder" element can be used to dispatch a confirmation message (optional). Just passing a "Notice dismissed." string to be used as the placeholder content: when focus is moved to the placeholder, the message will be read out by screen readers. Without a message, screen readers may read out "blank", probably not a big deal and maybe someway appropriate. On blur, the placeholder should be removed.

Many things to check, first of all if jQuery insertBefore() is the right thing to use, checking also what it does internally. Initially I thought it could lose the reference node, since we're deleting it but seems to work in latest Chrome, Firefox and also IE 8.

Temporary styling just to make things visible for debugging. All this should be hidden with screen-reader-text of course.
Would appreciate any help, thoughts, feedback and testing :)

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


22 months ago

#14 @afercia
22 months ago

  • Keywords dev-feedback removed

Discussed a bit with @azaozz and a couple of points came out:

  • maybe add it to wp.a11y
  • rename it in just focusPlaceholder
  • use a span for the temp element, so it fits "anywhere"

any thoughts (and help!) more than welcome :)

Last edited 22 months ago by afercia (previous) (diff)

@afercia
22 months ago

#15 @afercia
22 months ago

Refreshed patch, implements the 3 points in the comment above. Wondering if wp-a11y should be loaded as dependency of common.js.

#16 @afercia
18 months ago

Worth noting this just landed in Chromium and it would be a huge improvement for accessiblity:
https://bugs.chromium.org/p/chromium/issues/detail?id=454172#c21

#17 @afercia
16 months ago

As far as I see the new, smarter, Chrome focus handling has just landed in Chrome 50, see https://bugs.chromium.org/p/chromium/issues/detail?id=454172 marked as "fixed.
Tested a bit, it is smarter and way better. Would greatly appreciate some testing just to confirm Chrome is not an issue any more.

I still can reproduce the issue in Safari and IE of course (including Edge).
If only all browser implemented this improvement... :)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


15 months ago

#19 @afercia
15 months ago

  • Milestone changed from Awaiting Review to Future Release

Starting from version 50, Chrome implemented a "smarter" focus handling so at the the time of writing this, the focus loss happens just in IE and Safari. Moving to future release as something that we should monitor and maybe implement just for browsers that don't support "smart" focus. Any suggestion (feature detection?) welcome.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 months ago

Note: See TracTickets for help on using tickets.