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

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

Merged
merged 9 commits into from
Oct 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ee0767a
Fix some failed results from Web map tools WCAG 2.1 evaluation
tristen Sep 18, 2020
1a2e5d1
@Malvoz feedback
tristen Sep 21, 2020
705ae34
Remove mapboxgl-compact-show class when width <= 640
tristen Sep 21, 2020
5eff9d7
Fix CSS lint and flow
tristen Sep 21, 2020
27c6496
Account for top-left bottom-left placement in CSS
tristen Sep 21, 2020
5c7abbd
Add unit test for toggle
tristen Sep 21, 2020
5eb9ff9
Drop window
tristen Sep 21, 2020
67282f4
@ryanhamley feedback
tristen Oct 4, 2020
1380141
@ryanhamley feedback
tristen Oct 5, 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
32 changes: 20 additions & 12 deletions src/css/mapbox-gl.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@
padding: 0;
}

.mapboxgl-ctrl-attrib-button:focus,
.mapboxgl-ctrl-group button:focus {
box-shadow: 0 0 2px 2px rgba(0, 150, 255, 1);
}
Expand Down Expand Up @@ -440,35 +441,35 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
@media screen {
.mapboxgl-ctrl-attrib.mapboxgl-compact {
min-height: 20px;
padding: 0;
padding: 2px 24px 2px 0;
margin: 10px;
position: relative;
background-color: #fff;
border-radius: 3px 12px 12px 3px;
border-radius: 12px;
}

.mapboxgl-ctrl-attrib.mapboxgl-compact:hover {
padding: 2px 24px 2px 4px;
.mapboxgl-ctrl-attrib.mapboxgl-compact-show {
padding: 2px 24px 2px 8px;
visibility: visible;
margin-top: 6px;
}

.mapboxgl-ctrl-top-left > .mapboxgl-ctrl-attrib.mapboxgl-compact:hover,
.mapboxgl-ctrl-bottom-left > .mapboxgl-ctrl-attrib.mapboxgl-compact:hover {
padding: 2px 4px 2px 24px;
border-radius: 12px 3px 3px 12px;
.mapboxgl-ctrl-top-left > .mapboxgl-ctrl-attrib.mapboxgl-compact-show,
.mapboxgl-ctrl-bottom-left > .mapboxgl-ctrl-attrib.mapboxgl-compact-show {
padding: 2px 8px 2px 24px;
border-radius: 12px;
}

.mapboxgl-ctrl-attrib.mapboxgl-compact .mapboxgl-ctrl-attrib-inner {
display: none;
}

.mapboxgl-ctrl-attrib.mapboxgl-compact:hover .mapboxgl-ctrl-attrib-inner {
.mapboxgl-ctrl-attrib.mapboxgl-compact .mapboxgl-ctrl-attrib-button,
.mapboxgl-ctrl-attrib.mapboxgl-compact-show .mapboxgl-ctrl-attrib-inner {
display: block;
}

.mapboxgl-ctrl-attrib.mapboxgl-compact::after {
content: '';
.mapboxgl-ctrl-attrib-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

Screen Shot 2020-10-01 at 11 44 03 AM

I think the compact attribution could use a bit more space between the text and the button. When the button is focused, which it is when the control is first maximized after clicking it or hitting Enter, the blue focus ring abuts the text pretty tightly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Increased padding in 67282f4 to address this!

Screen Shot 2020-10-04 at 1 27 37 PM

display: none;
cursor: pointer;
position: absolute;
background-image: svg-load('svg/mapboxgl-ctrl-attrib.svg');
Expand All @@ -477,6 +478,13 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
height: 24px;
box-sizing: border-box;
border-radius: 12px;
outline: none;
top: 0;
right: 0;
border: 0;
}
.mapboxgl-ctrl-attrib.mapboxgl-compact-show .mapboxgl-ctrl-attrib-button {
background-color: rgba(0, 0, 0, 0.05);
}

.mapboxgl-ctrl-bottom-right > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
Expand Down
31 changes: 27 additions & 4 deletions src/ui/control/attribution_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class AttributionControl {
this.options = options;

bindAll([
'_toggleAttribution',
'_updateEditLink',
'_updateData',
'_updateCompact'
Expand All @@ -53,7 +54,12 @@ class AttributionControl {

this._map = map;
this._container = DOM.create('div', 'mapboxgl-ctrl mapboxgl-ctrl-attrib');
this._compactButton = DOM.create('button', 'mapboxgl-ctrl-attrib-button', this._container);
this._compactButton.addEventListener('click', this._toggleAttribution);
this._setButtonTitle(this._compactButton, 'ToggleAttribution');
this._compactButton.setAttribute('role', 'button');
tristen marked this conversation as resolved.
Show resolved Hide resolved
this._innerContainer = DOM.create('div', 'mapboxgl-ctrl-attrib-inner', this._container);
this._innerContainer.setAttribute('role', 'list');
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment about this feature is Control to display attribution and feedback links is missing name and role.. Should we add a name attribute to this feature as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, name and role for the feedback links themselves. I have a PR in a branch to address that in the TileJSON. The attribution control just receives these as strings and appends them in so it felt right to do it upstream.


if (compact) {
this._container.classList.add('mapboxgl-compact');
Expand Down Expand Up @@ -86,16 +92,32 @@ class AttributionControl {
this._attribHTML = (undefined: any);
}

_setButtonTitle(button: HTMLButtonElement, title: string) {
const str = this._map._getUIString(`AttributionControl.${title}`);
button.title = str;
button.setAttribute('aria-label', str);
}

_toggleAttribution() {
if (this._container.classList.contains('mapboxgl-compact-show')) {
this._container.classList.remove('mapboxgl-compact-show');
this._compactButton.setAttribute('aria-pressed', 'false');
} else {
this._container.classList.add('mapboxgl-compact-show');
this._compactButton.setAttribute('aria-pressed', 'true');
}
}

_updateEditLink() {
let editLink = this._editLink;
if (!editLink) {
editLink = this._editLink = (this._container.querySelector('.mapbox-improve-map'): any);
}

const params = [
{key: "owner", value: this.styleOwner},
{key: "id", value: this.styleId},
{key: "access_token", value: this._map._requestManager._customAccessToken || config.ACCESS_TOKEN}
{key: 'owner', value: this.styleOwner},
{key: 'id', value: this.styleId},
{key: 'access_token', value: this._map._requestManager._customAccessToken || config.ACCESS_TOKEN}
Comment on lines +118 to +120
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a necessary change but I just came across this string inconsistency.

];

if (editLink) {
Expand All @@ -106,7 +128,8 @@ class AttributionControl {
return acc;
}, `?`);
editLink.href = `${config.FEEDBACK_URL}/${paramString}${this._map._hash ? this._map._hash.getHashString(true) : ''}`;
editLink.rel = "noopener nofollow";
editLink.rel = 'noopener nofollow';
this._setButtonTitle(editLink, 'MapFeedback');
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/ui/control/navigation_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,12 @@ class NavigationControl {

_updateZoomButtons() {
const zoom = this._map.getZoom();
this._zoomInButton.disabled = zoom === this._map.getMaxZoom();
this._zoomOutButton.disabled = zoom === this._map.getMinZoom();
const isMax = zoom === this._map.getMaxZoom();
const isMin = zoom === this._map.getMinZoom();
this._zoomInButton.disabled = isMax;
this._zoomOutButton.disabled = isMin;
this._zoomInButton.setAttribute('aria-disabled', isMax ? 'true' : 'false');
tristen marked this conversation as resolved.
Show resolved Hide resolved
this._zoomOutButton.setAttribute('aria-disabled', isMin ? 'true' : 'false');
}

_rotateCompassArrow() {
Expand Down Expand Up @@ -129,6 +133,7 @@ class NavigationControl {
_createButton(className: string, fn: () => mixed) {
const a = DOM.create('button', className, this._container);
a.type = 'button';
a.setAttribute('role', 'button');
tristen marked this conversation as resolved.
Show resolved Hide resolved
a.addEventListener('click', fn);
return a;
}
Expand Down
2 changes: 2 additions & 0 deletions src/ui/default_locale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// @flow

const defaultLocale = {
'AttributionControl.ToggleAttribution': 'Toggle attribution',
'AttributionControl.MapFeedback': 'Map feedback',
'FullscreenControl.Enter': 'Enter fullscreen',
'FullscreenControl.Exit': 'Exit fullscreen',
'GeolocateControl.FindMyLocation': 'Find my location',
Expand Down
1 change: 1 addition & 0 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2289,6 +2289,7 @@ class Map extends Camera {
this._canvas.addEventListener('webglcontextrestored', this._contextRestored, false);
this._canvas.setAttribute('tabindex', '0');
this._canvas.setAttribute('aria-label', 'Map');
this._canvas.setAttribute('role', 'application');
tristen marked this conversation as resolved.
Show resolved Hide resolved

const dimensions = this._containerDimensions();
this._resizeCanvas(dimensions[0], dimensions[1]);
Expand Down