Fix duplicate HTML IDs

[gabalafou]

Both the version switcher and more dropdown Jinja templates need to use HTML IDs in order to properly implement ARIA. But these templates can be included in multiple places on a page. In our own docs, they are included both in the header nav and in the left sidebar (only visible in the left sidebar on mobile). However, whether on mobile or desktop, they appear twice in the DOM; they're visibility is toggled with CSS @media rules.

HTML IDs must be unique on the page, or else the page fails basic accessibility checks.

This PR assures that no matter how many times the switcher or dropdown templates are rendered on the same page, each instance will have a unique HTML ID.

[gabalafou]

Note to reviewer: I am primarily a JavaScript, not Python, developer. Please feel free to correct anything that's not sufficiently Pythonic

[gabalafou]

Converting to draft while I fix the tests

[gabalafou]

Hint for reviewer. You can open this PR's Read The Docs preview and run the following command in the browser console to easily get to the DOM elements affected by this PR (plus some others):

document.querySelectorAll('[id^=pst-]')

[12rambau]

Isn't it a problem in all components that use IDs ?

[gabalafou]

Yes, I think you're right.

It looks to me that the Jinja templates under components/ that have an id are:

• search-field
• searchbox
• sidebar-ethical-ads
• version-switcher

I could add calls to create_next_id in those places.

What about other templates like layout.html, search.html or sidebar-primary.html?

[12rambau]

the other templates cannot be called in 2 places right so no problem ?

[gabalafou]

Are you sure? Can a consumer of PyData Sphinx Theme configure it so that the search box appears in the desktop header bar and in the mobile hamburger menu? This is currently what our docs site does with the version switcher.

[gabalafou]

Unless by "the other templates" you were referring specifically to the three I asked about at the end of my earlier comment: layout.html, search.html or sidebar-primary.html

[drammock]

Can a consumer of PyData Sphinx Theme configure it so that the search box appears in the desktop header bar and in the mobile hamburger menu?

layout.html, search.html, and sidebar-primary.html definitely cannot be added twice to the page. They are not "components" --- layout and search are "templates" and sidebar is a "section". In principle, anything in src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components could be added twice. That includes searchbox.html; however, we explicitly discourage users from adding multiple search components (warning at the end of this section).

We should probably have a larger conversation about which component(s) we want to allow being placed twice, and adding documentation around that. I think in an ideal case we would never need to add anything twice --- everything that goes in the "drawer" on narrow screens would appear in the top bar on wide screens, and the CSS breakpoints would just magically move everything. I don't recall the details of the discussions when the drawer was created, but I'm assuming that if that were possible that's what we would have done, so probably we're stuck with placing some things twice and we need to decide what components that is allowed for and handle IDs (and keyboard shortcuts, and ???) accordingly.

[12rambau]

Unless by "the other templates" you were referring specifically to the three I asked about at the end of my earlier comment: layout.html, search.html or sidebar-primary.html

yes I was referring to this very one.

Also a common use case for me to use the same component in multiple places is top and bottom of article (like breadcrumb or external_links)

[drammock]

Also a common use case for me to use the same component in multiple places is top and bottom of article (like breadcrumb or external_links)

Do we need IDs for those though? seems like many (most?) of our components don't need IDs and thus can be freely added multiple times to the page with no problems.

[12rambau]

Do we need IDs for those though? seems like many (most?) of our components don't need IDs and thus can be freely added multiple times to the page with no problems.

Is it important for a11y ? if no I'm +1 to remove any unecessary class

[drammock]

this looks pretty good! Couple questions/suggestions.

[drammock]

slightly better name IMO would be html_id_generator

[gabalafou]

The "get" was supposed to hint at the fact that this function is memoized, so that get_id_generator("foo") == get_id_generator("foo"). I wasn't sure if programmers in Python that see _generator at the end of a function name might expect the function to return a unique generator each time they call it. If not, then I agree your suggestion is better.

[gabalafou]

