Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(action-menu, combobox, dropdown, popover, tooltip): use click instead of pointerdown for click contexts #8943

Merged
merged 1 commit into from
Mar 21, 2024
Merged

fix(action-menu, combobox, dropdown, popover, tooltip): use click instead of pointerdown for click contexts #8943

merged 1 commit into from
Mar 21, 2024

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Mar 14, 2024 •
edited
Loading

Related Issue: #7732

Summary

This updates event handling to use click vs pointerdown + checking the pointer type wherever clicks are expected. Both events have different semantics and trigger order. Simplified code is also an added bonus.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Mar 14, 2024
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 15, 2024
@jcfranco jcfranco marked this pull request as ready for review March 21, 2024 21:30
@jcfranco jcfranco requested a review from a team as a code owner March 21, 2024 21:30
driskull
driskull approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

click click 💥 🖱️ 👍

driskull
driskull reviewed Mar 21, 2024
View reviewed changes
packages/calcite-components/src/components/tooltip/TooltipManager.ts
return;
}

private clickHandler = (event: PointerEvent): void => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change to MouseEvent for consistency?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May apply to other places. Doesn't really matter much since PointerEvent extends MouseEvent.

I also notice some other places using onPointerUp/onPointerDown or using PointerEvent typing for click events.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we search and eliminate the util function isPrimaryPointerButton?

Copy link
Member Author

@jcfranco jcfranco Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change to MouseEvent for consistency?

I think for our purposes, we can use PointerEvent as it matches the latest spec changes. Worth noting that Firefox and Safari haven't updated yet (see web platform tests).

I also notice some other places using onPointerUp/onPointerDown or using PointerEvent typing for click events.

Do you recall where? I may have missed some, but I updated the places where pointerdown events were being treated as a click. I think isPrimaryPointerButton is still useful. E.g., color-picker's color field and sliders depend on the primary pointer's up and down events.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you spot the ones I missed, I can update them in a follow-up PR.

@jcfranco jcfranco merged commit cd7eed9 into main Mar 21, 2024
14 checks passed
@jcfranco jcfranco deleted the jcfranco/7732-use-click-vs-pointerdown-for-click-contexts branch March 21, 2024 23:04
@github-actions github-actions bot added this to the 2024-03-26 - Mar Release milestone Mar 21, 2024
@calcite-admin calcite-admin mentioned this pull request Mar 21, 2024
Merged
geospatialem pushed a commit that referenced this pull request Mar 27, 2024
@calcite-admin @github-actions
chore: release main (#8842)
2326ada 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-design-tokens: 2.1.2</summary>

##
[2.1.2](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-design-tokens@2.1.1...@esri/calcite-design-tokens@2.1.2)
(2024-03-26)


### Bug Fixes

* Fix invalid font stacks
([#8964](#8964))
([d55186a](d55186a))
</details>

<details><summary>@esri/calcite-components: 2.7.0</summary>

##
[2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.6.0...@esri/calcite-components@2.7.0)
(2024-03-26)


### Features

* **button:** Add download property
([#8882](#8882))
([fc55dde](fc55dde))
* **color-picker:** Add `clearable` prop and deprecate `allowEmpty`
([#8910](#8910))
([f036ac2](f036ac2))
* **table-row:** Add alignment property
([#8961](#8961))
([1f81fd7](1f81fd7))
* **tabs:** Make component responsive
([#8616](#8616))
([83a2ef4](83a2ef4))
* **tile:** Add content-top and content-bottom slots, deprecate
content-start and content-end slots
([#8984](#8984))
([eb000d8](eb000d8))


### Bug Fixes

* **action-menu, combobox, dropdown, popover, tooltip:** Use click
instead of pointerdown for click contexts
([#8943](#8943))
([cd7eed9](cd7eed9))
* **card:** Do not apply text color to slotted footer items
([#8839](#8839))
([30a2068](30a2068))
* **combobox:** Prevent spacebar from opening the menu when focused on
chip's close button
([#8990](#8990))
([1a20d0e](1a20d0e))
* **dropdown:** Correct positioning behavior when inside a scrollable
container
([#8973](#8973))
([2524391](2524391))
* **input-time-picker:** Update toggle icon color
([#8955](#8955))
([ce3ac5c](ce3ac5c))
* **input, input-number, input-text:** Ensure values are initialized
properly for dist and components output targets
([#8997](#8997))
([9152211](9152211))
* **list, list-item:** Calcite-select becomes unresponsive in a
list-item if drag-disabled is true
([#8957](#8957))
([e408c62](e408c62))
* **list:** Fix filter box when scrolling in Safari
([#8938](#8938))
([ea8ea1e](ea8ea1e))
* **popover:** Prevent disabled reference elements from toggling popover
([#8983](#8983))
([9f4b14b](9f4b14b))
* **stepper:** Fix layout when switching modes dynamically to
`horizontal-single`
([#8946](#8946))
([01f58bf](01f58bf))
* **table:** Prevent duplicate scrollbars in certain browsers
([#8962](#8962))
([8434eeb](8434eeb))
* **tab:** Style focus outline of tab content
([#8804](#8804))
([8f0133f](8f0133f))


### Performance Improvements

* Update `transition-default` Tailwind util to only target common,
animatable properties
([#8797](#8797))
([f4d016b](f4d016b))


### Reverts

* Refactor(popover): update token pattern
([#8889](#8889))
([c43c280](c43c280))


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @esri/calcite-design-tokens bumped from ^2.1.2-next.2 to ^2.1.2
</details>

<details><summary>@esri/calcite-components-angular: 2.7.0</summary>

##
[2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.6.0...@esri/calcite-components-angular@2.7.0)
(2024-03-26)


### Miscellaneous Chores

* **@esri/calcite-components-angular:** Synchronize components versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.7.0-next.17 to ^2.7.0
</details>

<details><summary>@esri/calcite-components-react: 2.7.0</summary>

##
[2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.6.0...@esri/calcite-components-react@2.7.0)
(2024-03-26)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize components versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.7.0-next.17 to ^2.7.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@calcite-admin calcite-admin mentioned this pull request May 14, 2024
Merged
@calcite-admin calcite-admin mentioned this pull request May 28, 2024
Merged
@driskull driskull mentioned this pull request Jun 7, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

Assignees

@jcfranco jcfranco

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2024-03-26 - Mar Release
Development

Successfully merging this pull request may close these issues.
2 participants
@jcfranco @driskull