fix: version selector JS #1305

jpfeuffer · 2023-04-24T18:39:40Z

jpfeuffer · 2023-04-24T18:43:22Z

supersedes and closes #1259

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

12rambau

first, nice catch, I think it was already identified by @drammock at some point and then long forgotten (here). Could you apply the format pre-commit to allow the tests to run ?

jpfeuffer · 2023-04-25T14:02:57Z

thanks. test failures seem unrelated though (it cannot even set up the environment)

drammock · 2023-04-25T15:41:19Z

thanks. test failures seem unrelated though (it cannot even set up the environment)

It appeared to be a network error, I restarted the CIs and they worked this time. Remaining CI errors are unrelated and being tracked in #1307 now.

I'd like to add a test that checks that styling the button works and also that highlighting the currently-being-viewed version also works. @jpfeuffer are you up for adding such a test? If not I can try to get to it later this week / early next week.

drammock · 2023-05-12T22:33:20Z

@jpfeuffer heads up that I force-pushed to this branch. I did so because a rebase was necessary to get changes from #1320, which were needed for the new test I added to pass.

@12rambau can you take another look here, it's still showing you as "changes requested" but I think that was just about the linting?

@trallard I wouldn't mind your eyes on this PR too, because in order to select the version menu elements with playwright I had to add ARIA roles to them, and I want to make sure I did it correctly. I'll self-review to point you to the relevant changes.

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html

drammock · 2023-05-12T22:36:52Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html

      {{ theme_switcher.get('version_match') }}  <!-- this text may get changed later by javascript -->
      <span class="caret"></span>
    </button>
-    <div class="version-switcher__menu dropdown-menu list-group-flush py-0">
+    <div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">


@trallard here the inner div (the thing that pops open when the button is clicked) gets an ID, role='menu' and an aria-labelledby (again I'm copying from MDN example, not certain I've done this optimally)

tabindex=-1: allows DOM focus on being set on the listbox container

no need for a labelledby aria now since it will inherit from the button above

Suggested change

<div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">

<div id="versionswitcherlist" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="listbox" tabindex=-1>

There is another aria property needed here aria-activedescendant which I assume will be related to activeVersionName, but I am unsure how to add this (seems to be dynamically set in the JS file)

There is another aria property needed here aria-activedescendant which I assume will be related to activeVersionName, but I am unsure how to add this (seems to be dynamically set in the JS file)

I'm actually unsure about this. IIUC, aria-activedescendant is about the element that has DOM focus whereas activeVersionName 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 set aria-activedescendant equal to activeVersionName, and we don't actually need to set aria-activedescendant at all since we set tabindex=-1 (therefore the container is not focusable). Does that sound right to you @trallard or am I misunderstanding?

drammock · 2023-05-12T22:38:35Z

tests/test_build/navbar_switcher.html

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.

drammock · 2023-05-12T22:39:04Z

tests/test_build.py



 @pytest.mark.parametrize("url", switcher_files)
