Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9662 bug a11y focus popup content #9774

Merged
merged 5 commits into from
Oct 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/ui/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ class Map extends Camera {
bindAll([
'_onWindowOnline',
'_onWindowResize',
'_onMapScroll',
'_contextLost',
'_contextRestored'
], this);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -2345,6 +2348,15 @@ class Map extends Camera {
this.fire(new Event('webglcontextrestored', {originalEvent: event}));
}

_onMapScroll(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.
*
Expand Down
6 changes: 0 additions & 6 deletions src/ui/marker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
32 changes: 30 additions & 2 deletions src/ui/popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {PointLike} from '@mapbox/point-geometry';
const defaultOptions = {
closeButton: true,
closeOnClick: true,
focusAfterOpen: true,
className: '',
maxWidth: "240px"
};
Expand All @@ -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]:not([contenteditable='false'])",
"button:not([disabled])",
"input:not([disabled])",
"select:not([disabled])",
"textarea:not([disabled])",
].join(", ");

/**
* A popup component.
*
Expand All @@ -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'`,
Expand Down Expand Up @@ -128,6 +142,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);
Expand Down Expand Up @@ -397,8 +412,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;
}

Expand Down Expand Up @@ -455,14 +473,16 @@ 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';
this._closeButton.setAttribute('aria-label', 'Close popup');
this._closeButton.innerHTML = '×';
this._closeButton.addEventListener('click', this._onClose);
}

}

_onMouseUp(event: MapMouseEvent) {
Expand All @@ -477,7 +497,7 @@ export default class Popup extends Evented {
this._update(event.point);
}

_update(cursor: PointLike) {
_update(cursor: ?PointLike) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

const hasPosition = this._lngLat || this._trackPointer;

if (!this._map || !hasPosition || !this._content) { return; }
Expand Down Expand Up @@ -542,6 +562,14 @@ export default class Popup extends Evented {
applyAnchorClass(this._container, anchor, 'popup');
}

_focusFirstElement() {
if (!this.options.focusAfterOpen || !this._container) return;

const firstFocusable = this._container.querySelector(focusQuerySelector);

if (firstFocusable) firstFocusable.focus();
}

_onClose() {
this.remove();
}
Expand Down
99 changes: 99 additions & 0 deletions test/unit/ui/popup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -651,3 +651,102 @@ 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('<span>Test</span>')
.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('<span>Test</span>')
.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('<span tabindex="0" data-testid="abc">Test</span>')
.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('<span tabindex="-1" data-testid="abc">Test</span>')
.setLngLat([0, 0])
.addTo(createMap(t));

const nonFocusableEl = popup._container.querySelector("[data-testid='abc']");
const closeButton = popup._container.querySelector("button[aria-label='Close popup']");

t.notEqual(window.document.activeElement, nonFocusableEl);
t.equal(window.document.activeElement, closeButton);
t.end();
});

test('If popup contains a disabled button and a focusable element then the latter is focused', (t) => {
const popup = new Popup({closeButton: true})
.setHTML(`
<button disabled>No focus here</button>
<select data-testid="abc">
<option value="1">1</option>
<option value="2">2</option>
</select>
`)
.setLngLat([0, 0])
.addTo(createMap(t));

const focusableEl = popup._container.querySelector("[data-testid='abc']");

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('<span tabindex="0" data-testid="abc">Test</span>')
.setLngLat([0, 0])
.addTo(createMap(t));

t.equal(window.document.activeElement, dummyFocusedEl);
t.end();
});