So it's really more like "create_or_get"

[drammock]

for Python devs, "generator" means "iterator that yields single entries on demand / doesn't precompute/store all its entries". It doesn't imply unique values each time it's called. IDK about other python devs, but prefixing with "get_" does not unambiguously communicate "memoized" to me.

In other news, is the output type hint correct here? I don't regularly use type hints in my other projects 🙈 what is yielded is always str (never a list of str, tuple of str, etc)

[gabalafou]

Totally with you on "get" not communicating "memoized." In general, I don't like trying to convey any subtlety via a function name, so I'm glad you called my questionable choice into question. Maybe the function name should be get_or_create_html_id_generator? A bit verbose—but clear—I think.

doesn't imply unique values each time it's called

My question about _generator() function was not about the generator returning unique values but about the generator instance being unique. I think that the functions in the Python standard library follow this rule, for example:

itertools.count() != itertools.count()

Whereas my generator function:

html_id_generator() == html_id_generator()

None of this is really an issue here given that this function isn't being used in a lot of different places. Asking more from a place of wanting to understand, learn and grow.

[gabalafou]

As for the type hint, I got that from somewhere off the Internet, don't remember where. I think I read that it's acceptable to use Iterator in the type hint for the return value of a generator function.

[drammock]

Maybe the function name should be get_or_create_html_id_generator? A bit verbose—but clear—I think.

To me it is misleading to call it "get_..._generator" because it implies that it returns a generator (it does not; it returns a string).

I think I read that it's acceptable to use Iterator in the type hint for the return value of a generator function

It's correct if the function returns a generator. It's incorrect (but usually not detected by type checkers! --- because strings are iterable) if the function is a generator and returns a string.

[gabalafou]

As I was revisiting this PR today, I realized that I'm not sure we ever resolved this discussion.

In the most up to date version of this PR, the function looks like this:

@lru_cache(maxsize=None)
def get_or_create_id_generator(base_id: str) -> Iterator[str]:
    for n in count(start=1):
        if n == 1:
            yield base_id
        else:
            yield f"{base_id}-{n}"

I'm totally confused by your previous remarks (about this function returning a string, not a generator). The following Python REPL may help explain why:

>>> from itertools import count
>>> def get_or_create_id_generator(base_id):
...     for n in count(start=1):
...         if n == 1:
...             yield base_id
...         else:
...             yield f"{base_id}-{n}"
... 
>>> maybe_generator = get_or_create_id_generator("foo")
>>> type(maybe_generator)
<class 'generator'>
>>> type("foo")
<class 'str'>
>>> next(maybe_generator)
'foo'
>>> next(maybe_generator)
'foo-2'

As the REPL log shows, when I call a function that is nearly identical to the one I wrote and check the type of the return value, I get <class 'generator'>, which suggests to me that the return type is a generator not a string. Furthermore, when I pass the return value to next() I get the next string value in a sequence, which suggests to me that the thing returned by my get_or_create_id_generator is something that follows the iterator protocol and therefore can be typed with Iterator[str] ... unless I have misunderstood something along the way here, which is totally possible ... Does the @lru_cache decorator change things?

[drammock]

you have not misunderstood anything. I was simply wrong / looking too quickly / not thinking clearly when I said those things. Sorry to have sent you down a rabbit hole.

[gabalafou]

Ah no worries! I didn't mind the rabbit hole :D

[drammock]

what's the status here? I see some failing CIs, @gabalafou do you need assistance sorting those out? (I haven't looked to see what they are yet)

[12rambau]

the one crashing are related to deps issues. I don't think they should be sort out in this PR. same for the RDT build. I solved the RDT one in #1482 and I will work on thothers once it's merge for traceability.

[gabalafou]

@drammock I request your review again because I would like you to double check two things, if you have the time:

1. The name and return type of the generator function (I left a long comment with a REPL log about this)
2. The way I separated the cacheable versus uncacheable parts of the generate_header_nav_html function