- Notifications
You must be signed in to change notification settings - Fork413
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Announce to screen reader when markers are added to the map#12707
Announce to screen reader when markers are added to the map#12707
Conversation
In order to avoid route definition errors, this commit implementsa basic map server as a rack middleware that serves all the maprelated routes in a secific local map domain.
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've just checked it out in BrowserStack with NVDA and it works as expected
One quick check regarding this fix, are you referring to these kind of warnings from " [CI] Meetings - System tests", right? |
Yes, I believe so. Testing with the development app using the following configuration at # ...config.maps={provider::osm}# ... I see the following in the browser's developer console when loading a page with map (e.g.
When changing the initializer configuration to the following, the error disappears and the map loads correctly (please note the tile usage policy if using this config): # ...config.maps={provider::osm,dynamic:{tile_layer:{url:"https://tile.openstreetmap.org/{z}/{x}/{y}.png"}}}config.content_security_policies_extra={"img-src"=>%w(https://tile.openstreetmap.org)}# ... |
Great, thanks for the outstanding explanation and the PR! |
* Announce to screen reader when markers are added to the map* Fix spelling* Fix linting issue* Do not announce empty message* Implement more robust testing map serverIn order to avoid route definition errors, this commit implementsa basic map server as a rack middleware that serves all the maprelated routes in a secific local map domain.* Fix error in the comment of the middleware* Fix spelling mistakes* Fix broken specs due to test env configuration change
🎩 What? Why?
It was suggested in an accessibility audit to announce to the screen reader when markers are added to the map.
This adds a new utility that allows announcing messages to screen reader dynamically and applies this utility to the proposal edit form map.
After seeing the CI failures, this PR also implements a basic test environment "map server" running as a rack middleware in order to avoid modifying the application routes for every map spec. The map tiles were not configured correctly for the test environment which caused JS errors to be shown on pages that display maps. After configuring the tile URLs for all specs, I started to see several different errors in the system specs that display maps. Therefore, I decided to create a new middleware that handles all the map related functionality in the test env. This particular PR requires the map to be shown on the page which requires the tile URLs to be configured correctly. If the URL is
nullas so far, Leaflet will show errors and the map display will fail which causes the spec added in this PR to work incorrectly (as the message never appears in the view, cannot be tested otherwise).At the same time I noticed there were also errors printed out in the console on pages that display static maps. This was because Webmock disabled these requests and therefore they errored out with HTTP 500.
Testing