open popup on a marker from keyboard

[andrewharvey]

Launch Checklist

closes #4678

• briefly describe the changes in this PR
  allows popups bound to markers to be opened via the keyboard
• write tests for all new functionality
• n/a document any changes to public APIs
• n/a post benchmark scores
• manually test the debug page
• n/a tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
• doesn't do anything without a tabindex on the element so it can be active via the keyboard, need to think about how that should play into this PR it now sets tabindex, but need to think more about wider impact this has when developers add their own event listeners. but only if not already set.

[mourner]

If you call setPopup multiple times (e.g. switching between different popup objects), the keyboard listeners will pile up. How can we avoid that?

[mourner]

doesn't do anything without a tabindex on the element so it can be active via the keyboard, need to think about how that should play into this PR

Leaflet sets tabIndex to '0' for markers by default so that they're accessible, with an ability to turn it off in options. Maybe we should do the same?

[ryanhamley]

@mourner @andrewharvey It looks like if a popup is already bound to the marker, it's being removed when setPopup is called again in line 270. We could just move the listener function into a variable then call this._element.removeEventListener('keypress', listenerFn); inside that if statement to avoid the listeners piling up.

[andrewharvey]

Leaflet sets tabIndex to '0' for markers by default so that they're accessible, with an ability to turn it off in options. Maybe we should do the same?

I think if there are any click or hover events registered on the marker, then yes it should have a tabindex to be accessible, but if it's a marker that you can't interact with then it shouldn't.

I've made it set and unset the tabindex based on a popup attached to the marker, but I agree we probably still need a new option to prevent this interacting with anything developers are doing in terms of their own event listeners to the marker element unknown to GL JS.

PS. This PR is still very much a WIP.

[mourner]

PS. This PR is still very much a WIP.

@andrewharvey Any updates on this one?

[andrewharvey]

@andrewharvey Any updates on this one?

No sorry, I haven't been able to prioritise picking this up again, but still keen on seeing a solution.

[asheemmamoowala]

@andrewharvey Still WIP?

[andrewharvey]

Thanks for the reminders @mourner @asheemmamoowala. I've taken another look at this, and tested it out again, and it's ready for review/merge now please.

[mourner]

As far as I remember, generic click event is triggered when you press either enter or space when the element is focused — the only thing it needs is a tabIndex: 0. E.g. that's how Leaflet handles marker keyboard events. So do we have to explicitly put keypress listeners then?

[andrewharvey]

As far as I remember, generic click event is triggered when you press either enter or space when the element is focused — the only thing it needs is a tabIndex: 0. E.g. that's how Leaflet handles marker keyboard events. So do we have to explicitly put keypress listeners then?

Probably, but because of 👇 there is no click listener on the element.

mapbox-gl-js/src/ui/marker.js

Lines 207 to 210 in 8f0998d

	// If we attached the `click` listener to the marker element, the popup
	// would close once the event propogated to `map` due to the
	// `Popup#_onClickClose` listener.
	this._map.on('click', this._onMapClick);

For that reason the popup's aren't being activated on enter or space, so we workaround the fact that we're not using a click listener on the element by watching for a space or enter keypress.

[mourner]

If we attached the click listener to the marker element, the popup would close once the event propogated to map due to the Popup#_onClickClose listener.

Hmm... I wonder if we could try fixing the root problem here instead of the hacky workaround? E.g. involving stopPropagation to prevent the click propagating to map?

[mourner]

OK, did another review pass and this looks great, apart from one thing — when tabbing through markers with a keyboard, if a marker is not fully within the current view, the canvas container will scroll to accommodate it (default focus action), breaking the map. We should add some code to prevent this (likely involving focus event and e.preventDefault).

[andrewharvey]

when tabbing through markers with a keyboard, if a marker is not fully within the current view, the canvas container will scroll to accommodate it (default focus action), breaking the map. We should add some code to prevent this (likely involving focus event and e.preventDefault).

Good catch. Since focus is not cancelable I couldn't work out how to stop the scroll.

I was able to get the map to panBy just enough to move the element into view on focusin so that it no longer to scrolls the page to fit the Marker as the Marker is now already within view. Although this does change behaviour, I think this is reasonable.

If that's okay, I suppose I should extend that to work for the left, top and right borders too to be consistent.

There's still a question on if we should disable this for Markers completely outside the view, but I'm hesitant to disable that since tab cycling through Markers, I think should continue even if outside the viewport, so then that would mean the map might pan a lot to get these Markers into view, but I think that's okay.

[andrewharvey]

... experimenting with ^ in d1a2a5b but it does have some downsides when the Marker is larger than the canvas.

[mourner]

@andrewharvey the focus event is not preventable, but we might be able to prevent the scrolling action itself — e.g. there's this preventScroll polyfill we can take some clues from: https://github.com/calvellido/focus-options-polyfill

[andrewharvey]

Thanks for the tips, I investigated and as far as I can tell, we can only use preventScroll option or the polyfill when manually triggering a focus with Element.focus(), you can't do that when the browser has invoked the focus event, since there's no way to preventDefault when a focus event is triggered we can't seem to stop the browser scrolling.

The polyfill grabs the window.scrollY then sets the window.scroll to the same, but that doesn't help here (at least for full screen maps) because the page isn't actually scrolled, and scrollY is still 0 before and after the focus.

It feels like a browser bug since it should respect the absolute position of the map at 0,0 regardless if we focus on a child that's positioned outside...

Maybe there is a solution, but I can't work it out.

[mourner]

OK, I think this is ready to merge:

• Rebased on master and fixed linting issues.
• Found a way to fix the scrolling issue by resetting the container scroll position on focus.
• Made it so that the markers aren't focused when you click on them — the outline may be distracting for non-keyboard users and this seems to be consistent with how browsers behave with native elements like button (not focusing unless you explicitly tab to it).

[mourner]

Merged — let's follow up with any remaining issues about keyboard accessibility of markers in other issues/PRs.

[andrewharvey]

@mourner That's smart, I didn't think of setting the scroll of the map Element itself. Thank you.