9662 bug a11y focus popup content #9774

watofundefined · 2020-06-10T13:02:43Z

Hi folks - this is my first PR - so please shout if I missed some of your rules or anything!

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
manually test the debug page

PR for bug: #9662

Commit `b546e09`

Q1: Should consumers be able to disable this auto-focus functionality?

Q2 Should Esc close the opened popup? Currently it doesn't.

Otherwise it's nothing big - for better keyboard UX I changed the order in which elements in popup are added to DOM - this shouldn't change anything since the close button has position: absolute;.

I also wanted to add functionality that re-focuses the previously focused element when popup is closed.
But it was getting messy in one scenario:

user opens a popup -> popup stores a reference to a previously focused element
user clicks on a different marker -> new popup is opened and after that the old popup is removed - this would result in a situation where the new popup would store a reference to a focusable element from the old popup (already bad but not too bad)
user clicks on a different marker -> same thing happens but since the new popup is added before the old one is removed we would first focus element in the new popup and then the popup which is on its way to DOM graveyard would jump in and it would try to re-focus the element from the very first popup which is long gone - so the third popup would lose focus and document.activeElement would be either null or document.body.

Both popup's removal (popup.js) and popup's appearing (marker.js) are driven by map's "click" event and I don't see an easy way how to ensure they happen in different order.

So I was thinking that the marker could pass a reference to its DOM element to the popup instance.
Then popup would focus it when it'll be closed - but only if popup was closed by the 'x' button - i.e. not by clicking on another marker.
It' an easy change and it would give users good keyboard UX. (I'm happy to add it to the PR if you think it's a good idea)

Commit 068d10d