-def test_version_switcher(sphinx_build_factory, file_regression, url) -> None:
+def test_version_switcher_error_states(


this is just a rename to make the test name more specific, since I'm adding another related test.

drammock · 2023-05-12T22:49:11Z

tests/test_a11y.py

+
+
+def test_version_switcher_highlighting(page: Page, url_base: str) -> None:
+    """This isn't an a11y test, but needs a served site for Javascript to inject the version menu."""


@trallard In a follow-up PR I'd like to add a @requires_playwright decorator so that I can move this test out of test_a11y.py into test_build.py. Something similar to

https://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 the importorskip issue; didn't want to importorskip the entire other tests file when only one test needed playwright)

sure, this makes sense to me

trallard · 2023-05-15T17:25:29Z

Thanks for the ping @drammock - I need to review my notes on accessible selectors and the correct ARIA use but at a quick glance, this looks like it would be best to use a <select> or more specifically a combobox widget. Or a menu button.
I shall have time tomorrow morning to do an in-depth review + suggestions

Ref:

Select your poison by Sarah Highley

12rambau · 2023-05-16T13:39:51Z

@drammock sorry for not answering I have a thumb tendonitis, the doctors are trying to keep me away from keyboards.

That's effectciveley a legacy request I've removed it

12rambau

I'll leave it to the maintainers very capable hands

trallard

Need to stop saying "tomorrow" as I keep losing control over my schedule.
Anyway, did a review and made some suggestions + added the rationale on the changes needed

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html

trallard · 2023-05-19T10:42:23Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html

      {{ theme_switcher.get('version_match') }}  <!-- this text may get changed later by javascript -->
      <span class="caret"></span>
    </button>
-    <div class="version-switcher__menu dropdown-menu list-group-flush py-0">
+    <div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">


tabindex=-1: allows DOM focus on being set on the listbox container

no need for a labelledby aria now since it will inherit from the button above

Suggested change

<div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">

<div id="versionswitcherlist" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="listbox" tabindex=-1>

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

tests/test_build/navbar_switcher.html

trallard · 2023-05-19T11:18:51Z

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html

      {{ theme_switcher.get('version_match') }}  <!-- this text may get changed later by javascript -->
      <span class="caret"></span>
    </button>
-    <div class="version-switcher__menu dropdown-menu list-group-flush py-0">
+    <div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">


There is another aria property needed here aria-activedescendant which I assume will be related to activeVersionName, but I am unsure how to add this (seems to be dynamically set in the JS file)

trallard · 2023-05-19T11:20:04Z

tests/test_a11y.py

+
+
+def test_version_switcher_highlighting(page: Page, url_base: str) -> None:
+    """This isn't an a11y test, but needs a served site for Javascript to inject the version menu."""


sure, this makes sense to me

trallard · 2023-05-19T13:08:17Z

Also, I'd like to point out that the version selector component is not currently keyboard focusable and operable. I tried fixing / testing by setting tabindex="0" on the parent <div> with no success, so I will need to spend more time debugging/fixing this behaviour.

The desired behaviours would be as follows:

Collapsed select - menu button

TAB Focuses the selector button
Space / Enter opens the list and moves focus to the current option
Down and Up arrows expand the list

Expanded list

Up and Down arrows move through options, changing the highlighted option (do not change selection).
Space does nothing
Enter closes the menu, keeping the currently highlighted option selected
Escape closes the menu, keeping the currently highlighted option selected, and sets focus to the button

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

jpfeuffer changed the title ~~fixes #1304~~ fix: version selector JS Apr 24, 2023

jpfeuffer commented Apr 24, 2023

View reviewed changes

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js Outdated Show resolved Hide resolved

12rambau requested changes Apr 25, 2023

View reviewed changes

drammock mentioned this pull request May 11, 2023

BUG: Mobile version switcher drop-down broken on 0.13.3 #1322

Closed

jpfeuffer and others added 8 commits May 12, 2023 17:05

fixes pydata#1304

459ceda

rearrange to be able to clone nodes

318be61

Lint pydata-sphinx-theme.js

5570a4f

lint

f18590a

add aria roles to version switcher elements

655a966

fix (?) setting data-active-version-name

b979d2f

rename existing test (more specific name)

5fca8ee

add test of switcher active version highlighting

d4b37c6

drammock force-pushed the patch-1 branch from 115aaec to d4b37c6 Compare May 12, 2023 22:28

drammock reviewed May 12, 2023

View reviewed changes

12rambau reviewed May 16, 2023

View reviewed changes

12rambau approved these changes May 16, 2023

View reviewed changes

trallard requested changes May 19, 2023

View reviewed changes

drammock and others added 2 commits May 26, 2023 13:07

Apply suggestions from code review

544f6ba

Co-authored-by: Tania Allard <taniar.allard@gmail.com>

update test target

ca643be

12rambau mentioned this pull request Jun 8, 2023

fix highlighting of active version in version dropdown #1259

Closed

drammock mentioned this pull request Jun 8, 2023

Several issues with version switcher #1345

Closed

This was referenced Jun 20, 2023

BUG: fix highlighting of current version in switcher menu #1357

Merged

version switcher dropdown not accessible #1358

Closed

drammock closed this in #1357 Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: version selector JS #1305

fix: version selector JS #1305

jpfeuffer commented Apr 24, 2023 •

edited

jpfeuffer commented Apr 24, 2023

12rambau left a comment •

edited by drammock

jpfeuffer commented Apr 25, 2023

drammock commented Apr 25, 2023

drammock commented May 12, 2023

drammock May 12, 2023

trallard May 19, 2023

trallard May 19, 2023

drammock May 26, 2023

drammock May 12, 2023

drammock May 12, 2023

drammock May 12, 2023

trallard May 19, 2023

trallard commented May 15, 2023 •

edited

12rambau commented May 16, 2023

12rambau left a comment •

edited

trallard left a comment

trallard May 19, 2023

trallard May 19, 2023

trallard May 19, 2023

trallard commented May 19, 2023

	<div id="versionswitchermenu" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="menu" aria-labelledby="versionswitcherbutton">
	<div id="versionswitcherlist" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="listbox" tabindex=-1>



		def test_version_switcher_highlighting(page: Page, url_base: str) -> None:
		"""This isn't an a11y test, but needs a served site for Javascript to inject the version menu."""

fix: version selector JS #1305

fix: version selector JS #1305

Conversation

jpfeuffer commented Apr 24, 2023 • edited

jpfeuffer commented Apr 24, 2023

12rambau left a comment • edited by drammock

Choose a reason for hiding this comment

jpfeuffer commented Apr 25, 2023

drammock commented Apr 25, 2023

drammock commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trallard commented May 15, 2023 • edited

12rambau commented May 16, 2023

12rambau left a comment • edited

Choose a reason for hiding this comment

trallard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trallard commented May 19, 2023

jpfeuffer commented Apr 24, 2023 •

edited

12rambau left a comment •

edited by drammock

trallard commented May 15, 2023 •

edited

12rambau left a comment •

edited