-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature index #636
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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. |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9f33d64
to
b53e31a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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! |
Looks good to me. It was decided to have the feature index disabled by default? Things we can deal with after the merge:
|
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. |
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. 😋 |
There was a problem hiding this 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.
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? |
Yep you can just add this script into the head:
|
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 Change these: 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: (These changes may affect #558) |
Re #636 (review):
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. |
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). |
So only investigate features using the number keys when using the feature index? 2022-05-20.12-08-16.mp4 |
Yes I think so, hopefully Robert agrees |
Yes I think that should be fine. 🙂 |
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). |
* 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
Sorry I have limited time to (re-)review, I'm about to go on vacation. I'm seeing the following after opening a dialog: This could be fixed similar to .leaflet-container:not(:focus) .mapml-feature-index-box {
display: none;
} However there'd still be the case where the
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
That's correct (#470): A potentially valid workaround is to move the 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) |
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. |
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? |
OK, looks good, big milestone 🍾 🥳 @ben-lu-uw and @Malvoz congratulations. We will deal with arrow key navigation soon. |
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 |
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. |
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