-
Notifications
You must be signed in to change notification settings - Fork 225
fix(card): replaced the time-based click detection with a distance-based approach #5521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
04058ea
bed3485
4da7d74
96feba0
13aa6f6
b8fe72c
64e3036
42ca6a0
a4932f5
40a92a7
0327f79
c128578
55003c2
800e61c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@spectrum-web-components/card': minor | ||
--- | ||
|
||
**Fixed**: On mobile Chrome (both Android and iOS), scrolling on `sp-card` components would inadvertently trigger click events. This was caused by the timing-based click detection (200ms threshold) in the pointer event handling, which could misinterpret quick scrolls as clicks. This issue did not affect Safari on mobile devices. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,21 +186,46 @@ export class Card extends LikeAnchor( | |
} | ||
} | ||
|
||
private handlePointerdown(event: Event): void { | ||
/** | ||
* Handles pointer down events on the card element. | ||
* Implements a click detection system that distinguishes between clicks and drags | ||
* based on duration and movement distance. | ||
*/ | ||
private handlePointerdown(event: PointerEvent): void { | ||
const path = event.composedPath(); | ||
const hasAnchor = path.some( | ||
(el) => (el as HTMLElement).localName === 'a' | ||
); | ||
if (hasAnchor) return; | ||
const start = +new Date(); | ||
const handleEnd = (): void => { | ||
const end = +new Date(); | ||
if (end - start < 200) { | ||
// Record the time and initial position of the pointerdown event | ||
const startTime = event.timeStamp; | ||
const startX = event.clientX; | ||
const startY = event.clientY; | ||
Comment on lines
+202
to
+203
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have (or need) any error handling for if the event object is undefined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These events are bound natively without any argument, so the browser always passes an |
||
|
||
// Define the handler for when the pointer interaction ends | ||
const handleEnd = (endEvent: PointerEvent): void => { | ||
const endTime = event.timeStamp; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const endX = endEvent.clientX; | ||
const endY = endEvent.clientY; | ||
|
||
// Calculate time duration and movement distance of the pointer | ||
const timeDelta = endTime - startTime; | ||
const moveX = Math.abs(endX - startX); | ||
const moveY = Math.abs(endY - startY); | ||
|
||
// Consider the pointer interaction a "click" only if: | ||
// - It was short (under 200ms) | ||
// - It didn't move significantly (less than 10px in any direction) | ||
const moved = moveX > 10 || moveY > 10; | ||
|
||
if (timeDelta < 200 && !moved) { | ||
this.click(); | ||
} | ||
|
||
this.removeEventListener('pointerup', handleEnd); | ||
this.removeEventListener('pointercancel', handleEnd); | ||
}; | ||
|
||
this.addEventListener('pointerup', handleEnd); | ||
this.addEventListener('pointercancel', handleEnd); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import '@spectrum-web-components/menu/sp-menu.js'; | |
import '@spectrum-web-components/menu/sp-menu-item.js'; | ||
import '@spectrum-web-components/menu/sp-menu-divider.js'; | ||
import '@spectrum-web-components/link/sp-link.js'; | ||
import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; | ||
|
||
export default { | ||
component: 'sp-card', | ||
|
@@ -342,7 +343,7 @@ export const smallQuiet = (args: StoryArgs): TemplateResult => { | |
return html` | ||
<div> | ||
<sp-card | ||
size=${args.size} | ||
size=${ifDefined(args.size)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the default size medium or is default considered There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SizedMixin is configured with noDefaultSize: true, which means there is no default size, it's considered :not([size]) when no size is specified |
||
?horizontal=${args.horizontal} | ||
heading="Card Heading" | ||
subheading="JPG" | ||
|
@@ -415,3 +416,25 @@ export const SlottedHeading = (args: StoryArgs): TemplateResult => { | |
</sp-card> | ||
`; | ||
}; | ||
|
||
export const ScrollTest = { | ||
render: () => html` | ||
<div class="scroll-container"> | ||
<div class="scroll-indicator"> | ||
<h3>Switch to mobile view to test touch behavior.</h3> | ||
<p> | ||
In mobile view, verify that touch events work correctly and | ||
scrolling doesn't trigger unwanted clicks. | ||
</p> | ||
</div> | ||
${Array.from( | ||
{ length: 20 }, | ||
(_) => html` | ||
<div style="margin: 10px;"> | ||
${horizontalWithHREF({ horizontal: false })} | ||
</div> | ||
` | ||
)} | ||
</div> | ||
`, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the addition of these comments!