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

Jump to bottom

Make sure IDs on collapsible navigation are unique #160

Merged
merged 2 commits into from
Nov 15, 2019
Merged

Make sure IDs on collapsible navigation are unique #160

merged 2 commits into from
Nov 15, 2019

Conversation

jakubmiarka
Copy link
Contributor

Multiple nested pages were getting assigned the same ID (from the parent). This change
appends the iterator index to the elements to make sure they're unique.
Also added an extra nested page to the example docs and a test.

@NickColley
Copy link
Contributor

Looks like there's some issues with the format of the JavaScript as StandardJs is throwing errors.

I will check this out but seems sensible so far, thanks :)

NickColley
NickColley approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

I've tested this out locally seems to work as expected :)

Needs a CHANGELOG entry and linting fixes then we can get it merged.

NickColley
NickColley suggested changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Hmm actually this seems to break the relationship between aria-controls on the button and the expanded element. 🤔

@jakubmiarka jakubmiarka force-pushed the fix-unique-ids-on-collapsible-navigation branch from aabbd43 to a9270b8 Compare November 15, 2019 10:23
Make sure IDs on collapsible navigation are unique
c186288 
Multiple nested pages were getting assigned the same ID (from the parent). This change
appends the iterator index to the elements to make sure they're unique.
Also added a extra nested page to the example docs and a test.
@jakubmiarka jakubmiarka force-pushed the fix-unique-ids-on-collapsible-navigation branch from a9270b8 to c186288 Compare November 15, 2019 10:25
@jakubmiarka
Copy link
Contributor Author

Hmm actually this seems to break the relationship between aria-controls on the button and the expanded element. 🤔

@NickColley I have amended the the logic in 41c0956 ... the other option was to wrap the headings in a single element for aria purposes, thought this was easier. Let me know what you think.

@NickColley
Copy link
Contributor

I imagine from a real world point of view having a single expanded area would be easier to use. I suspect having multiple places being controlled may behave in a weird way in screen readers even though it's allowed by the aria specification.

@NickColley
Copy link
Contributor

Maybe worth giving this a test in a screen reader before making a decision? It may work great with multiple IDs.

@jakubmiarka jakubmiarka force-pushed the fix-unique-ids-on-collapsible-navigation branch from 41c0956 to 8d229a4 Compare November 15, 2019 12:49
Fixed aria-controls attribute
511d1ce 
When using multiple nested unique IDs the aria relationship broke. This
change adds the list of the nested elements in the `aria-controls` attribute.
@jakubmiarka jakubmiarka force-pushed the fix-unique-ids-on-collapsible-navigation branch from 8d229a4 to 511d1ce Compare November 15, 2019 12:57
@jakubmiarka
Copy link
Contributor Author

From what I can see, aria-controls is only supported in JAWS and supposedly* it just says "press the JAWS key plus Alt and M to move to the controlled element" when using multiple IDs.
So we can either leave it as it as and be compliant with the spec, or add a wrapper around the expandable area. However, this will be a slightly more work as there are inconsistencies how the tree is getting build using headers vs nested pages.

*I don't have the capacity to be testing this with JAWS right now

@NickColley
Copy link
Contributor

I think with that in mind we should get this in and consider raising an issue to improve it in the future.

@jakubmiarka jakubmiarka merged commit 18c193c into master Nov 15, 2019
@jakubmiarka jakubmiarka deleted the fix-unique-ids-on-collapsible-navigation branch November 15, 2019 14:32
@bravegrape bravegrape mentioned this pull request Nov 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickColley NickColley NickColley approved these changes

@tombye tombye Awaiting requested review from tombye

@bravegrape bravegrape Awaiting requested review from bravegrape

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jakubmiarka @NickColley