fix(a11y): WCAG 1.4.13 — make hover content dismissable and hoverable#39239
fix(a11y): WCAG 1.4.13 — make hover content dismissable and hoverable#39239Aitema-gmbh wants to merge 3 commits intoapache:masterfrom
Conversation
- Add Escape key handler to TaskStackTracePopover (useEffect + keydown) - Add Escape key handler to TaskPayloadPopover (useEffect + keydown) - Add Escape key handler to AG Grid CustomPopover (keydown in existing useEffect) - Fix DeckGL Tooltip: pointerEvents none->auto, add Escape dismiss with state reset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review Agent Run #1b45a3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // Reset dismissed state when tooltip content changes (new hover target) | ||
| useEffect(() => { | ||
| setDismissed(false); | ||
| }, [tooltip?.x, tooltip?.y]); |
There was a problem hiding this comment.
Suggestion: The dismiss state is reset on every mouse-coordinate update, so pressing Escape can be immediately undone as soon as the cursor moves by a pixel while still hovering. This breaks the dismiss behavior by making the tooltip reappear without ending the hover interaction. Reset the dismissed state only after hover ends (when tooltip becomes null/undefined), so Escape dismissal remains effective until the user leaves and re-enters. [logic error]
Severity Level: Major ⚠️
- ❌ Deck.gl map tooltips reappear after Escape while hovering.
- ⚠️ Breaks WCAG 1.4.13 hover-content dismissal for deck.gl.| // Reset dismissed state when tooltip content changes (new hover target) | |
| useEffect(() => { | |
| setDismissed(false); | |
| }, [tooltip?.x, tooltip?.y]); | |
| // Reset dismissed state only after hover ends, so Escape dismissal persists while hovered | |
| useEffect(() => { | |
| if (!tooltip) { | |
| setDismissed(false); | |
| } | |
| }, [tooltip]); |
Steps of Reproduction ✅
1. Open any deck.gl visualization in Superset that uses `DeckGLContainer` (e.g., a
Screengrid chart; container implementation at
`superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:62-187`), so that
map interactions and tooltips are provided by `DeckGLContainer`.
2. Hover over a rendered data point so that the layer `onHover` handler calls `setTooltip`
with the mouse coordinates, for example in Screengrid: `Screengrid.tsx:170-23` sets
`setTooltip({ content: tooltipContent(enhancedInfo), x: info.x, y: info.y });`, and
`DeckGLContainer` stores this in its `tooltip` state (`DeckGLContainer.tsx:62-66`).
3. Observe the tooltip rendered via `renderTooltip` in `DeckGLContainer`
(`DeckGLContainer.tsx:113-121`), which mounts the `Tooltip` component with the current
`tooltip` prop; `Tooltip` manages a local `dismissed` state and registers an Escape key
handler to set `dismissed` to `true` when `e.key === 'Escape'` (`Tooltip.tsx:69-85`).
4. Press Escape to dismiss the tooltip (setting `dismissed` to `true` in
`Tooltip.tsx:78-80` so `Tooltip` returns `null` at `Tooltip.tsx:87-89`), then move the
mouse slightly while still over the same hovered geometry: the layer `onHover` fires again
and updates `tooltip.x`/`tooltip.y`, which causes the `useEffect` at `Tooltip.tsx:71-74`
(dependent on `[tooltip?.x, tooltip?.y]`) to reset `dismissed` back to `false`, making the
tooltip reappear immediately despite the Escape dismissal having been requested.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/preset-chart-deckgl/src/components/Tooltip.tsx
**Line:** 71:74
**Comment:**
*Logic Error: The dismiss state is reset on every mouse-coordinate update, so pressing Escape can be immediately undone as soon as the cursor moves by a pixel while still hovering. This breaks the dismiss behavior by making the tooltip reappear without ending the hover interaction. Reset the dismissed state only after hover ends (when tooltip becomes null/undefined), so Escape dismissal remains effective until the user leaves and re-enters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| // Reset dismissed state when tooltip content changes (new hover target) | ||
| useEffect(() => { | ||
| setDismissed(false); | ||
| }, [tooltip?.x, tooltip?.y]); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The Escape-dismiss state is reset purely when tooltip?.x/tooltip?.y change, so after pressing Escape, any small mouse movement that keeps the pointer over the same DeckGL pickable object causes the tooltip coordinates to update, which immediately clears dismissed and makes the tooltip reappear. As a result, Escape does not provide a stable dismissal for the current hover session.
Suggestion: Reset the dismissed state based on the tooltip lifecycle (e.g., when tooltip is cleared to null or when a stable identity/content key changes) rather than raw cursor coordinates, so that pressing Escape keeps the current tooltip hidden until the user actually leaves and re-enters its trigger.
Code Review Agent Run #810535Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
The deck.gl Tooltip.tsx change has real risk: it flips pointer-events: none → pointer-events: auto. That original none was almost certainly load-bearing, i.e. deck.gl tooltips follow the cursor and sit between the pointer and the interactive map. Making them capture pointer events can:
- Block cursor from reaching the underlying map layer (blocks further hover, clicks, pans)
- Cause flicker loops when the cursor enters the tooltip → map loses hover → tooltip hides → cursor back on map → reappears
- The Escape-dismissed state sticks until tooltip.content changes, so re-hovering the same feature after dismiss won't reshow the tooltip — feels broken.
The PR description mentions ECharts tooltips, but I don't see anything ECharts specific in the code. Can. you explain if/where this affects ECharts?
Lastly, for the ones in /tasks, would we be able to avoid having the key listener here if it were added to the underlying component (popover, tooltip, whatever component it's leveraging) so the fix is more DRY/centralized?
There was a problem hiding this comment.
Pull request overview
Implements parts of WCAG 2.1 SC 1.4.13 by making certain hover-triggered UI overlays dismissible via the Escape key, and by allowing deck.gl tooltips to receive pointer events so they can be hovered.
Changes:
- Add document-level
Escapekey handling to dismiss task hover popovers (payload + stack trace). - Add
Escapekey handling to dismiss the AG Grid custom popover. - Update deck.gl tooltip to be hoverable (
pointer-events: auto) and dismissible via Escape with local “dismissed” state.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| superset-frontend/src/features/tasks/TaskStackTracePopover.tsx | Adds Escape key listener to close the stack trace hover popover. |
| superset-frontend/src/features/tasks/TaskPayloadPopover.tsx | Adds Escape key listener to close the payload hover popover. |
| superset-frontend/plugins/preset-chart-deckgl/src/components/Tooltip.tsx | Enables pointer events on deck.gl tooltip and adds Escape-to-dismiss behavior. |
| superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomPopover.tsx | Adds Escape-to-close behavior for the AG Grid custom popover. |
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') setVisible(false); | ||
| }; | ||
| if (visible) document.addEventListener('keydown', handleKeyDown); |
There was a problem hiding this comment.
The PR description mentions setting ECharts tooltips to enterable: true and adding ESC handlers on chart containers, but these changes don't appear in this diff (e.g. no enterable: true usage found in the frontend). Either update the PR description to match what's actually included, or add the missing ECharts/chart-container changes in this PR.
| if (tooltip && !dismissed) { | ||
| document.addEventListener('keydown', handleKeyDown); | ||
| } | ||
| return () => document.removeEventListener('keydown', handleKeyDown); | ||
| }, [tooltip, dismissed]); |
There was a problem hiding this comment.
In deck.gl, the tooltip prop is updated on every hover move (x/y changes), so using [tooltip, dismissed] here will cause the effect to tear down and re-add the document keydown listener very frequently. Consider depending on a stable boolean (e.g. Boolean(tooltip) / tooltip != null) plus dismissed, or keep the latest tooltip in a ref, to avoid repeated global listener churn.
| // Reset dismissed state only when tooltip content changes (new hover target), | ||
| // not on coordinate changes from mouse movement | ||
| useEffect(() => { | ||
| setDismissed(false); | ||
| }, [tooltip?.content]); |
There was a problem hiding this comment.
Resetting dismissed based only on tooltip?.content can leave the tooltip permanently hidden when the user moves to a different hovered object that renders the same content (common with repeated values). A more reliable approach is to include a stable hover-target identifier in the tooltip state (e.g. deck.gl PickingInfo.index / a feature id) and reset dismissed when that identifier changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39239 +/- ##
==========================================
- Coverage 64.37% 64.36% -0.02%
==========================================
Files 2550 2550
Lines 132180 132210 +30
Branches 30661 30670 +9
==========================================
+ Hits 85096 85102 +6
- Misses 45599 45623 +24
Partials 1485 1485
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e dismiss Addresses Rusackas' regression concern and three bot review items on PR apache#39239: - pointer-events back to `none` on deck.gl tooltips. The previous switch to `auto` was the load-bearing regression Rusackas flagged: deck.gl tooltips track the cursor across the canvas, and capturing pointer events here blocks the underlying map from receiving hover/click and causes flicker loops. WCAG 1.4.13 "hoverable" is still satisfied because the tooltip stays under the cursor while pointing at the feature. - Dismiss-state reset now also fires on the hidden-to-visible transition, not only on content change. Previously, moving off a feature and back onto a different pickable that renders the same content would leave the tooltip permanently dismissed — bot bito flagged this for repeated-value layers. - Escape keydown listener is now bound based on a stable visibility flag derived from `(tooltip && !dismissed)` instead of `[tooltip, dismissed]` directly. The old dependency array re-ran on every cursor coordinate change, churning addEventListener/removeEventListener pairs at hover rate.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
Implements WCAG 2.1 criterion 1.4.13 (Content on Hover or Focus, Level AA).
pointer-events: noneso they continue to track the cursor without blocking layer hover/click; WCAG 1.4.13 hoverable is satisfied because the tooltip remains under the cursor while pointing at the featureTESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Part of a series of 16 individual WCAG 2.1 accessibility PRs. See also #38342.