From b546e0935ad4b204d34552b0f3f74b928b8831ce Mon Sep 17 00:00:00 2001 From: Matej Duracka Date: Wed, 10 Jun 2020 14:06:35 +0300 Subject: [PATCH 1/5] Focus popup content when opened or if DOM changes --- src/ui/popup.js | 32 ++++++++++++++- test/unit/ui/popup.test.js | 81 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/src/ui/popup.js b/src/ui/popup.js index 2732eb82857..bf1d29bb354 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -128,6 +128,7 @@ export default class Popup extends Evented { this._map.on('remove', this.remove); this._update(); + this._focusFirstElement(); if (this._trackPointer) { this._map.on('mousemove', this._onMouseMove); @@ -397,8 +398,11 @@ export default class Popup extends Evented { */ setDOMContent(htmlNode: Node) { this._createContent(); + // The close button should be the last tabbable element inside the popup for a good keyboard UX. this._content.appendChild(htmlNode); + this._createCloseButton(); this._update(); + this._focusFirstElement(); return this; } @@ -455,6 +459,9 @@ export default class Popup extends Evented { } this._content = DOM.create('div', 'mapboxgl-popup-content', this._container); + } + + _createCloseButton() { if (this.options.closeButton) { this._closeButton = DOM.create('button', 'mapboxgl-popup-close-button', this._content); this._closeButton.type = 'button'; @@ -462,7 +469,6 @@ export default class Popup extends Evented { this._closeButton.innerHTML = '×'; this._closeButton.addEventListener('click', this._onClose); } - } _onMouseUp(event: MapMouseEvent) { @@ -477,7 +483,7 @@ export default class Popup extends Evented { this._update(event.point); } - _update(cursor: PointLike) { + _update(cursor: ?PointLike) { const hasPosition = this._lngLat || this._trackPointer; if (!this._map || !hasPosition || !this._content) { return; } @@ -542,6 +548,28 @@ export default class Popup extends Evented { applyAnchorClass(this._container, anchor, 'popup'); } + _focusFirstElement() { + if (!this._container) return; + + // 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 = [ + "a[href]", + "[tabindex]:not([tabindex='-1'])", + "contenteditable", + "button:not([disabled])", + "input:not([disabled])", + "select:not([disabled])", + "textarea:not([disabled])", + ]; + const firstFocusable = this._container.querySelector( + selectors.join(", ") + ); + + if (firstFocusable) firstFocusable.focus(); + } + _onClose() { this.remove(); } diff --git a/test/unit/ui/popup.test.js b/test/unit/ui/popup.test.js index 5de4adc716b..5a59afecbf4 100644 --- a/test/unit/ui/popup.test.js +++ b/test/unit/ui/popup.test.js @@ -651,3 +651,84 @@ test('Popup closes on Map#remove', (t) => { t.ok(!popup.isOpen()); t.end(); }); + +test('Adding popup with no focusable content (Popup#setText) does not change the active element', (t) => { + const dummyFocusedEl = window.document.createElement('button'); + dummyFocusedEl.focus(); + + new Popup({closeButton: false}) + .setText('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + t.equal(window.document.activeElement, dummyFocusedEl); + t.end(); +}); + +test('Adding popup with no focusable content (Popup#setHTML) does not change the active element', (t) => { + const dummyFocusedEl = window.document.createElement('button'); + dummyFocusedEl.focus(); + + new Popup({closeButton: false}) + .setHTML('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + t.equal(window.document.activeElement, dummyFocusedEl); + t.end(); +}); + +test('Close button is focused if it is the only focusable element', (t) => { + const dummyFocusedEl = window.document.createElement('button'); + dummyFocusedEl.focus(); + + const popup = new Popup({closeButton: true}) + .setHTML('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + // Suboptimal because the string matching is case-sensitive + const closeButton = popup._container.querySelector("[aria-label^='Close']"); + + t.equal(window.document.activeElement, closeButton); + t.end(); +}); + +test('If popup content contains a focusable element it is focused', (t) => { + const popup = new Popup({closeButton: true}) + .setHTML('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + const focusableEl = popup._container.querySelector("[data-testid='abc']"); + + t.equal(window.document.activeElement, focusableEl); + t.end(); +}); + +test('Element with tabindex="-1" is not focused', (t) => { + const popup = new Popup({closeButton: true}) + .setHTML('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + const focusableEl = popup._container.querySelector("[data-testid='abc']"); + + t.notEqual(window.document.activeElement, focusableEl); + t.end(); +}); + +test('If popup content contains a disabled input followed by a focusable element then the latter is focused', (t) => { + const popup = new Popup({closeButton: true}) + .setHTML(` + + Test + `) + .setLngLat([0, 0]) + .addTo(createMap(t)); + + const focusableEl = popup._container.querySelector("[data-testid='abc']"); + + t.equal(window.document.activeElement, focusableEl); + t.end(); +}); From 068d10dc2bb87ec1647bf1ad3283cfc565baad65 Mon Sep 17 00:00:00 2001 From: Matej Duracka Date: Wed, 10 Jun 2020 14:45:02 +0300 Subject: [PATCH 2/5] Move the map-scroll reverting code to map.js 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. --- src/ui/map.js | 12 ++++++++++++ src/ui/marker.js | 6 ------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/ui/map.js b/src/ui/map.js index ab184f327b5..45210b40200 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -419,6 +419,7 @@ class Map extends Camera { bindAll([ '_onWindowOnline', '_onWindowResize', + '_onMapScroll', '_contextLost', '_contextRestored' ], this); @@ -2295,6 +2296,8 @@ class Map extends Camera { ['top-left', 'top-right', 'bottom-left', 'bottom-right'].forEach((positionName) => { positions[positionName] = DOM.create('div', `mapboxgl-ctrl-${positionName}`, controlContainer); }); + + this._container.addEventListener('scroll', this._onMapScroll, false); } _resizeCanvas(width: number, height: number) { @@ -2345,6 +2348,15 @@ class Map extends Camera { this.fire(new Event('webglcontextrestored', {originalEvent: event})); } + _onMapScroll(event: Event) { + if (event.target !== this._container) return; + + // Revert any scroll which would move the canvas outside of the view + this._container.scrollTop = 0; + this._container.scrollLeft = 0; + return false; + } + /** * Returns a Boolean indicating whether the map is fully loaded. * diff --git a/src/ui/marker.js b/src/ui/marker.js index 7dcdc22670c..b8cf99aad84 100644 --- a/src/ui/marker.js +++ b/src/ui/marker.js @@ -213,12 +213,6 @@ export default class Marker extends Evented { // prevent focusing on click e.preventDefault(); }); - this._element.addEventListener('focus', () => { - // revert the default scrolling action of the container - const el = this._map.getContainer(); - el.scrollTop = 0; - el.scrollLeft = 0; - }); applyAnchorClass(this._element, this._anchor, 'marker'); this._popup = null; From 6a42beb0aa1c6c40cae82d39b1f90aa8eeca2b1e Mon Sep 17 00:00:00 2001 From: Matej Duracka Date: Wed, 10 Jun 2020 17:15:52 +0300 Subject: [PATCH 3/5] Fix lint and flow bug --- src/ui/map.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ui/map.js b/src/ui/map.js index 45210b40200..ae7c908fcf1 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -2348,10 +2348,10 @@ class Map extends Camera { this.fire(new Event('webglcontextrestored', {originalEvent: event})); } - _onMapScroll(event: Event) { + _onMapScroll(event: *) { if (event.target !== this._container) return; - // Revert any scroll which would move the canvas outside of the view + // Revert any scroll which would move the canvas outside of the view this._container.scrollTop = 0; this._container.scrollLeft = 0; return false; From e1288dc176a97e32836ef8306c8529576ece25e1 Mon Sep 17 00:00:00 2001 From: Matej Duracka Date: Sat, 25 Jul 2020 20:51:06 +0300 Subject: [PATCH 4/5] Add `focusAfterOpen` option to popup + refactor --- src/ui/popup.js | 34 +++++++++++++++++----------------- test/unit/ui/popup.test.js | 13 +++++++++++++ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/ui/popup.js b/src/ui/popup.js index bf1d29bb354..6c3c8d8f4d7 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -17,6 +17,7 @@ import type {PointLike} from '@mapbox/point-geometry'; const defaultOptions = { closeButton: true, closeOnClick: true, + focusAfterOpen: true, className: '', maxWidth: "240px" }; @@ -27,12 +28,23 @@ export type PopupOptions = { closeButton?: boolean, closeOnClick?: boolean, closeOnMove?: boolean, + focusAfterOpen?: boolean, anchor?: Anchor, offset?: Offset, className?: string, maxWidth?: string }; +const focusQuerySelector = [ + "a[href]", + "[tabindex]:not([tabindex='-1'])", + "contenteditable", + "button:not([disabled])", + "input:not([disabled])", + "select:not([disabled])", + "textarea:not([disabled])", +].join(", "); + /** * A popup component. * @@ -43,6 +55,8 @@ export type PopupOptions = { * map is clicked. * @param {boolean} [options.closeOnMove=false] If `true`, the popup will closed when the * map moves. + * @param {boolean} [options.focusAfterOpen=true] If `true`, the popup will try to focus the + * first focusable element inside the popup. * @param {string} [options.anchor] - A string indicating the part of the Popup that should * be positioned closest to the coordinate set via {@link Popup#setLngLat}. * Options are `'center'`, `'top'`, `'bottom'`, `'left'`, `'right'`, `'top-left'`, @@ -549,23 +563,9 @@ export default class Popup extends Evented { } _focusFirstElement() { - if (!this._container) return; - - // 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 = [ - "a[href]", - "[tabindex]:not([tabindex='-1'])", - "contenteditable", - "button:not([disabled])", - "input:not([disabled])", - "select:not([disabled])", - "textarea:not([disabled])", - ]; - const firstFocusable = this._container.querySelector( - selectors.join(", ") - ); + if (!this.options.focusAfterOpen || !this._container) return; + + const firstFocusable = this._container.querySelector(focusQuerySelector); if (firstFocusable) firstFocusable.focus(); } diff --git a/test/unit/ui/popup.test.js b/test/unit/ui/popup.test.js index 5a59afecbf4..b23791827f3 100644 --- a/test/unit/ui/popup.test.js +++ b/test/unit/ui/popup.test.js @@ -732,3 +732,16 @@ test('If popup content contains a disabled input followed by a focusable element t.equal(window.document.activeElement, focusableEl); t.end(); }); + +test('Popup with disabled focusing does not change the active element', (t) => { + const dummyFocusedEl = window.document.createElement('button'); + dummyFocusedEl.focus(); + + new Popup({closeButton: false, focusAfterOpen: false}) + .setHTML('Test') + .setLngLat([0, 0]) + .addTo(createMap(t)); + + t.equal(window.document.activeElement, dummyFocusedEl); + t.end(); +}); From 4597c5b53555411f163176e8126ddee3303b3227 Mon Sep 17 00:00:00 2001 From: Matej Duracka Date: Wed, 7 Oct 2020 11:07:06 +0300 Subject: [PATCH 5/5] fix contenteditable selector, improve tests --- src/ui/popup.js | 2 +- test/unit/ui/popup.test.js | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/ui/popup.js b/src/ui/popup.js index 6c3c8d8f4d7..092af27c96a 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -38,7 +38,7 @@ export type PopupOptions = { const focusQuerySelector = [ "a[href]", "[tabindex]:not([tabindex='-1'])", - "contenteditable", + "[contenteditable]:not([contenteditable='false'])", "button:not([disabled])", "input:not([disabled])", "select:not([disabled])", diff --git a/test/unit/ui/popup.test.js b/test/unit/ui/popup.test.js index b23791827f3..ed56ed43f9b 100644 --- a/test/unit/ui/popup.test.js +++ b/test/unit/ui/popup.test.js @@ -712,17 +712,22 @@ test('Element with tabindex="-1" is not focused', (t) => { .setLngLat([0, 0]) .addTo(createMap(t)); - const focusableEl = popup._container.querySelector("[data-testid='abc']"); + const nonFocusableEl = popup._container.querySelector("[data-testid='abc']"); + const closeButton = popup._container.querySelector("button[aria-label='Close popup']"); - t.notEqual(window.document.activeElement, focusableEl); + t.notEqual(window.document.activeElement, nonFocusableEl); + t.equal(window.document.activeElement, closeButton); t.end(); }); -test('If popup content contains a disabled input followed by a focusable element then the latter is focused', (t) => { +test('If popup contains a disabled button and a focusable element then the latter is focused', (t) => { const popup = new Popup({closeButton: true}) .setHTML(` - Test + `) .setLngLat([0, 0]) .addTo(createMap(t));