fix(QTooltip): show on keyboard focus & dismiss with ESC (fix #18241) - #18318
Conversation
📝 WalkthroughWalkthrough
ChangesTooltip accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant KeyboardUser
participant AnchorElement
participant QTooltip
participant EscapeKeyStack
KeyboardUser->>AnchorElement: focus
AnchorElement->>QTooltip: focusShow
QTooltip->>QTooltip: show when :focus-visible matches
QTooltip->>EscapeKeyStack: register onEscapeKey
KeyboardUser->>QTooltip: press ESC
QTooltip->>QTooltip: hide
AnchorElement->>QTooltip: blur
AnchorElement->>QTooltip: delayHide
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ui/src/components/tooltip/QTooltip.js (1)
191-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the interaction matrix.
Cover keyboard focus display, pointer focus suppression, delayed blur, ESC dismissal without focus movement,
persistent,noParentEvent, and repeated show/hide cleanup.Also applies to: 316-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/components/tooltip/QTooltip.js` around lines 191 - 201, Add regression tests for QTooltip covering keyboard-triggered focus display, pointer-triggered focus suppression, delayed blur, ESC dismissal without moving focus, persistent behavior, noParentEvent behavior, and repeated show/hide listener cleanup. Exercise the existing focus, blur, escape-stack, and visibility paths around the computed handlesEscape logic and related lines, including interaction combinations rather than isolated cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/components/tooltip/QTooltip.js`:
- Around line 341-344: Update the event handling around QTooltip’s anchor
mouseenter/mouseleave/focus/blur listeners to track hover and focus
independently, and only execute delayHide when neither state remains active.
Preserve delayShow behavior while preventing blur from hiding during hover and
mouseleave from hiding during focus.
- Around line 321-326: Update the focus handling around the
el.matches(':focus-visible') check so a selector parsing failure explicitly
treats the event as pointer focus and returns before delayShow(). Preserve the
existing return for elements that do not match :focus-visible, ensuring
unsupported engines do not open the tooltip on click focus.
- Around line 343-344: Update the focus event listeners in the tooltip anchor
handling to use focusin/focusout, or otherwise account for events from nested
focusable elements, so wrapper anchors correctly show on descendant focus and
hide when focus leaves the wrapper while preserving the existing focusShow and
delayHide behavior.
---
Nitpick comments:
In `@ui/src/components/tooltip/QTooltip.js`:
- Around line 191-201: Add regression tests for QTooltip covering
keyboard-triggered focus display, pointer-triggered focus suppression, delayed
blur, ESC dismissal without moving focus, persistent behavior, noParentEvent
behavior, and repeated show/hide listener cleanup. Exercise the existing focus,
blur, escape-stack, and visibility paths around the computed handlesEscape logic
and related lines, including interaction combinations rather than isolated
cases.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5a2b1c2-11e9-4e69-811d-265b87af0532
📒 Files selected for processing (1)
ui/src/components/tooltip/QTooltip.js
| // only react to keyboard focus, not to focus coming from a pointer, | ||
| // so the tooltip doesn't pop up when the target is clicked; | ||
| // guard the call for engines that don't support :focus-visible | ||
| try { | ||
| if (el.matches(':focus-visible') === false) return | ||
| } catch {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'focusShow|:focus-visible|Cordova|Electron' .Repository: quasarframework/quasar
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- QTooltip context ---\n'
sed -n '280,360p' ui/src/components/tooltip/QTooltip.js
printf '\n--- focus-related search in QTooltip ---\n'
rg -n -C 3 'delayShow|focusShow|focus-visible|matches\\(' ui/src/components/tooltip/QTooltip.jsRepository: quasarframework/quasar
Length of output: 2620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '240,340p' ui/src/components/tooltip/QTooltip.jsRepository: quasarframework/quasar
Length of output: 2728
Make the :focus-visible fallback explicit.
If el.matches(':focus-visible') throws, the empty catch still reaches delayShow(), so click focus opens the tooltip in engines that don’t parse the selector. Use an explicit keyboard/pointer fallback instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/components/tooltip/QTooltip.js` around lines 321 - 326, Update the
focus handling around the el.matches(':focus-visible') check so a selector
parsing failure explicitly treats the event as pointer focus and returns before
delayShow(). Preserve the existing return for elements that do not match
:focus-visible, ensuring unsupported engines do not open the tooltip on click
focus.
| [anchorEl.value, 'mouseenter', 'delayShow', 'passive'], | ||
| [anchorEl.value, 'mouseleave', 'delayHide', 'passive'] | ||
| [anchorEl.value, 'mouseleave', 'delayHide', 'passive'], | ||
| [anchorEl.value, 'focus', 'focusShow', 'passive'], | ||
| [anchorEl.value, 'blur', 'delayHide', 'passive'] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Coordinate hover and focus before hiding.
mouseenter/mouseleave and focus/blur now share delayShow/delayHide, but each hide path is unconditional. When hover and keyboard focus overlap, blur can hide a tooltip while the pointer remains over the anchor, and mouseleave can hide it while focus remains. Track both states, or gate hiding until neither trigger is active.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/components/tooltip/QTooltip.js` around lines 341 - 344, Update the
event handling around QTooltip’s anchor mouseenter/mouseleave/focus/blur
listeners to track hover and focus independently, and only execute delayHide
when neither state remains active. Preserve delayShow behavior while preventing
blur from hiding during hover and mouseleave from hiding during focus.
| [anchorEl.value, 'focus', 'focusShow', 'passive'], | ||
| [anchorEl.value, 'blur', 'delayHide', 'passive'] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'focusin|focusout|focusShow|<button|<input|target=' .Repository: quasarframework/quasar
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of QTooltip.js with line numbers
sed -n '300,380p' ui/src/components/tooltip/QTooltip.js
# Find where anchorEl is assigned and how listeners are attached
rg -n -C 3 'anchorEl|focusShow|delayHide|focusin|focusout|addEventListener|on\(' ui/src/components/tooltip/QTooltip.js
# Check whether nested focusable support is documented or used nearby
rg -n -C 2 'focusin|focusout|nested focusable|anchorEl' ui/src/components/tooltipRepository: quasarframework/quasar
Length of output: 9880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the useAnchor composable and inspect how anchorEl is resolved
fd -a 'use-anchor' ui/src
fd -a 'useAnchor' ui/src
rg -n -C 4 'function useAnchor|const anchorEl|anchorEl =|anchorEl.value|configureAnchorEl|canShow' ui/src
# Read the relevant implementation if found
file=$(rg -l 'function useAnchor|const anchorEl|anchorEl.value' ui/src | head -n 1)
if [ -n "${file:-}" ]; then
sed -n '1,260p' "$file"
fiRepository: quasarframework/quasar
Length of output: 28803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' ui/src/composables/private.use-anchor/use-anchor.js
printf '\n--- QTooltip docs/usages ---\n'
rg -n -C 3 'QTooltip|q-tooltip|focusShow|blur|focusin|focusout|target: true|target === true' ui/src/components ui/src/composables ui/src/docs ui/srcRepository: quasarframework/quasar
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect QTooltip API/docs if present
fd -a 'QTooltip.json' ui/src
fd -a 'QTooltip.*' ui/src/components/tooltip
# Read any tooltip API file and nearby slot/docs text
for f in $(fd -a 'QTooltip.json' ui/src/components/tooltip ui/src); do
echo "--- $f ---"
sed -n '1,240p' "$f"
done
# Look for explicit target/anchor guidance
rg -n -C 2 '"target"|anchor|parentNode|focusin|focusout|focusShow|delayHide' ui/src/components/tooltip ui/src/composables/private.use-anchorRepository: quasarframework/quasar
Length of output: 26628
Handle focus transitions on wrapper anchors. focus/blur only fire on anchorEl.value itself, so a tooltip anchored to a wrapper with nested focusable children won’t show/hide when focus moves to the child. Use focusin/focusout or inspect evt.target if that anchor shape is supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/components/tooltip/QTooltip.js` around lines 343 - 344, Update the
focus event listeners in the tooltip anchor handling to use focusin/focusout, or
otherwise account for events from nested focusable elements, so wrapper anchors
correctly show on descendant focus and hide when focus leaves the wrapper while
preserving the existing focusShow and delayHide behavior.
|
@coderabbitai In the ui package, components must not create reactive state that the current platform never uses. In this PR, the new computed prop and its watcher only serve desktop keyboard-focus handling, yet they are also created on mobile where they are never consumed. In future reviews of ui/src, flag computed props, watchers and event listeners that are created unconditionally but only consumed on one platform (desktop-only hover/keyboard logic, mobile-only touch logic); their creation should be gated behind the platform check. This matters because these components run on every Quasar app, so per-instance reactive overhead multiplies quickly. |
|
Tip For best results, initiate chat on the files or code changes.
🐇💭 ✏️ Learnings added
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Failed to generate fixes. The agent execution returned an error and no code changes were found. |
|
@arbaev Thanks for contributing! And sorry for the coderabbit noise. This will go into Quasar v2.22 later today. |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
The PR fulfills these requirements:
devbranchfix #18241)Other information:
Fixes #18241.
Problem
QTooltip only showed on mouse hover (and touch), so keyboard-only users never saw it when tabbing to the target — a violation of WCAG 1.4.13 (Content on Hover or Focus).
Change
In the tooltip's anchor wiring (desktop):
:focus-visibleso the tooltip does not appear on plain pointer clicks (mouse users keep hover).blur.ESCwhile shown, via Quasar's shared escape-key stack (same mechanism as QMenu/QDialog) — only the top-most popup reacts and focus is not moved (WCAG "dismissible").No new props/events; respects the existing
noParentEventandpersistent.The touch/mobile path is unchanged.
Scope / note
A focusable trigger is required (QBtn, links, inputs…). A tooltip on a non-focusable element (a bare
<div>withouttabindex) still can't receive focus — that remains the app's responsibility.Verification
Built from source and exercised in headless Chromium (Playwright):
:focus-visible) → not shownCordova/Electron not separately tested — the change uses standard DOM
focus/blur+:focus-visibleand the existing escape-key stack, with no platform-specific code. No unit test added: QTooltip currently has no test file and the spec harness requires full-API coverage for any existing test file; the:focus-visiblegating also can't be exercised under jsdom.Happy to follow up with full QTooltip test coverage separately.
Summary by CodeRabbit