shell: Fix and test skip links #20561
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing `key_press()` is named and implemented poorly, is hard to use for non-letter characters and requires Chromium/Firefox specific implementations. Introduce a new `key()` function which takes a *single* key name. This is portable across browsers, and something like "Enter" is much easier to read than "\r". TestHostSwitching.testEdit already used it that way.
|
@marusak As you introduced the skip links, I'd appreciate your review. My interactive test seems quite reasonable now (no reloads and focusing works), but I'd like to make sure it's what you had in mind. @jelly You recently dabbled with the keyboard test API -- please let me know if |
jelly
reviewed
Jun 7, 2024
| # FIXME: This should be key_press(), and key_press() should be something else | ||
| def key(self, name: str) -> None: | ||
| self.cdp.invoke("Input.dispatchKeyEvent", type="keyDown", key=name) | ||
| self.cdp.invoke("Input.dispatchKeyEvent", type="keyUp", key=name) |
There was a problem hiding this comment.
key sounds like a good compromise, breaking key_press is imo a no-go at this moment as everyone uses it.
There was a problem hiding this comment.
Yes, I thought about .key() and .input_text(), moving everything over to these two, and eventually dropping .key_press()
We don't want the skiplinks to *actually* change the URL -- they will go from e.g. `/system` to `/cockpit/@localhost/shell/index.html#content`. This causes a page reload, breaks the focusing, and is also not an URL which we actually want to expose. Fix this by stopping event progression. Also remove the `return false` -- event handlers don't return anything [1]. Introduce a `TestMenu.testKeyboardNavigation` -- skip links and tabbing was previously not tested at all. [1] https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#the_event_listener_callback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We don't want the skiplinks to actually change the URL -- they will go
from e.g.
/systemto/cockpit/@localhost/shell/index.html#content.This causes a page reload, breaks the focusing, and is also not an URL
which we actually want to expose. Fix this by stopping event
progression.
Also remove the
return false-- event handlers don't return anything.Introduce a
TestMenu.testKeyboardNavigation-- skip links and tabbingwas previously not tested at all.
[1] https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#the_event_listener_callback
Preparation for #20553 -- let's first test skiplinks before we change them.