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 all commits
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
44 changes: 29 additions & 15 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,30 @@ 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 28px 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 28px;
border-radius: 12px;
}

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

.mapboxgl-ctrl-attrib.mapboxgl-compact:hover .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 +473,24 @@ 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-top-left .mapboxgl-ctrl-attrib-button,
.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-attrib-button {
left: 0;
}

.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-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
33 changes: 28 additions & 5 deletions src/ui/control/attribution_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class AttributionControl {
_map: Map;
_container: HTMLElement;
_innerContainer: HTMLElement;
_compactButton: HTMLButtonElement;
_editLink: ?HTMLAnchorElement;
_attribHTML: string;
styleId: string;
Expand All @@ -38,6 +39,7 @@ class AttributionControl {
this.options = options;

bindAll([
'_toggleAttribution',
'_updateEditLink',
'_updateData',
'_updateCompact'
Expand All @@ -53,7 +55,11 @@ 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._setElementTitle(this._compactButton, 'ToggleAttribution');
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);
}

_setElementTitle(element: HTMLElement, title: string) {
const str = this._map._getUIString(`AttributionControl.${title}`);
element.title = str;
element.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._setElementTitle(editLink, 'MapFeedback');
}
}

Expand Down Expand Up @@ -180,7 +203,7 @@ class AttributionControl {
if (this._map.getCanvasContainer().offsetWidth <= 640) {
this._container.classList.add('mapboxgl-compact');
} else {
this._container.classList.remove('mapboxgl-compact');
this._container.classList.remove('mapboxgl-compact', 'mapboxgl-compact-show');
}
}

Expand Down
8 changes: 6 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.toString());
this._zoomOutButton.setAttribute('aria-disabled', isMin.toString());
}

_rotateCompassArrow() {
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', 'region');

const dimensions = this._containerDimensions();
this._resizeCanvas(dimensions[0], dimensions[1]);
Expand Down
23 changes: 23 additions & 0 deletions test/unit/ui/control/attribution.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {test} from '../../../util/test';
import config from '../../../../src/util/config';
import AttributionControl from '../../../../src/ui/control/attribution_control';
import {createMap as globalCreateMap} from '../../../util';
import simulate from '../../../util/simulate_interaction';

function createMap(t) {
config.ACCESS_TOKEN = 'pk.123';
Expand Down Expand Up @@ -75,6 +76,28 @@ test('AttributionControl appears in compact mode if container is less then 640 p
t.end();
});

test('AttributionControl compact mode control toggles attribution', (t) => {
const map = createMap(t);
map.addControl(new AttributionControl({
compact: true
}));

const container = map.getContainer();
const toggle = container.querySelector('.mapboxgl-ctrl-attrib-button');

t.equal(container.querySelectorAll('.mapboxgl-compact-show').length, 0);

simulate.click(toggle);

t.equal(container.querySelectorAll('.mapboxgl-compact-show').length, 1);

simulate.click(toggle);

t.equal(container.querySelectorAll('.mapboxgl-compact-show').length, 0);

t.end();
});

test('AttributionControl dedupes attributions that are substrings of others', (t) => {
const map = createMap(t);
const attribution = new AttributionControl();
Expand Down