ui.skip_to_main: skip to main content button#5790
ui.skip_to_main: skip to main content button#5790evnchn wants to merge 5 commits intozauberzeug:mainfrom
ui.skip_to_main: skip to main content button#5790Conversation
|
Thanks for the contribution, @evnchn! The "skip to main content" feature is certainly a great accessibility improvement. One note for future PRs: It's best to lead with what the change does and how it works, so reviewers can quickly understand the PR. Personal context and background story are fine to include as a comment on the PR, but keeping the description itself focused on the technical substance helps people "pick up" the PR faster - and keeps commit messages clean when squash-merging. |
|
Been a while since I wrote any PR message. Will bear this in mind 🙇 @falkoschindler as you scheduled this for 3.10, do you think it's a good idea to schedule #5795 also for 3.10 as well? And preferably early 3.10 so that I can get the accessibility documentation by late 3.10. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@falkoschindler Ready for your review into 3.10 I'd say. All checklist items done. |
Use JS click instead of native Selenium click for the skip-to-main button. The button is off-screen (left: -9999px) until focused, and Selenium native click() may check interactability before the browser re-layouts after focus(), causing ElementNotInteractableException on some Chrome versions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
falkoschindler
left a comment
There was a problem hiding this comment.
Thanks again for the PR, @evnchn!
It needs some more work though. We should clarify API and behavior first before looking into some implementation details. Anyway, I'll move it to milestone "Next": We can do it whenever we have time, but don't need to specify an individual release.
1. PR description needs work
The PR description focuses on personal background and the process of building the feature rather than the technical substance. It should lead with what the feature does, why it's needed (accessibility / WCAG compliance), and how it works. The test script is helpful but belongs after a clear technical explanation.
2. Implicit targeting is surprising and fragile
The connection between ui.skip_to_main() and its target is invisible — the user just has to know that the next element they create becomes the target. There's nothing in the code that makes this relationship explicit. This is unlike any other NiceGUI element and will be a source of confusion. Consider accepting an explicit target parameter: ui.skip_to_main(target=main_content).
3. Name vs. capability mismatch
The name skip_to_main implies a single "main content" target, but the implementation supports multiple buttons targeting arbitrary elements. If the intent is to skip to different sections, the name is too narrow. If the intent is truly "skip to main content," why support multiple buttons?
The standard accessibility term for this pattern is "skip link" (see also WCAG 2.4.1, WebAIM). ui.skip_link would be both more standard and more flexible — it doesn't lock the API into the single-target assumption.
3. next_element_id is a fragile targeting mechanism
skip_to_main.py:32 and skip_to_main.py:36: The target is set to self.client.next_element_id, assuming the very next element created will be the intended target. This is the only place in the codebase that reads next_element_id outside of Element.__init__. If any internal NiceGUI bookkeeping creates an element in between (e.g. a future refactor adds a wrapper element, or a decorator/timer fires), the target silently points to the wrong element. The user has no way to detect or debug this.
4. JS click handler has no null guard — will throw on missing target
skip_to_main.py:27-30: If the target element is removed or never created (e.g. conditional rendering), document.getElementById(...) returns null and the next line throws TypeError: Cannot read properties of null.
5. Should use getHtmlElement instead of document.getElementById
skip_to_main.py:28: The JS handler manually constructs 'c' + e.target.dataset.target and uses document.getElementById. NiceGUI provides getHtmlElement(id) in nicegui.js which already handles the c prefix and is the idiomatic way to look up element DOM nodes. Other elements (e.g. code.py, scene_view.js) use getElement/getHtmlElement consistently.
6. Docstring doesn't explain usage
skip_to_main.py:8-17: The docstring describes what the button does but not how to use it. It doesn't explain the critical detail that the target is the next element created after the call, nor that the context manager resets the target on exit. A user reading only the docstring would have no idea how targeting works.
7. Constructor doesn't accept text parameter
skip_to_main.py:7: __init__ takes no arguments, so ui.skip_to_main('Go to content') doesn't work. Users must use the context manager just to change the label text. Since TextElement already supports a text parameter, consider exposing it. This would keep the simple case simple and reserve the context manager for truly custom content (icons, styling, etc.).
8. Default styling could use some padding
The focused button is visible but looks a bit cramped without any padding. Consider adding a small padding to the base .nicegui-skip-to-main class.
Also worth considering: should this inherit from ui.button (Quasar's QBtn) instead of a plain <button> via TextElement? That would give consistent styling with the rest of NiceGUI's components for free. On the other hand, a plain <button> is lighter and arguably more robust for an accessibility primitive. Either way, the current unstyled <button> sits in an awkward middle ground — not Quasar-styled, not explicitly minimal either.
9. __exit__ missing return type annotation
skip_to_main.py:34: def __exit__(self, *_) is missing the -> None return annotation. The *_ pattern is fine — it matches the convention in element.py, client.py, slot.py, etc.
10. Test quality and coverage
Existing tests:
test_context_managerchecks attributes but never clicks the button — it tests wiring, not behaviorget_attribute('innerText')is non-idiomatic for Selenium;.textis the standard way to get visible text'Jump to content' in button.get_attribute('innerText')is a loose substring check — doesn't verify that the original "Skip to main content" text was actually cleared by__exit__screen.wait(0.5)is a hard-coded sleep; ascreen.should_containor explicit wait-for-condition would be more robust
Missing coverage:
- DOM ordering logic (lines 22-25): if
ui.skip_to_main()is called after other elements, does it actually appear first? - Multiple
skip_to_main()on the same page (the scenario from the PR description with 3 skip buttons) - No subsequent element (edge case — target points to a non-existent ID)
Note: some of these gaps may be hard to test reliably (e.g. focus behavior in headless Selenium) or may become obsolete if the API changes based on earlier review findings (e.g. explicit target parameter would make the ordering and "no subsequent element" scenarios straightforward to test — or irrelevant).
Motivation
These days, I have been testing some stuff on my Windows laptop (for multi-Claude as well - you can do so much with a MacBook).
My Windows laptop's touchpad has broke for several years now (thanks Lenovo), and TIL
https://nicegui.iodoes NOT offer a "skip to main content" function for us tab-navigators.Implementation
The PR was bootstrapped by Claude Code but it was utter garbage tbh.
ui.skip_to_main()nicegui/static/nicegui.cssI question everything and whittled things down.
Progress
Test script
Final notes
This PR description is manually-written, if it isn't obvious enough already...