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: usability improvements #442

Merged
merged 5 commits into from
Apr 5, 2024
Merged

fix: usability improvements #442

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
142f587
focus styles
jessicamcinchak Apr 4, 2024
f13e82d
set tabindex for controls and attribution links
jessicamcinchak Apr 4, 2024
29bc5bf
update tests, account for collapsed attributions
jessicamcinchak Apr 4, 2024
96c83c3
use scss variables
jessicamcinchak Apr 4, 2024
2e5244d
reorder overlay dom elements instead of setting positive tabindex
jessicamcinchak Apr 5, 2024
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
1 change: 1 addition & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ <h1 style="color:red;font-size:16px;">*** This is a testing sandbox - these comp
<script>
// --- MAP --- //
const map = document.querySelector("my-map");
map.geojsonDataCopyright = `<a href="https://www.planning.data.gov.uk/dataset/title-boundary" target="_blank">Title boundary</a> subject to Crown copyright and database rights ${new Date().getFullYear()} OS (0)100026316`;
map.clipGeojsonData = {
"type": "Feature",
"geometry": {
Expand Down
55 changes: 38 additions & 17 deletions src/components/my-map/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,22 +309,6 @@ export class MyMap extends LitElement {
);
const modify = configureModify(this.drawPointer, this.drawColor);

// add custom scale line and north arrow controls to the map
let scale: ScaleLine;
if (this.showNorthArrow) {
map.addControl(northArrowControl());
}

if (this.showScale) {
scale = scaleControl(this.useScaleBarStyle);
map.addControl(scale);
}

if (this.showPrint) {
const printControl = new PrintControl({ map });
map.addControl(printControl);
}

jessicamcinchak marked this conversation as resolved.
Show resolved Hide resolved
// add a custom 'reset' control to the map
const handleReset = () => {
if (this.showFeaturesAtPoint) {
Expand Down Expand Up @@ -357,13 +341,46 @@ export class MyMap extends LitElement {
map.addControl(resetControl(handleReset, this.resetControlImage));
}

// add custom scale line and north arrow controls to the map
let scale: ScaleLine;
if (this.showNorthArrow) {
map.addControl(northArrowControl());
}

if (this.showScale) {
scale = scaleControl(this.useScaleBarStyle);
map.addControl(scale);
}

if (this.showPrint) {
const printControl = new PrintControl({ map });
map.addControl(printControl);
}

// Apply aria-labels to OL Controls for accessibility
const olControls: NodeListOf<HTMLButtonElement> | undefined =
this.renderRoot?.querySelectorAll(".ol-control button");
olControls?.forEach((node) =>
node.setAttribute("aria-label", node.getAttribute("title") || ""),
);

// Apply tabindexes to OL Controls & Attribution links for accessibility (container div has starting tabindex="1")
const controlsLength = olControls?.length;
olControls?.forEach((node, i) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the order of these in the DOM is determined by their order in the collection of map controls.

We set up defaults, then based on props, append new ones. This is why the variable ones come after the defaults (e.g. attribution).

Totally fair if you've already explored this one, but it might be worth trying to just manage this via the order they're added to the map controls? Main motivations here would be simplicity (arguable depending on how that works out!), and also the general recommendation to to not use tabindex unless totally required -

Warning: You are recommended to only use 0 and -1 as tabindex values. Avoid using tabindex values greater than 0 and CSS properties that can change the order of focusable HTML elements (Ordering flex items). Doing so makes it difficult for people who rely on using keyboard for navigation or assistive technology to navigate and operate page content. Instead, write the document with the elements in a logical sequence.

Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

Copy link
Member Author

Choose a reason for hiding this comment

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

This is sorted now by re-ordering, no longer having to rely on positive tab indexes!

    // Re-order overlay elements so that OL Attribution is final element
    //   making OL Controls first in natural tab order for accessibility
    const olAttribution = this.renderRoot?.querySelector(".ol-attribution") as Node;
    const olOverlay = this.renderRoot?.querySelector(".ol-overlaycontainer-stopevent");
    olOverlay?.append(olAttribution);

Copy link
Contributor

Choose a reason for hiding this comment

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

Great solution - super simple 👍

// if `collapseAttributions` is set, ensure attributions control button is last, else follow top-down order
node.title === "Attributions"
? (node.tabIndex = 2 + controlsLength)
: (node.tabIndex = i + 2),
);

if (!this.collapseAttributions) {
const olAttributionLinks: NodeListOf<HTMLAnchorElement> | undefined =
this.renderRoot?.querySelectorAll(".ol-attribution a");
olAttributionLinks?.forEach(
(node, i) => (node.tabIndex = i + 2 + controlsLength),
);
}

// define cursors for dragging/panning and moving
map.on("pointerdrag", () => {
map.getViewport().style.cursor = "grabbing";
Expand Down Expand Up @@ -631,7 +648,11 @@ export class MyMap extends LitElement {
render() {
return html`<script src="https://cdn.polyfill.io/v2/polyfill.min.js"></script>
<link rel="stylesheet" href="https://cdn.skypack.dev/ol@^6.6.1/ol.css" />
<div id="${this.id}" class="map" tabindex="0" />`;
<div
id="${this.id}"
class="map"
tabindex="${this.staticMode && !this.collapseAttributions ? -1 : 1}"
/>`;
}

// unmount the map
Expand Down
27 changes: 23 additions & 4 deletions src/components/my-map/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ test("olMap is added to the global window for tests", async () => {
describe("MyMap on initial render with OSM basemap", async () => {
beforeEach(
() => setupMap('<my-map id="map-vitest" disableVectorTiles />'),
2500
2500,
);

it("should render a custom element with a shadow root", () => {
Expand All @@ -33,11 +33,30 @@ describe("MyMap on initial render with OSM basemap", async () => {
const mapShadowRoot = getShadowRoot("my-map");
expect(mapShadowRoot).toBeTruthy;
});
});

describe("Keyboard navigation of map container, controls and attribution links", () => {
it("map container should be keyboard navigable by default", async () => {
await setupMap(`<my-map id="map-vitest" disableVectorTiles />`);
const map = getShadowRoot("my-map")?.getElementById("map-vitest");
expect(map).toBeTruthy;
expect(map?.getAttribute("tabindex")).toEqual("1");
});

it("should omit map container from tab order if not interactive", async () => {
await setupMap(`<my-map id="map-vitest" disableVectorTiles staticMode />`);
const map = getShadowRoot("my-map")?.getElementById("map-vitest");
expect(map).toBeTruthy;
expect(map?.getAttribute("tabindex")).toEqual("-1");
});

it("should be keyboard navigable", () => {
it("should keep map container in tab order if attributions are collapsed", async () => {
await setupMap(
`<my-map id="map-vitest" disableVectorTiles staticMode collapseAttributions />`,
);
const map = getShadowRoot("my-map")?.getElementById("map-vitest");
expect(map).toBeTruthy;
expect(map?.getAttribute("tabindex")).toEqual("0");
expect(map?.getAttribute("tabindex")).toEqual("1");
});
});

Expand All @@ -47,7 +66,7 @@ describe("Snap points loading behaviour", () => {

const getSnapSpy: MockInstance = vi.spyOn(
snapping,
"getSnapPointsFromVectorTiles"
"getSnapPointsFromVectorTiles",
);
afterEach(() => {
vi.resetAllMocks();
Expand Down
25 changes: 21 additions & 4 deletions src/components/my-map/styles.scss
View file
Open in desktop
jessicamcinchak marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
$gov-uk-yellow: #ffdd00;
$planx-blue: #0010a4;
$planx-dark-grey: #2c2c2c;

// default map size, can be overwritten with CSS
:host {
display: block;
Expand All @@ -15,12 +19,12 @@
}

.map:focus {
outline: #ffdd00 solid 0.25em;
outline: $gov-uk-yellow solid 0.25em;
}

.ol-control button {
border-radius: 0 !important;
background-color: #2c2c2c !important;
background-color: $planx-dark-grey !important;
cursor: pointer;
min-width: 44px;
min-height: 44px;
Expand All @@ -31,14 +35,18 @@
background-color: rgba(44, 44, 44, 0.85) !important;
}

.ol-control button:focus {
outline: $gov-uk-yellow solid 0.15em;
}

.ol-scale-line {
background-color: transparent;
}

.ol-scale-line-inner {
border: 0.2em solid #2c2c2c;
border: 0.2em solid $planx-dark-grey;
border-top: none;
color: #2c2c2c;
color: $planx-dark-grey;
font-size: 1em;
font-family: inherit;
}
Expand Down Expand Up @@ -98,3 +106,12 @@
.ol-attribution li {
display: list-item;
}

.ol-attribution a {
color: $planx-blue;
}

.ol-attribution a:focus {
color: black;
background-color: $gov-uk-yellow;
}