fix: skip the shortcut owner token when the owner is the UI (#25631) (CP: 25.2) - #25644
Merged
Merged
Conversation
## Summary A shortcut's owner-scope guard puts a unique id into the keydown filter text, and that text is kept for the whole life of the UI by the client expression cache and both constant pools. A shortcut owned by the `UI` does not need that id, because the UI element is `<body>`, which can never be inside an open popover or modal. Such shortcuts now share one filter expression instead of creating a new one per registration. Fixes #25624 ## What changed - `ShortcutRegistration` no longer generates the owner token eagerly. `getOwnerToken()` is now the single place that decides whether a token is needed and creates it, and it returns `null` when none is needed: the guard is off (`allowEventsFromNestedModals`) or the lifecycle owner is the `UI`. Callers just use what they get. - For a UI-owned shortcut the filter is now `window.Vaadin.Flow.shortcut.eventInTopLevelScope(event)`, the same text for every registration. - `FlowShortcut.js` gains `eventInTopLevelScope(event)`, which only checks that the keydown did not originate inside a popover or modal. The shared "which scope did the event come from" logic moved into a small `_originScope` helper used by both guards. - Side fix: with the guard off, `updateOwnerMarker` used to ask for a token before checking the flag, so a registration that never uses one still created it. ## Test summary | # | Status | What the test verifies | Why it matters | |---|--------|------------------------|----------------| | 1 | ✅ | A UI-owned shortcut leaves the UI element unmarked and its filter is `eventInTopLevelScope(event)`, with no owner attribute or token in the text | This is the fix; a token here is what leaked per registration | | 2 | ✅ | Removing and re-registering the same UI shortcut produces exactly the same set of keydown expressions | A filter text that differs per registration grows the client expression cache and both constant pools forever | | 3 | ✅ | A component-owned shortcut still marks its owner and its filter locates that owner by the marker token | The normal, non-UI path must keep working unchanged | | 4 | ✅ | In a real browser, a UI-owned Alt+S stays silent for a keydown inside an open popover and fires once for a keydown on the top layer | The new token-free client helper must guard exactly like the token-based one | | 5 | ✅ | A shortcut owned by a component inside a popover still fires for keydowns from that popover (#24974) | Guards against re-breaking the earlier fix while the test view was reworked | - `ShortcutRegistrationTest.ownerScopeGuard_uiLifecycleOwner_filterHasNoTokenAndUiIsNotMarked` → 1 - `ShortcutRegistrationTest.ownerScopeGuard_uiLifecycleOwner_repeatedRegistrationsShareFilter` → 2 - `ShortcutRegistrationTest.ownerScopeGuard_componentLifecycleOwner_filterLocatesOwnerByToken` → 3 - `PopoverOwnerShortcutIT.uiOwnedShortcut_firesOnlyOutsideThePopover` → 4 (uses the new UI-owned shortcut, second input and sync button added to `PopoverOwnerShortcutView`) - `PopoverOwnerShortcutIT.ownerInsidePopoverFocused_shortcutFires` → 5 (pre-existing, now shares the view with the new case) Deliberately untested: skipping the token when `allowEventsFromNestedModals` is on has no observable effect (no filter is generated in that case at all), and the existing `ownerScopeGuard_lifecycleOwnerNotMarkedWhenAllowed` already pins that the owner ends up unmarked. The `_originScope` extraction in `FlowShortcut.js` is a pure move, still covered by the existing delegate tests. --------- Co-authored-by: totally-not-ai[bot] <290682512+totally-not-ai[bot]@users.noreply.github.com>
vaadin-review-bot
approved these changes
Sep 10, 2026
Collaborator
Author
|
This PR is eligible for auto-merging policy, so it has been approved automatically. If there are pending conditions, auto merge (with 'squash' method) has been enabled for this PR [Message is sent from bot] |
vaadin-bot
enabled auto-merge (squash)
September 10, 2026 13:54
|
Contributor
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR cherry-picks changes from the original PR #25631 to branch 25.2.
Original PR description