I added a lot of details to the commit message but I don't know mapbox-gl-js that well so please think if there are some valid cases when map's container is scrolled (it's the div with id "mapid")

The solution in marker.js is good but while testing my changes I found that the bug it tries to prevent happens also when popup content is focused. It's just less likely to happen - user opens a popup containing a focusable element, then zooms in so that it's outside of the view and then tabs until the element in popup received focus. So instead of duplicating the logic in popup I moved it to map.js. Just as a small reminder about what that bug is about: When user tab-focuses an element that's outside of the map view, browser scrolls to make it visible which moves the canvas fully/partially outside of the view.

ryanhamley

Hi @watofundefined thanks for the contribution and for your patience. I think overall this seems pretty good. I noticed one situation where the focus seems to not work like I'd expect though.

Open a popup by either clicking on the marker or by focusing it and pressing Enter.
Click elsewhere on the map to close the popup.
Repeat step 1 (can be the same marker or a different one).
Note that there's no focused element in the popup.
Press tab and notice that the second focusable element comes into focus.

It seems like this code will need to clear or reset the focus when a popup is closed.

I also wanted to add functionality that re-focuses the previously focused element when popup is closed.

I think this is unnecessary, at least in this PR.

Q1: Should consumers be able to disable this auto-focus functionality?

I'd say yes.

Q2 Should Esc close the opened popup? Currently it doesn't.

Let's leave this out of this PR.

src/ui/popup.js

ryanhamley · 2020-07-24T23:11:42Z

src/ui/popup.js

@@ -477,7 +483,7 @@ export default class Popup extends Evented {
        this._update(event.point);
    }

-    _update(cursor: PointLike) {
+    _update(cursor: ?PointLike) {


Did something change that made it necessary to make this a Maybe type?

I should have annotated this - it's called without any arguments several times even within this file, so I couldn't resist fixing the type. I can revert it, to make the commit more focused!

ryanhamley · 2020-07-24T23:14:03Z

src/ui/popup.js

+        // This approach isn't covering all the quirks and cases but it should be good enough.
+        // If we would want to be really thorough we would need much more code, see e.g.:
+        // https://github.com/angular/components/blob/master/src/cdk/a11y/interactivity-checker/interactivity-checker.ts
+        const selectors = [


There's no need to allocate this array every time this function is called. We could move it out of the function. You could also save it as a querySelector to avoid joining the array every time.

I moved it to the top-level scope - leaving the [item1, item2].join(', ') for better readability.
Let me know if that's fine with you - I can change it to whatever you think works better with the codebase.
I did try one long string, but it was around 190 characters which is just too much, and two strings on two lines didn't look right to me.

watofundefined · 2020-07-25T17:53:46Z

Hey @ryanhamley, no problem at all, thanks for feedback!

I've added a new popup option focusAfterOpen, defaulting to true.

As for the bug, I can replicate the behaviour you describe in both Firefox and Chrome.
It's a weird one though - it seems like it's just the browser's default outline style that's missing.
When I check document.activeElement in console, I can see that the element is focused correctly.
When I add a:focus {background-color: red;}, it's also applied correctly.

Here's what I tried (without luck):

setting focus on body when popup's closed, no luck there.
wrapping the initial focus() in setTimeout to see if it's a timing issue

Any ideas?

Cheers!

tristen

👋 @watofundefined apologies on the delay here. @ryanhamley asked me to give this a second set of eyes.

Weirdly, I can't reproduce the bug you and @ryanhamley ran into. If document.activeElement is still in focus correctly and its the browsers default handling of outline I personally think it's fine as is.

This overall looks great! Just had some small requests in the tests. Let's get this merged!

test/unit/ui/popup.test.js

tristen · 2020-09-21T15:39:04Z

test/unit/ui/popup.test.js

+    const popup = new Popup({closeButton: true})
+        .setHTML(`
+            <button disabled>No focus here</button>
+            <span tabindex="0" data-testid="abc">Test</span>


For variance, Mind changing this to a different focusable element? like a contenteditable region or a:href?

It seems contenteditable is not supported by jsdom so I used select. But while playing around with contenteditable I've noticed that the selector is wrong - hence the update in popup.js.

I tested it locally and it works as expected for these variants:

<div contenteditable>...</div> (focuses)

<div contenteditable="true">...</div> (focuses)

<div contenteditable="false">...</div> (doesn't focus)

tristen · 2020-10-05T16:32:06Z

👋 @watofundefined just checking in. This looks close! are you able to make these remaining adjustments?

watofundefined · 2020-10-07T08:15:08Z

Hey @tristen, sorry for the delay, it totally slipped my mind.
Have a look at the latest commit and let me know if you spot anything else!

tristen

@watofundefined awesome! I'll let @ryanhamley hit the merge but these changes look great to me

Matej Duracka added 3 commits June 10, 2020 14:06

Focus popup content when opened or if DOM changes

b546e09

Fix lint and flow bug

6a42beb

arindam1993 changed the base branch from master to main June 18, 2020 18:13

asheemmamoowala requested a review from ryanhamley June 24, 2020 17:46

ryanhamley suggested changes Jul 24, 2020

View reviewed changes

Add focusAfterOpen option to popup + refactor

e1288dc

ryanhamley mentioned this pull request Sep 18, 2020

Fix some failed results from Web map tools WCAG 2.1 evaluation #9991

Merged

10 tasks

tristen requested changes Sep 21, 2020

View reviewed changes

fix contenteditable selector, improve tests

4597c5b

tristen approved these changes Oct 7, 2020

View reviewed changes

ryanhamley approved these changes Oct 7, 2020

View reviewed changes

ryanhamley added the feature 🍏 label Oct 7, 2020

ryanhamley merged commit f0cc015 into mapbox:main Oct 7, 2020

avpeery mentioned this pull request Aug 9, 2021

Opening a marker's pop-up should set focus to the pop-up's content #9662

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9662 bug a11y focus popup content #9774

9662 bug a11y focus popup content #9774

watofundefined commented Jun 10, 2020 •

edited

Loading

ryanhamley left a comment

ryanhamley Jul 24, 2020

watofundefined Jul 25, 2020

ryanhamley Jul 24, 2020

watofundefined Jul 25, 2020

watofundefined commented Jul 25, 2020

tristen left a comment

tristen Sep 21, 2020

watofundefined Oct 7, 2020

tristen commented Oct 5, 2020

watofundefined commented Oct 7, 2020

tristen left a comment

9662 bug a11y focus popup content #9774

9662 bug a11y focus popup content #9774

Conversation

watofundefined commented Jun 10, 2020 • edited Loading

Launch Checklist

Commit b546e09

Commit 068d10d

ryanhamley left a comment

Choose a reason for hiding this comment

ryanhamley Jul 24, 2020

Choose a reason for hiding this comment

watofundefined Jul 25, 2020

Choose a reason for hiding this comment

ryanhamley Jul 24, 2020

Choose a reason for hiding this comment

watofundefined Jul 25, 2020

Choose a reason for hiding this comment

watofundefined commented Jul 25, 2020

tristen left a comment

Choose a reason for hiding this comment

tristen Sep 21, 2020

Choose a reason for hiding this comment

watofundefined Oct 7, 2020

Choose a reason for hiding this comment

tristen commented Oct 5, 2020

watofundefined commented Oct 7, 2020

tristen left a comment

Choose a reason for hiding this comment

watofundefined commented Jun 10, 2020 •

edited

Loading

Commit `b546e09`