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.

Sign up for GitHub

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

9662 bug a11y focus popup content #9774

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b546e09
Focus popup content when opened or if DOM changes
Jun 10, 2020
068d10d
Move the map-scroll reverting code to map.js
Jun 10, 2020
6a42beb
Fix lint and flow bug
Jun 10, 2020
e1288dc
Add `focusAfterOpen` option to popup + refactor
Jul 25, 2020
4597c5b
fix contenteditable selector, improve tests
Oct 7, 2020
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -455,14 +459,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 +483,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 +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 = [
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

"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();
}
Expand Down
81 changes: 81 additions & 0 deletions test/unit/ui/popup.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('<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 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(`
<button disabled>No focus here</button>
<span tabindex="0" data-testid="abc">Test</span>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

`)
.setLngLat([0, 0])
.addTo(createMap(t));

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

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