fix: #1435 Avoid double taps on buttons with tooltip on safari iOS #1442
Conversation
|
@mrh1997 is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in the pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This breaks all the Popover examples and SpeedDial. Tooltip and Dropdown work as expected. In order to close the popover, you have to hover the popover and then out of the popover. |
|
I close the PR for now. Thank you for your contribution. |
|
Stop please reopen it. I have already created a fix but I am waiting with filing it until I tested it. |
mrh1997
force-pushed
the
fix-ios-tooltip-fail
branch
from
f69367d
to
8146794
Compare
|
@shinokada : Is this fix still lacking any requirements that does prevent it from being authorized? |
|
|
I will have a look into the issue with not closing when clicking outside. But regarding Tooltip does not open on iOS: Is it really not opening?
How do you think about keeping that behviour? |
|
I checked on iPadOS 17.7.
This means developers have to change to |
|
Your proposal (3sec timeout) is a great idea. I will implement it that way (if you are OK with that)! I will not be able to spend time for this issue the next one or two weeks but I will fix it thereafter. So please don't close this pull request... |
|
@shinokada : I fixed the issue now including the timeout you proposed, but before pushing it I would like to get your opinion of another Idea: The solution still suffers from the problem, that on mobile devices when tapping an element for viewing the Tooltip/Popover/... it is also clicked (firing whichever event is behind If you think so, too, I would give this a try... |
|
That may work but I'm not sure how it will affect to web users until I see it in action. |
When triggering a Tooltip/Speeddial/Popover on a touch device it will close after 3secs (or "closeOnTouchDelay" ms) when set to trigger="hover". Furthermore Svelte 5 will now behave identical to Svelte 4 (instead of two touches a single one is required)
mrh1997
force-pushed
the
fix-ios-tooltip-fail
branch
from
8146794
to
0d7447e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/lib/utils/Popper.svelte (4)
20-20: LGTM! Consider enhancing the property documentation.The new
closeOnTouchDelayproperty is well-implemented with a sensible default value of 3000ms.Consider adding more detailed documentation about the special
-1value that keeps tooltips open until tap outside:-@prop export let closeOnTouchDelay: number = 3000; +@prop export let closeOnTouchDelay: number = 3000; // Delay in ms before closing on touch devices. Set to -1 to keep open until tap outside.Also applies to: 33-33, 246-246
66-68: LGTM! Consider making the touch show delay configurable.The 300ms delay for touch events is a good solution to prevent accidental tooltip triggers while maintaining immediate response for mouse events.
Consider making the touch show delay configurable similar to
closeOnTouchDelay:+ export let showOnTouchDelay: number = 300; setTimeout(() => { open = ev.type === 'click' ? !open : true; - }, (ev as PointerEvent).pointerType === "touch" ? 300 : 0) + }, (ev as PointerEvent).pointerType === "touch" ? showOnTouchDelay : 0)
76-91: LGTM! Consider improving type safety.The hide handler implementation properly manages different interaction modes and respects the
closeOnTouchDelaysetting.Consider using type assertion for the timer to avoid the
unknowncast:- timer = setTimeout(() => { + timer = window.setTimeout(() => { if ((ev.type === 'mouseleave' || ev.type === 'pointerleave') && (isTouch || !elements.some(hasHover))) { open = false; } - }, closeDelay) as unknown as number; + }, closeDelay);
172-172: LGTM! Consider optimizing the click outside handler.The click outside handler is a necessary addition to fix the iOS Safari issue where clicking outside doesn't close the popper.
Consider adding a passive event listener for better scrolling performance:
- document.addEventListener('click', closeOnClickOutside); + document.addEventListener('click', closeOnClickOutside, { passive: true }); return () => { - document.removeEventListener('click', closeOnClickOutside); + document.removeEventListener('click', closeOnClickOutside, { passive: true });Also applies to: 192-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/lib/speed-dial/SpeedDial.svelte(3 hunks)src/lib/utils/Popper.svelte(9 hunks)
🔇 Additional comments (4)
src/lib/speed-dial/SpeedDial.svelte (4)
62-62: Improved rotation behavior for touch devices.
Good change! Binding rotation directly to the open state instead of hover improves touch device interaction and helps fix the iOS double-tap issue. The transition class ensures smooth animation.
71-71: Consistent implementation with GradientButton.
Good! The rotation behavior is consistently implemented across both regular and gradient buttons, ensuring uniform interaction patterns.
Line range hint 49-71: Consider additional touch device testing.
The changes look good and align well with fixing the iOS double-tap issue. However, given the concerns raised in PR comments about Popover examples, please ensure comprehensive testing on:
- Different iOS devices and versions
- Various touch interaction scenarios
- Integration with Popover components
Consider adding automated tests for touch interactions to prevent regression:
// Example test structure for future implementation
it('should handle touch interactions correctly', async () => {
// Test single tap opens the speed dial
// Test rotation animation triggers with open state
// Test child button interactions
});49-49: Verify removal of 'group' class impact.
The removal of the hardcoded 'group' class might affect child elements that relied on group-* utility classes for styling. Please ensure this change doesn't unintentionally break any group-based hover or interaction styles.
✅ Verification successful
Removal of 'group' class is safe
The removal of the hardcoded 'group' class from SpeedDial.svelte is safe. While group-* classes are used in other components, the SpeedDial component and its direct children don't rely on group-based styling. The component uses direct class assignments for styling its button and icon elements, with transitions handled through the rotate-45 class binding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any group-* classes in child components or related files
rg -l 'group-[a-zA-Z0-9:-]+' --type svelte
Length of output: 75
Script:
#!/bin/bash
# Search for any group-* classes in all files, without file type restriction
rg -l 'group-[a-zA-Z0-9:-]+'
# Also check for potential group-related Tailwind classes in the SpeedDial component
rg 'class=' src/lib/speed-dial/SpeedDial.svelte -A 2 -B 2
Length of output: 1817
| ['pointerenter', showHandler, hoverable], | ||
| ['pointerleave', hideHandler, hoverable] |
There was a problem hiding this comment.
💡 Codebase verification
Found inconsistency in mouse/pointer event handling
The verification revealed that while Popper.svelte has been updated to use pointer events in some places, it still contains mixed usage of mouse events in the Frame component at line 184:
on:mouseenter={optional(activeContent && hoverable, showHandler)} on:mouseleave={optional(activeContent && hoverable, hideHandler)}This should be updated to use pointer events for consistency with the other changes:
src/lib/utils/Frame.svelte: Updatemouseenter/mouseleavetopointerenter/pointerleave
🔗 Analysis chain
LGTM! The switch to pointer events should fix the iOS Safari issue.
Using pointer events instead of mouse events is the correct approach to handle both touch and mouse interactions consistently.
Let's verify that all mouse events have been replaced with pointer events:
Also applies to: 166-166, 184-184
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining mouse event listeners that might need updating
# Test: Search for mouseenter/mouseleave events that might have been missed
echo "Checking for remaining mouse events..."
rg -i 'mouse(enter|leave)' --type svelte
Length of output: 160
Script:
#!/bin/bash
# Let's try a more general search without file type restriction
echo "Checking for remaining mouse events in all files..."
rg -i 'mouse(enter|leave)'
# Also search for any event listeners to get a comprehensive view
echo -e "\nChecking all event listeners in the Popper component..."
rg -A 2 "addEventListener|removeEventListener" src/lib/utils/Popper.svelte
Length of output: 15736
Due to an Safari Issue the first "mouseenter" event will block the "click" event. This can be worked around by using the "pointerenter" instead.
Closes #1435
📑 Description
As described here instead of
"mouseenter" the event "pointerenter" should be used to ensure correct behaviour on iOS-
Status
✅ Checks
ℹ Additional Information
nothing
Summary by CodeRabbit
closeOnTouchDelayproperty for the popper, allowing users to define how long the popper remains open on touch devices.