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.

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

Feature index #636

Merged
merged 31 commits into from
May 27, 2022
Merged

Feature index #636

merged 31 commits into from
May 27, 2022

Conversation

ben-lu-uw
Copy link
Contributor

@ben-lu-uw ben-lu-uw commented Dec 15, 2021

2021-12-15.10-17-58.mov

I tried making the feature index table an aria-live region but it is an earful and the map location does not get announced. Left the table as tabbable for now.

Edit: I guess they could share the output element

@Malvoz

This comment has been minimized.

@ben-lu-uw

This comment has been minimized.

@Malvoz

This comment has been minimized.

@ahmadayubi
Copy link
Member

That looks really good! The naming is somewhat conflicting with #596, it maybe also be able to potentially merge together, instead of having to sort twice.

Checking distance could then be done simply by looping through the sorted inbounds array from the PR as that accounts for the subparts of features as well.

Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

I'm not exactly sure what's in scope for this PR (in relation to #397), but here are a few more thoughts:

  • The list should probably not have more than 9 items at any given time, such that all features can be accessed using the 1-9 number keys (i.e. only 1 mapml-feature-index-content container is needed, where 9 would show more features, 8 would show the previously shown features):

    A11y.Map.-.Google.Chrome.2021-12-16.12-00-12.mp4

    Fixed in 19fbf5d

  • What should happen when a number key is pressed? Probably send focus to the corresponding feature (Esri's a11y-map opens a popup, but we don't expect all features to have a popup)? This would allow the user to discover what type of feature it is (link, button, etc.(?)) before choosing to interact with it.

    Focus on feature when key is pressed fixed in 9f33d64

  • Should Esc - when focus is on a feature - return focus to the leaflet container? (so that the user doesn't have to tab all the way back to be able to pan the map. Esri does this too.)

    Fixed in 86c860c

Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

Resolved by #636 (comment).

Feature index bug?

duplicate-entries

Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

Resolved in afe4f56.

Small visual bug when feature index is empty:

empty-feature-index

@ben-lu-uw
Copy link
Contributor Author

I'm not exactly sure what's in scope for this PR (in relation to #397)

Sorry forgot to mention, this draft PR is just to get feedback on points 1 and 2 in #397 atm.

src/mapml.css Outdated Show resolved Hide resolved
src/mapml.css Outdated Show resolved Hide resolved
src/mapml.css Show resolved Hide resolved
@ben-lu-uw

This comment has been minimized.

src/mapml.css Show resolved Hide resolved
@Malvoz

This comment has been minimized.

@ben-lu-uw

This comment has been minimized.

@prushforth

This comment has been minimized.

@ben-lu-uw

This comment has been minimized.

@Malvoz

This comment has been minimized.

src/mapml.css Outdated Show resolved Hide resolved
@prushforth prushforth marked this pull request as ready for review May 19, 2022 18:48
@prushforth
Copy link
Member

Hey @Malvoz I don't know if you've got any time these days, but I think this PR could use a review by yourself. Thanks!

@prushforth prushforth requested a review from Malvoz May 19, 2022 18:48
@Malvoz
Copy link
Member

Malvoz commented May 19, 2022

Looks good to me. It was decided to have the feature index disabled by default?

Things we can deal with after the merge:

  • I think there is room for some small CSS improvements

  • NVDA doesn't announce the features that are initially in view on the first tab press to focus the map.

    I tested a bunch of different solutions last year and found that it could be improved with some modifications to aria-label + role="region" on leaflet-container. Once that issue was fixed the result was (in Chrome, not Firefox) that the features were announced twice, so I filed: https://bugs.chromium.org/p/chromium/issues/detail?id=1285603

@prushforth
Copy link
Member

prushforth commented May 19, 2022

Yes, we put access to the feature index into the user preferences (extension). We plan to implement feature navigation with arrow keys as the default, but user configurable. We left the default at tabbing through features for now. Is that appropriate? I realize it doesn't showcase feature indexing. I believe we could make an experiments page that is hard-coded to use feature index even without the extension installed.

Edit: have you seen Goodmaps? I think it gives us an idea of what non-visual mobile users might find useful.

@Malvoz
Copy link
Member

Malvoz commented May 19, 2022

Anything that relies on the extension is by default inaccessible, so that's a consideration. Esri's a11y-map has a map control to toggle their feature index on/off. But we could indeed start with an experiments page.

I might've stumbled upon Goodmaps before, I usually leave quickly when I don't find a live demo of a map. 😋

Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

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

Thanks , looking much better with your popup fix. I think it works better, visually at least. I had a listen on NVDA, and I think if we could make it read out the entire feature index, that would be a better experience (see Robert's comment, maybe that is relevant). The user could just listen and hit a key when something piqued their interest. Another issue that I noticed was that I think the feature index should replace all the features in the tab order i.e. when you hit tab with the focus on the map/feature index, it should skip to the first control in tab order. Note that controls can be "pruned" individually (i.e. see the controlslist attribute), so the code will have to take that into account i.e. not hardcode which control gets the focus after the feature index. Finally, is there a way to "hard code" a page to use the feature index, if the user doesn't have the extension installed and set to select that option? We should make a copy of the restaurants page in our experiments repo and make a link to it from the index.html page in that repo.

@prushforth
Copy link
Member

@Malvoz

Anything that relies on the extension is by default inaccessible, so that's a consideration.

I understand. The intention is to have an accessible map experience by default. I think the feature index will work well for some users, and tab/arrow keys for others. Both experiences should be accessible, but I think the user should be able to set a preference for one or the other from the browser settings (in our case we are using our extension as a proxy for browser settings). The a11ymap control is another possibility. Would you want to allow a web site to turn it off, though?

@ben-lu-uw
Copy link
Contributor Author

Finally, is there a way to "hard code" a page to use the feature index, if the user doesn't have the extension installed and set to select that option?

Yep you can just add this script into the head:

<script>
    let options = document.createElement("map-options");
    options.innerHTML = '{"featureIndexOverlayOption":true}';
    document.head.appendChild(options);
</script>

@Malvoz
Copy link
Member

Malvoz commented May 20, 2022

  • NVDA doesn't announce the features that are initially in view on the first tab press to focus the map.

Last year I tried a bunch of stuff, and concluded the following to be the most reliable to have the features announced on the initial tab to the leaflet-container:

Change these:

https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/2fcac6022f66003d9e5a27899904f8e26cd8fcf8/src/mapml-viewer.js#L227-L228

https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/2fcac6022f66003d9e5a27899904f8e26cd8fcf8/src/web-map.js#L266-L267

to either:

this._container.setAttribute('role', 'application'); 
this._container.setAttribute('aria-roledescription', 'Interactive map'); 
this._container.setAttribute('aria-labelledby', 'mapml-feature-index');  // And set this ID on the <output>

or

this._container.setAttribute('role', 'application'); 
this._container.setAttribute('aria-label', 'Interactive map'); 
this._container.setAttribute('aria-describedby', 'mapml-feature-index');  // And set this ID on the <output>

and remove these lines:

https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/2fcac6022f66003d9e5a27899904f8e26cd8fcf8/src/mapml-viewer.js#L224

https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/2fcac6022f66003d9e5a27899904f8e26cd8fcf8/src/web-map.js#L263

(These changes may affect #558)

@Malvoz
Copy link
Member

Malvoz commented May 20, 2022

Re #636 (review):

Another issue that I noticed was that I think the feature index should replace all the features in the tab order i.e. when you hit tab with the focus on the map/feature index, it should skip to the first control in tab order.

If I understand correctly, what you're suggesting is "Changing DOM order" in Leaflet/Leaflet#7479, whereas I think arrow keys for feature navigation (#535) may be a more desirable solution.

@prushforth
Copy link
Member

prushforth commented May 20, 2022

If I understand correctly, what you're suggesting is "Leaflet/Leaflet#7479 (comment)" in Leaflet/Leaflet#7479, whereas I think Leaflet/Leaflet#7479 (comment) (#535) may be a more desirable solution.

No, I think option C: Arrow keys is desirable, and I want us to implement it. But I think it should be EITHER feature index OR a tab stop + arrow keys for features (right now it is feature index then as many tabs as it takes to get through all features), although if we are able to implement arrow keys it would only be one tab stop, so perhaps not that big a deal.

So in the short term I think we should replace all the features' tab stops with the feature index, if that's doable (edit only when the feature index is enabled).

@ben-lu-uw
Copy link
Contributor Author

So in the short term I think we should replace all the features' tab stops with the feature index, if that's doable (edit only when the feature index is enabled).

So only investigate features using the number keys when using the feature index?
And then tab focuses the map and tabbing again would go to controls?

2022-05-20.12-08-16.mp4

@prushforth
Copy link
Member

Yes I think so, hopefully Robert agrees

@Malvoz
Copy link
Member

Malvoz commented May 20, 2022

Yes I think that should be fine. 🙂

@prushforth
Copy link
Member

So I think this is pretty good right now. The only issue that Ben and I discussed was that when NVDA is running, and you select a feature by index number, you have to dismiss the popup in order to select another feature, whereas if NVDA is not running, you can select another feature from the index while the popup for the first feature is displayed. We believe this has to do with NVDA switching to browse mode when the popup is read, re-mapping the index number keys to heading level shortcuts, something we feel we should not attempt to change (the popup could have headings and so on, who can say).
Ben will push the changes shortly.

* Only start checking overlap when/after the first focus happens

* Announce map details and then feature index on initial focus

* Announce map details and then feature index on refocus

* Focus map directly when popup is closed and feature index option is on

* Remove reticle when the map is not focused

* Add hidden comma for brief pauses in output reading

* Announce feature index on popupclose

* Update featureIndexOverlay.test.js
@Malvoz
Copy link
Member

Malvoz commented May 27, 2022

Sorry I have limited time to (re-)review, I'm about to go on vacation. I'm seeing the following after opening a dialog:

feature-index-box-overlapping

This could be fixed similar to mapml-outline for example:

.leaflet-container:not(:focus) .mapml-feature-index-box {
  display: none;
}

However there'd still be the case where the mapml-feature-index-box's visibility flashes on mouse click on the map.


mapml-feature-index is now both mapml-screen-reader-output (screen reader accessible but visibly hidden) and aria-hidden="true" if the map isn't focused. Should it just toggle the hidden attribute instead then?

OTOH, my intention was to always have the features accessible whether or not the feature index was visibly hidden or not. Though obviously some screen readers don't like to reread things even after a user focuses an element, thus we had the issue of features not being announced after the map was tabbed to. Toggling aria-hidden helps with that, but now features aren't announced on e.g. zoom from the +/- controls, etc.?


The only issue that Ben and I discussed was that when NVDA is running, and you select a feature by index number, you have to dismiss the popup in order to select another feature, whereas if NVDA is not running, you can select another feature from the index while the popup for the first feature is displayed. We believe this has to do with NVDA switching to browse mode when the popup is read

That's correct (#470):

https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/b108e33535b9aad61be269a0c97db2460d28e155/src/mapml/layers/MapLayer.js#L1246

A potentially valid workaround is to move the document role to a new wrapper element for the content in mapml-popup-content.

Unfortunately this will be the responsibility of all authors after #403. 😕

Edit: the role fixed an issue with popups not being announced on focus, so it probably can't be moved.


Longing for the day we'll be doing: 1) <map(viewer)> 2) Done. 😏

@ben-lu-uw
Copy link
Contributor Author

Should it just toggle the hidden attribute instead then?

I chose to to toggle the aria-hidden attribute since toggling hidden would sometimes make it visually hidden in cases where it shouldn't be, like when opening popups since at that time the map is not focused.

@prushforth
Copy link
Member

now features aren't announced on e.g. zoom from the +/- controls

Features are announced when the map is focused, and if you use the +/- keys with the map focused, features are announced. Is that acceptable? I'm not sure if it would be a good idea to announce features when the control is focused anyway?

@prushforth
Copy link
Member

prushforth commented May 27, 2022

OK, looks good, big milestone 🍾 🥳 @ben-lu-uw and @Malvoz congratulations. We will deal with arrow key navigation soon.

@prushforth prushforth merged commit 62c27f2 into Maps4HTML:main May 27, 2022
@Malvoz
Copy link
Member

Malvoz commented May 27, 2022

now features aren't announced on e.g. zoom from the +/- controls

Features are announced when the map is focused, and if you use the +/- keys with the map focused, features are announced. Is that acceptable? I'm not sure if it would be a good idea to announce features when the control is focused anyway?

We expect visual feedback when we click the zoom controls, likewise AT users would expect auditory feedback.

Users on touch devices (using e.g. TalkBack, VoiceOver) can't use the feature index in the demo because it remains aria-hidden="true".

@Malvoz Malvoz mentioned this pull request May 27, 2022
@prushforth
Copy link
Member

We expect visual feedback when we click the zoom controls, likewise AT users would expect auditory feedback.

Agreed. I think we should make it so that zoom level changes are always announced, at initial load and whenever it changes, not just with the announce movement option enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants