fix(card): replaced the time-based click detection with a distance-based approach #5521

Rajdeepc · 2025-06-06T07:20:42Z

Description

Replaced the time-based click detection with a distance-based approach:

Added tracking of initial touch coordinates (X and Y)
Introduced a 10px movement threshold to distinguish between clicks and scrolls
Added pointermove event handling to detect actual scrolling
Improved event listener cleanup

Motivation and context

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.

Root Cause

The pointerdown handler is capturing all touch starts, and then manually measuring the duration of the interaction. If the duration is less than 200ms, we are triggering a .click() on the card:

const start = +new Date();
const handleEnd = (): void => {
    const end = +new Date();
    if (end - start < 200) {
        this.click();
    }

The problem is: mobile Chrome often doesn't distinguish well between taps and touch scrolls unless there's actual movement. So if the user slightly touches and scrolls (even without crossing a threshold), it registers as a tap and triggers .click().

Technical Details

Changed handlePointerdown to use PointerEvent instead of generic Event
Added startX, startY, and isScrolling properties to track touch state
Introduced SCROLL_THRESHOLD constant (10px) for movement detection
Added proper cleanup of all event listeners
Maintained existing behavior for anchor elements and desktop browsers

Related issue(s)

Screenshots (if appropriate)

scrollcard.mov

Author's checklist

I have read the CONTRIBUTING and PULL_REQUESTS documents.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
I have added automated tests to cover my changes.
I have included a well-written changeset if my change needs to be published.
I have included updated documentation if my change required it.

Reviewer's checklist

Includes a Github Issue with appropriate flag or Jira ticket number without a link
Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
Automated tests cover all use cases and follow best practices for writing
Validated on all supported browsers
All VRTs are approved before the author can update Golden Hash

Manual review test cases

Scroll Issue test in Chrome in Android
1. Go here
2. Open Chrome in Android
3. Change to Mobile View in an actual simulator or device
4. Scroll through the card list
5. Check the click event doesn't fire anymore while scrolling
Scroll Issue test in Chrome in iOS
1. Go here
2. Open Chrome in iOS
3. Change to Mobile View in an actual simulator or device
4. Scroll through the card list
5. Check the click event doesn't fire anymore while scrolling

Device review

Did it pass in Desktop?
Did it pass in (emulated) Mobile?
Did it pass in (emulated) iPad?

changeset-bot · 2025-06-06T07:20:45Z

🦋 Changeset detected

Latest commit: 800e61c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 84 packages

Name	Type
@spectrum-web-components/card	Minor
@spectrum-web-components/bundle	Minor
documentation	Patch
@spectrum-web-components/eslint-plugin	Minor
@spectrum-web-components/accordion	Minor
@spectrum-web-components/action-bar	Minor
@spectrum-web-components/action-button	Minor
@spectrum-web-components/action-group	Minor
@spectrum-web-components/action-menu	Minor
@spectrum-web-components/alert-banner	Minor
@spectrum-web-components/alert-dialog	Minor
@spectrum-web-components/asset	Minor
@spectrum-web-components/avatar	Minor
@spectrum-web-components/badge	Minor
@spectrum-web-components/breadcrumbs	Minor
@spectrum-web-components/button-group	Minor
@spectrum-web-components/button	Minor
@spectrum-web-components/checkbox	Minor
@spectrum-web-components/clear-button	Minor
@spectrum-web-components/close-button	Minor
@spectrum-web-components/coachmark	Minor
@spectrum-web-components/color-area	Minor
@spectrum-web-components/color-field	Minor
@spectrum-web-components/color-handle	Minor
@spectrum-web-components/color-loupe	Minor
@spectrum-web-components/color-slider	Minor
@spectrum-web-components/color-wheel	Minor
@spectrum-web-components/combobox	Minor
@spectrum-web-components/contextual-help	Minor
@spectrum-web-components/dialog	Minor
@spectrum-web-components/divider	Minor
@spectrum-web-components/dropzone	Minor
@spectrum-web-components/field-group	Minor
@spectrum-web-components/field-label	Minor
@spectrum-web-components/help-text	Minor
@spectrum-web-components/icon	Minor
@spectrum-web-components/icons-ui	Minor
@spectrum-web-components/icons-workflow	Minor
@spectrum-web-components/icons	Minor
@spectrum-web-components/iconset	Minor
@spectrum-web-components/illustrated-message	Minor
@spectrum-web-components/infield-button	Minor
@spectrum-web-components/link	Minor
@spectrum-web-components/menu	Minor
@spectrum-web-components/meter	Minor
@spectrum-web-components/modal	Minor
@spectrum-web-components/number-field	Minor
@spectrum-web-components/overlay	Minor
@spectrum-web-components/picker-button	Minor
@spectrum-web-components/picker	Minor
@spectrum-web-components/popover	Minor
@spectrum-web-components/progress-bar	Minor
@spectrum-web-components/progress-circle	Minor
@spectrum-web-components/radio	Minor
@spectrum-web-components/search	Minor
@spectrum-web-components/sidenav	Minor
@spectrum-web-components/slider	Minor
@spectrum-web-components/split-view	Minor
@spectrum-web-components/status-light	Minor
@spectrum-web-components/swatch	Minor
@spectrum-web-components/switch	Minor
@spectrum-web-components/table	Minor
@spectrum-web-components/tabs	Minor
@spectrum-web-components/tags	Minor
@spectrum-web-components/textfield	Minor
@spectrum-web-components/thumbnail	Minor
@spectrum-web-components/toast	Minor
@spectrum-web-components/tooltip	Minor
@spectrum-web-components/top-nav	Minor
@spectrum-web-components/tray	Minor
@spectrum-web-components/underlay	Minor
@spectrum-web-components/custom-vars-viewer	Minor
@spectrum-web-components/story-decorator	Minor
@spectrum-web-components/vrt-compare	Minor
@spectrum-web-components/base	Minor
@spectrum-web-components/grid	Minor
@spectrum-web-components/opacity-checkerboard	Minor
@spectrum-web-components/reactive-controllers	Minor
@spectrum-web-components/shared	Minor
@spectrum-web-components/styles	Minor
@spectrum-web-components/theme	Minor
@spectrum-web-components/truncated	Minor
example-project-rollup	Patch
example-project-webpack	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-06-06T07:22:56Z

Branch preview

Review the following VRT differences

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

github-actions · 2025-06-06T07:29:55Z

Tachometer results

Chrome

card permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	569 kB	36.93ms - 38.18ms	-	unsure 🔍 -3% - +2% -0.98ms - +0.84ms
branch	498 kB	36.96ms - 38.29ms	unsure 🔍 -2% - +3% -0.84ms - +0.98ms	-

Firefox

card permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	569 kB	71.34ms - 72.14ms	-	unsure 🔍 -1% - +1% -0.73ms - +0.49ms
branch	498 kB	71.39ms - 72.33ms	unsure 🔍 -1% - +1% -0.49ms - +0.73ms	-

castastrophe

No blockers but a few questions and suggestions added.

castastrophe · 2025-06-10T16:10:53Z

packages/card/src/Card.ts

@@ -186,21 +186,46 @@ export class Card extends LikeAnchor(
        }
    }

-    private handlePointerdown(event: Event): void {
+    /**
+     * Handles pointer down events on the card element.


Love the addition of these comments!

castastrophe · 2025-06-10T16:18:24Z

packages/card/src/Card.ts

-            const end = +new Date();
-            if (end - start < 200) {
+        // Record the time and initial position of the pointerdown event
+        const startTime = +new Date();


Wow! No idea what +new Date() did! TIL! That said, since it's a bit of a hack since JS is loosely typed, should we replace this with Number(new Date()) and/or perhaps a comment that this is converting the date to a numeric string?

+1 on TIL! 😄 Even though I like it, I think the most idiomatic approach here is to rely on event.timeStamp for measuring the delta between the two events.

Going with the appreciated choice of the people.

castastrophe · 2025-06-10T16:19:00Z

packages/card/src/Card.ts

+        const startX = event.clientX;
+        const startY = event.clientY;


Do we have (or need) any error handling for if the event object is undefined?

These events are bound natively without any argument, so the browser always passes an Event object as the first argument. But I agree it wouldn’t hurt to wrap this logic in a try/finally block (with the event listener removals in the finally). That way, if any line ever throws (for any reason?), at least we don't throw and still clean up the dangling handlers.

castastrophe · 2025-06-10T16:19:36Z

packages/card/stories/card.stories.ts

@@ -342,7 +343,7 @@ export const smallQuiet = (args: StoryArgs): TemplateResult => {
    return html`
        <div>
            <sp-card
-                size=${args.size}
+                size=${ifDefined(args.size)}


Is the default size medium or is default considered :not([size])?

SizedMixin is configured with noDefaultSize: true, which means there is no default size, it's considered :not([size]) when no size is specified

castastrophe · 2025-06-10T16:20:29Z

packages/card/test/card.test.ts

+                        <sp-menu-item>Deselect</sp-menu-item>
+                        <sp-menu-item>Select Inverse</sp-menu-item>
+                        <sp-menu-item>Feather...</sp-menu-item>
+                        <sp-menu-item>Select and Mask...</sp-menu-item>
+                        <sp-menu-divider></sp-menu-divider>
+                        <sp-menu-item>Save Selection</sp-menu-item>
+                        <sp-menu-item disabled>Make Work Path</sp-menu-item>


Suggested change

<sp-menu-item>Deselect</sp-menu-item>

<sp-menu-item>Select Inverse</sp-menu-item>

<sp-menu-item>Feather...</sp-menu-item>

<sp-menu-item>Select and Mask...</sp-menu-item>

<sp-menu-divider></sp-menu-divider>

<sp-menu-item>Save Selection</sp-menu-item>

<sp-menu-item disabled>Make Work Path</sp-menu-item>

<sp-menu-item>Deselect</sp-menu-item>

<sp-menu-item>Select inverse</sp-menu-item>

<sp-menu-item>Feather...</sp-menu-item>

<sp-menu-item>Select and mask...</sp-menu-item>

<sp-menu-divider></sp-menu-divider>

<sp-menu-item>Save selection</sp-menu-item>

<sp-menu-item disabled>Make work path</sp-menu-item>

I know this is just an indent update but should we take the opportunity to fix the sentence case here anyway?

Sure will take this as a part of the clean up!

castastrophe · 2025-06-10T16:21:03Z

packages/card/test/card.test.ts

+            <sp-card variant="gallery" heading="Card Heading" subheading="JPG">
+                <img
+                    slot="preview"
+                    src="https://picsum.photos/532/192"


A future suggestion, I've seen CI issues with using API services to render images, I think we should consider having a few local assets for testing.

Will add this in a follow up PR. Not a part of this change

rubencarvalho · 2025-06-11T07:33:13Z

packages/card/src/Card.ts

        const startX = event.clientX;
        const startY = event.clientY;

        // Define the handler for when the pointer interaction ends
        const handleEnd = (endEvent: PointerEvent): void => {
-            const endTime = +new Date();
+            const endTime = event.timeStamp;


endEvent.timeStamp

Rajdeep Chandra added 2 commits June 6, 2025 12:47

fix(card): scroll in card in mobile chrome adds click event

04058ea

chore: changeset added

bed3485

Rajdeepc added bug Something isn't working Priority labels Jun 6, 2025

Rajdeepc changed the title ~~Rajdeep/card chrome mobile bug~~ fix(card): replaced the time-based click detection with a distance-based approach Jun 6, 2025

Rajdeepc added the Component: Card label Jun 6, 2025

Rajdeep Chandra added 3 commits June 9, 2025 13:22

fix: added test for scrolling in card

4da7d74

fix: added story to test

96feba0

fix: added story to test

13aa6f6

Rajdeepc marked this pull request as ready for review June 9, 2025 09:13

Rajdeepc requested a review from a team as a code owner June 9, 2025 09:13

Rajdeepc and others added 3 commits June 9, 2025 14:43

Merge branch 'main' into rajdeep/card-chrome-mobile-bug

b8fe72c

chore: added story for scrolling touch issue fix in mobile devices

64e3036

chore: added code comments

42ca6a0

castastrophe approved these changes Jun 10, 2025

View reviewed changes

chore: removed +new Date and added event timeStamp

a4932f5

rubencarvalho reviewed Jun 11, 2025

View reviewed changes

Rajdeep Chandra and others added 5 commits June 11, 2025 13:04

chore: endEvent typo fix

40a92a7

chore: added new car scroll story

0327f79

chore: story fix

c128578

ci: updated golden image cache

55003c2

Merge branch 'main' into rajdeep/card-chrome-mobile-bug

800e61c

Rajdeepc requested a review from rubencarvalho June 11, 2025 11:44

rubencarvalho approved these changes Jun 11, 2025

View reviewed changes

Rajdeepc merged commit 56f2ff4 into main Jun 11, 2025
23 checks passed

Rajdeepc deleted the rajdeep/card-chrome-mobile-bug branch June 11, 2025 11:52

fix(card): replaced the time-based click detection with a distance-based approach #5521

fix(card): replaced the time-based click detection with a distance-based approach #5521

Uh oh!

Conversation

Rajdeepc commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

Root Cause

Technical Details

Related issue(s)

Screenshots (if appropriate)

Author's checklist

Reviewer's checklist

Manual review test cases

Device review

Uh oh!

changeset-bot bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jun 6, 2025

Branch preview

Review the following VRT differences

Uh oh!

github-actions bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tachometer results

card permalink

test-basic

card permalink

test-basic

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubencarvalho Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Rajdeepc commented Jun 6, 2025 •

edited

Loading

changeset-bot bot commented Jun 6, 2025 •

edited

Loading

github-actions bot commented Jun 6, 2025 •

edited

Loading

rubencarvalho Jun 10, 2025 •

edited

Loading

Rajdeepc Jun 11, 2025 •

edited

Loading