Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: version selector JS #1305
fix: version selector JS #1305
Changes from 8 commits
459ceda
318be61
5570a4f
f18590a
655a966
b979d2f
5fca8ee
d4b37c6
544f6ba
ca643be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@trallard here the inner div (the thing that pops open when the button is clicked) gets an ID,
role='menu'
and anaria-labelledby
(again I'm copying from MDN example, not certain I've done this optimally)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.
tabindex=-1
: allows DOM focus on being set on the listbox containerlabelledby
aria now since it will inherit from the button aboveThere 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 is another
aria
property needed herearia-activedescendant
which I assume will be related toactiveVersionName
, but I am unsure how to add this (seems to be dynamically set in the JS file)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 actually unsure about this. IIUC,
aria-activedescendant
is about the element that has DOM focus whereasactiveVersionName
is about the version of the docs that is currently loaded in the browser (regardless of which version choice the user happens to have tabbed to when preparing to switch versions). Is that right? If so / if I'm reading the w3c page correctly, we don't want to setaria-activedescendant
equal toactiveVersionName
, and we don't actually need to setaria-activedescendant
at all since we settabindex=-1
(therefore the container is not focusable). Does that sound right to you @trallard or am I misunderstanding?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.
@trallard In a follow-up PR I'd like to add a
@requires_playwright
decorator so that I can move this test out oftest_a11y.py
intotest_build.py
. Something similar tohttps://github.com/mne-tools/mne-python/blob/68e4f89763135637110859ee0e05e594955815f1/mne/utils/_testing.py#L67-L81
OK with you if this non-a11y test squats in the
test_a11y.py
file for now until that gets done? (did it this way because of theimportorskip
issue; didn't want toimportorskip
the entire other tests file when only one test needed playwright)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.
sure, this makes sense to me
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 is just a rename to make the test name more specific, since I'm adding another related test.
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 file is used in our tests to make sure the version switcher component gets embedded correctly (prior to any JS adding the actual entries). Changes here simply mirror the changes to the template so that test still passes.