Skip to content

feat: add optional prop to set aria-label on OL canvas element in shadow root #445

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

Merged
jessicamcinchak merged 3 commits into from
Apr 10, 2024
Merged

feat: add optional prop to set aria-label on OL canvas element in shadow root #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cfb66d
add optional prop to set aria-label on ol canvas
jessicamcinchak Apr 8, 2024
f1ccdf7
Merge branch 'main' of github.com:theopensystemslab/map into jess/ari…
jessicamcinchak Apr 9, 2024
b3807a0
update pitsby docs
jessicamcinchak Apr 9, 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
2 changes: 1 addition & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ <h1 style="color:red;font-size:16px;">*** This is a testing sandbox - these comp
***</h1>
<div style="margin-bottom:1em">
<my-map zoom="20" maxZoom="23" id="example-map" drawMode useScaleBarStyle showScale showNorthArrow showPrint
osProxyEndpoint="https://api.editor.planx.dev/proxy/ordnance-survey" />
osProxyEndpoint="https://api.editor.planx.dev/proxy/ordnance-survey" ariaLabelOlFixedOverlay="Interactive example map" />
</div>
<div style="margin-bottom:1em">
<postcode-search hintText="Optional hint text shows up here" id="example-postcode" />
Expand Down
4 changes: 2 additions & 2 deletions src/components/my-map/docs/my-map-basic.doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ module.exports = {
values: "map (default)",
},
{
name: "ariaLabel",
name: "ariaLabelOlFixedOverlay",
type: "String",
values: "An interactive map (default)",
values: "An interactive map",
},
{
name: "disableVectorTiles",
Expand Down
4 changes: 2 additions & 2 deletions src/components/my-map/docs/my-map-proxy.doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ module.exports = {
values: "map (default)",
},
{
name: "ariaLabel",
name: "ariaLabelOlFixedOverlay",
type: "String",
values: "An interactive map (default)",
values: "An interactive map",
},
{
name: "disableVectorTiles",
Expand Down
7 changes: 7 additions & 0 deletions src/components/my-map/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ export class MyMap extends LitElement {
},
};

@property({ type: String })
ariaLabelOlFixedOverlay = "";
Comment thread
jessicamcinchak marked this conversation as resolved.

// set class property (map doesn't require any reactivity using @state)
map?: Map;

Expand Down Expand Up @@ -629,6 +632,10 @@ export class MyMap extends LitElement {
});
}

// Add an aria-label to the overlay canvas for accessibility
const olCanvas = this.renderRoot?.querySelector("canvas.ol-fixedoverlay");
Copy link
Copy Markdown
Member Author

@jessicamcinchak jessicamcinchak Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's two canvas elements rendered by OL in the shadow root, but I've only been able to successfully select this one - which does differ than the code reference given in our report:

Current code ref(s):
#property-information-map > div > div.ol-unselectable.ol-layers > div:nth-child(2) > canvas

So,

  • Am I missing something obvious to be able to select the .ol-unselectable .ol-layers canvas element?
  • If that canvas is actually unselectable, are we still happy applying this proposed fix to this canvas and seeing how it re-tests?

Copy link
Copy Markdown
Contributor

@DafyddLlyr DafyddLlyr Apr 9, 2024

Choose a reason for hiding this comment

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

This is a good point to flag and I think I know why you're getting inconsistent results.

My understanding is that OpenLayers doesn't generate either a single canvas, or canvas per-layer, but rather adds additional canvases as needed for memory/rendering.

I'm trying to find some sort of reference to this online and nothing yet to confirm this in writing!

Looking through OpenLayers examples and inspecting the DOM you can see maps with multiple layers on a single canvas, and some spread over multiple canvases.

What might be best here is trying to select all canvas elements (querySelectorAll() I think) and appending ${ariaLabelOlFixedOverlay}-${i}?

Copy link
Copy Markdown
Member Author

@jessicamcinchak jessicamcinchak Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dev call resolution here:

  • We don't want to label all & overread (even if we could successfully select all)
  • By targeting fixedoverlay, we are targetting the outermost canvas which will hopefully still achieve same result as original suggestion
  • It's a usability issue, not A-level - so let's keep it timeboxed!

olCanvas?.setAttribute("aria-label", this.ariaLabelOlFixedOverlay);

// XXX: force re-render for safari due to it thinking map is 0 height on load
setTimeout(() => {
window.dispatchEvent(new Event("resize"));
Expand Down