Fix custom tooltip showing "null" on sliders with child elements#5580
Fix custom tooltip showing "null" on sliders with child elements#5580
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTooltip event handlers in the UI are updated to use ChangesTooltip Event Handler Refinement
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f82ab3c to
b6335d3
Compare
b6335d3 to
fdd47a7
Compare
|
thanks for spotting and fixing this. The original code works for me on chrome and safari. On what setup did it not work? |
The original bug appeared in Firefox for me. |
|
Not seeing it in firefox either... |
|
Looks like a good update in general, I can't judge if it would have any negative side effects. Maybe the rabbit can dig a little. |
|
@coderabbitai does this change have any unforseen consequences? Please evaluate for all major browsers and operating systems. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
✅ Browser SupportBoth 🔍 Behavioral Difference (the core of this fix)
This is precisely what caused the "null" bug. The old
🖱️ Mouse (Desktop) — Chrome, Firefox, Safari, EdgeNo regressions. The tooltip will show on hover and dismiss on exit, firing exactly once each way. This is the intended behavior and matches what a native 👆 Touch (Mobile/Tablet)For touchscreen browsers that allow direct manipulation, an implicit pointer capture is called on the element when a This means on touch devices, tooltips were already effectively non-functional in both old and new code during a touch-drag interaction (e.g., sliding the slider). This is unchanged behavior — no regression. 🖊️ Pen / StylusThe
|
| Concern | Old (pointerover/out) |
New (pointerenter/leave) |
|---|---|---|
| Browser support | ✅ All major | ✅ All major |
| Child element re-firing | ❌ Yes (causes "null" bug) | ✅ No |
| Mouse hover | ✅ Works | ✅ Works |
| Touch (drag) | ||
| Pen/stylus hover | ✅ Works | ✅ Works |
| Unforeseen regressions | — | None identified |
The change is semantically correct, well-supported across all major browsers and OSes, and actually aligns with what MDN explicitly recommends for this exact use case. No unforeseen negative consequences were found.
🌐 Web query:
pointerenter pointerleave vs pointerover pointerout browser compatibility differences
💡 Result:
Behavioral differences
| Event pair | Bubbles? | Fires when moving between child elements inside the same parent? | “Entered/left” meaning |
|---|---|---|---|
pointerover / pointerout |
Yes | Yes (because the event target changes as you cross child boundaries) | Crosses the hit-test boundary of the current target element (can be a child). [2] |
pointerenter / pointerleave |
No | No (they don’t bubble, and pointerleave waits until you leave the element and all descendants) |
pointerenter: into element or any descendant; pointerleave: out of element and all descendants. [2] |
Also: if you use pointer capture (setPointerCapture), browsers suppress pointerover/pointerout/pointerenter/pointerleave while capture is active (because retargeting prevents normal hit-testing). [3]
Browser compatibility differences (practical)
- In modern browsers, all four are available as part of the Pointer Events API (Chrome/Edge, Firefox, Safari, iOS Safari). Safari support starts at Safari 13 (and iOS Safari 13.2+ listed as supported). [1]
- IE10 is a notable outlier: Pointer Events are partial there, and the partial-support note is specifically that it lacks
pointerenterandpointerleave. (IE11 supports Pointer Events.) [1] - MDN also marks
pointerenteras “widely available” (Baseline) across browsers since July 2020, reflecting that modern evergreen browser versions have long supported it. [4]
Rule of thumb: use pointerenter/pointerleave when you want “hover-like” semantics that don’t spam when moving across children; use pointerover/pointerout when you need bubbling/delegation (but expect extra events as the target changes). [2]
Sources: [1] Can I use: Pointer Events support table; [2] W3C Pointer Events spec (Level 2 working draft) event definitions/bubbling; [3] MDN Pointer Events (pointer capture note); [4] MDN pointerenter event page (Baseline statement).
🌐 Web query:
pointerenter pointerleave touch device behavior mobile browser support 2024
💡 Result:
What pointerenter / pointerleave do on touch devices
- On devices that don’t support hover (typical phones/tablets), browsers may dispatch
pointerenteras a result ofpointerdown(not from “hovering”), because the finger only becomes a “pointer” when it touches the screen. (developer.mozilla.org) pointerleavefires when the pointer leaves the element’s hit-test bounds; for touch this generally corresponds to the contact ending / moving away, not a hover-out. (developer.mozilla.org)
Mobile browser support status (relevant for 2024)
- Pointer Events (and thus
pointerenter/pointerleave) are supported in iOS Safari from 13.2+, with partial support in iOS Safari 13, and no support in iOS Safari 12.5 and earlier. (caniuse.com) - On modern Android browsers (Chromium-based), Pointer Events support is broadly available. (caniuse.com)
iOS Safari / WebKit gotchas you should still account for
- Historical WebKit behavior:
pointerenter/pointerleave(and related over/out) might not fire unless a page also registerspointerdown/pointermove/pointeruplisteners (bug was marked fixed, but it’s a common legacy pitfall when testing older iOS/Safari or embedded WebViews). (bugs.webkit.org) - WebKit has had edge-case bugs where iOS Safari could emit unexpected
pointerenter/pointerleavesequences and/or unexpectedpointerTypevalues in specific situations (e.g., focusable elements, DOM removal timing). (bugs.webkit.org)
Practical guidance (2024-era best practice)
- For “hover-like” UI, don’t rely on
pointerenter/leavealone on mobile; prefer explicit interaction events (pointerdown,pointerup,click) and design mobile states accordingly. (developer.mozilla.org) - If you use
pointerenter/leavefor styling, consider gating hover UX with CSS media queries like(any-hover: hover)rather than assuming touch can “hover” (touch typically can’t). (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Element/pointerenter_event
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Element/pointerleave_event
- 3: https://caniuse.com/pointer
- 4: https://caniuse.com/pointer
- 5: https://bugs.webkit.org/show_bug.cgi?id=197882
- 6: https://bugs.webkit.org/show_bug.cgi?id=214609
- 7: https://developer.mozilla.org/en-US/docs/Web/API/Element/pointerenter_event
- 8: https://developer.mozilla.org/en-US/docs/Web/API/Element/pointerenter_event
@DedeHai: Here's a thorough cross-browser/OS evaluation of the pointerover/pointerout → pointerenter/pointerleave change:
🔍 Core Behavioral Difference
The key distinction is bubbling:
| Event pair | Bubbles? | Re-fires when crossing child element boundaries? |
|---|---|---|
pointerover / pointerout |
✅ Yes | ✅ Yes — root cause of the "null" bug |
pointerenter / pointerleave |
❌ No | ❌ No — fires once on entry/exit of the whole element+descendants |
The old pair fires again whenever the pointer crosses a child element boundary (because the event target changes), while pointerenter/pointerleave wait until the pointer has truly left the element and all its descendants before firing. This is precisely why the "null" bug occurred.
✅ Desktop Browser Support
pointerenter is marked "widely available" (Baseline) across browsers since July 2020, meaning all modern evergreen browser versions have long supported it. This covers Chrome, Firefox, Safari, and Edge without any concern.
The only notable outlier is IE10, which has partial Pointer Events support and specifically lacks pointerenter and pointerleave — IE11 and all modern browsers support them. IE10 is not a relevant target for WLED.
📱 Touch / Mobile
On devices that don't support hover (typical phones/tablets), browsers may dispatch pointerenter as a result of pointerdown (not from "hovering"), because the finger only becomes a "pointer" when it touches the screen. pointerleave fires when the pointer leaves the element's hit-test bounds, which for touch generally corresponds to the contact ending or moving away.
This is the same general behavior as before with pointerover/pointerout — tooltips on touch were already non-interactive hover UIs, so no regression here.
iOS Safari support for Pointer Events (including pointerenter/pointerleave) starts at iOS Safari 13.2+, with partial support in iOS 13 and no support in 12.5 and earlier. Given that WLED targets modern devices, this is not a practical concern.
⚠️ One Minor iOS/WebKit Caveat
There was a historical WebKit bug where pointerenter/pointerleave might not fire unless a page also registers pointerdown/pointermove/pointerup listeners — though this bug was marked as fixed. If WLED's tooltip code is the only pointer event listener on those elements, very old embedded WebViews could theoretically miss the events, but this is an unlikely edge case for WLED's use case.
⚙️ Pointer Capture (Slider Dragging)
When pointer capture (setPointerCapture) is active — which browsers invoke implicitly during slider dragging — all four event types (pointerover, pointerout, pointerenter, pointerleave) are suppressed, since retargeting prevents normal hit-testing. This means slider-drag behavior is identical between old and new code.
🏁 Summary
| Scenario | Old (pointerover/out) |
New (pointerenter/leave) |
|---|---|---|
| Desktop Chrome/Firefox/Edge | ✅ Works (but buggy) | ✅ Works correctly |
| Desktop Safari | ✅ Works (but buggy) | ✅ Works correctly |
| Child element re-firing | ❌ Yes → causes "null" bug | ✅ No |
| Touch (mobile/tablet) | ||
| Slider drag (pointer capture) | ||
| iOS Safari 12.5- | ✅ Supported | ❌ Not supported |
| IE10 | ✅ Supported | ❌ Not supported |
Verdict: The change is semantically correct, aligns with the W3C recommended approach for hover-like semantics, and has no unforeseen negative consequences for WLED's supported browser targets. The only platforms that lose support (IE10, iOS ≤12.5) are effectively irrelevant for WLED's use case. The old code was actually the problematic implementation.
|
I can't recreate the initial problem anymore, myself. I only cared to validate the fix (which obviously works, when the initial problem isn't even there). I don't know how I managed to create the initial error state in the first place (it just happened during regular usage). Since I can't recreate it, I don't think anything should be fixed, until we know what the initial problem was. So I'm closing this PR, but I'll keep the Issue open if that's okay. Maybe other people will come encounter this same error state. |
closes #5578
Summary
Problem
The custom tooltip implementation at
wled00/data/index.jssaves an element's title attribute to data-title on hover, removes title (to suppress the native browser tooltip), and restores it from data-title on exit.Because pointerover bubbles, the handler fired again whenever the cursor crossed onto a child element (e.g. moving from the sliderwrap div onto its or .sliderdisplay child). On that second invocation getAttribute("title") returned null (it had already been removed), and setAttribute("data-title", null) stored the literal string "null". Subsequent hovers then displayed a tooltip reading "null".
This was reproducible on the Value/Brightness slider on the Colors tab, and could affect any element with a title attribute that contains child nodes the cursor can land on.
Fix
pointerenter/pointerleave do not bubble — they fire only on the actual entry to and exit from the listening element, regardless of internal cursor movement. This makes the handler idempotent for a single hover and matches the intended semantics.
AI Disclaimer: Claude Code was used to find and fix this bug and create this PR description. However it's not "vibe coded" as I understand the code completely and have reviewed and tested it manually.
Summary by CodeRabbit