Removing mouse up and mouse down from modal `Backdrop` component #8090

emmanueletti · 2023-01-18T23:21:29Z

Spin instance

WHY are these changes introduced?

Closes Modal closes when mouse released out of bounds #8144
Modal component closes when a user selects within the modal (for example, in an input field while selecting typed input) but overshoots the modal boundaries and releases their mouse outside the modal (in the backdrop).

WHAT is this pull request doing?

Removes the onMouseUp and onMouseDown handles from Backdrop component. There is a test that covers this case of explicitly clicking the backdrop to close it and we think that is sufficient behaviour.

Before

BEFORE-export.mov

After

AFTER-export.mov

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility

github-actions · 2023-01-18T23:46:06Z

size-limit report 📦

Path	Size
polaris-react-cjs	215.23 KB (-0.01% 🔽)
polaris-react-esm	137.12 KB (+0.01% 🔺)
polaris-react-esnext	192.63 KB (-0.01% 🔽)
polaris-react-css	42 KB (0%)

.changeset/gentle-chefs-hear.md

danielle-dsouza

Code looks good and bug seems to be fixed!

Most of the code being removed was introduced in #6986 to add an animation to the x close button when the backdrop is clicked. Does this mean that animation is no longer going to be seen? 🤔

If so - we should probably remove the rest of the code associated with that PR and reopen the original issue https://github.com/Shopify/shopify/issues/361647.

Co-authored-by: Jason Kurian <jason.kurian@shopify.com>

emmanueletti · 2023-01-23T18:03:50Z

@danielle-dsouza I pushed another commit that preserves the closing icon animation (with an updated screen recording)

@JaKXz bringing back the animation to the "X" icon meant also bringing the props to Backdrop. With the limited change now, this seems like a patch

JaKXz

LGTM once the checklist is ✅ ed -- the code running in storybook looks 💯 to me

danielle-dsouza

LGTM!

kyledurand

👍 Thanks @emmanueletti

.changeset/gentle-chefs-hear.md

Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>

@leileu

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris-icons@6.9.0 ### Minor Changes - [#8139](#8139) [`b998ca007`](b998ca0) Thanks [@leileu](https://github.com/leileu)! - Adding content minor icon for left nav in admin - [#8094](#8094) [`94988bc26`](94988bc) Thanks [@Tamas-Kisss](https://github.com/Tamas-Kisss)! - Added major and minor icon for Papercheck - [#8121](#8121) [`f74e8ffcc`](f74e8ff) Thanks [@zecarlostorre](https://github.com/zecarlostorre)! - Added EnterMajor icon ## @shopify/polaris@10.23.0 ### Minor Changes - [#8134](#8134) [`8d80691b5`](8d80691) Thanks [@mrcthms](https://github.com/mrcthms)! - Removed the focus ring from `Listbox` options ### Patch Changes - [#8093](#8093) [`60dd5a0c5`](60dd5a0) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Added `borderRadius` style to `TooltipOverlay` - [#8090](#8090) [`bdcc291a4`](bdcc291) Thanks [@emmanueletti](https://github.com/emmanueletti)! - Replaced mouse up and down events on Backdrop with onClick to close Modal - [#8131](#8131) [`6096c3492`](6096c34) Thanks [@henryyi](https://github.com/henryyi)! - Fixed Navigation item secondaryActions alignment in mobile when floating actions are enabled - [#8114](#8114) [`e6aa9c801`](e6aa9c8) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Dismiss index table tooltip on mouse out - [#8091](#8091) [`23ee70d13`](23ee70d) Thanks [@ginabak](https://github.com/ginabak)! - Added `onBlur` prop to numerical steppers (`Spinner` component of `TextField`) to remove multi focus issue in `TextField`. - Updated dependencies \[[`b998ca007`](b998ca0), [`94988bc26`](94988bc), [`f74e8ffcc`](f74e8ff)]: - @shopify/polaris-icons@6.9.0 ## @shopify/plugin-polaris@0.0.30 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.11.1 ## @shopify/polaris-migrator@0.11.1 ### Patch Changes - Updated dependencies \[[`cd150396b`](cd15039)]: - @shopify/stylelint-polaris@5.1.1 ## @shopify/stylelint-polaris@5.1.1 ### Patch Changes - [#8097](#8097) [`cd150396b`](cd15039) Thanks [@qt314](https://github.com/qt314)! - Fix incorrect unit function categorization ## polaris.shopify.com@0.30.0 ### Minor Changes - [#8110](#8110) [`5db7778e4`](5db7778) Thanks [@yurm04](https://github.com/yurm04)! - Added New badge pattern guidance for the primary nav ### Patch Changes - [#8107](#8107) [`fc30bbd32`](fc30bbd) Thanks [@Rmnlly](https://github.com/Rmnlly)! - Adding examples for truncateText and multiple secondary actions and updating props on the documentation site - Updated dependencies \[[`b998ca007`](b998ca0), [`60dd5a0c5`](60dd5a0), [`bdcc291a4`](bdcc291), [`6096c3492`](6096c34), [`94988bc26`](94988bc), [`e6aa9c801`](e6aa9c8), [`8d80691b5`](8d80691), [`f74e8ffcc`](f74e8ff), [`23ee70d13`](23ee70d)]: - @shopify/polaris-icons@6.9.0 - @shopify/polaris@10.23.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…8237)  ### WHY are these changes introduced? I believe #8090 introduced a regression where the `onClick` prop of the `Backdrop` component is not called when the consumer of the component also doesn't provide `setClosing` as a prop (released in Polaris `10.23.0`). There are various occurrences of Web Admin using `Backdrop` without providing `setClosing` as a prop, so some sheets are not properly closing when clicking outside. For example in Customer Segmentation: <img width="1428" alt="Screenshot 2023-02-01 at 1 40 45 PM" src="https://user-images.githubusercontent.com/3925905/216133586-92b760bf-57c2-4bfd-9f46-3382d8f935c2.png"> You can reproduce the issue in this [CodeSandbox](https://codesandbox.io/s/condescending-cannon-7om9z7?file=/App.js:115-155). Switching to a version of Polaris < `10.23.0` fixes the issue. ### WHAT is this pull request doing? This PR ensures `onClick` is always called, regardless of `setClosing` being provided. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)  See Playground code. After opening the backdrop, clicking on it again should properly close it. <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React, {useState} from 'react'; import {Backdrop, Button, Page} from '../src'; export function Playground() { const [open, setOpen] = useState(false); return ( <Page title="Playground"> <div style={{ position: "absolute", zIndex: 999999 }}> <Button onClick={() => setOpen(!open)}>Toggle backdrop</Button> </div> {open && ( <Backdrop setClosing={() => {}} onClick={() => { console.log("onClick from Backdrop"); setOpen(false); }} /> )} </Page> ); } ``` </details> You can also 🎩 directly in Admin using this [Spin instance](https://admin.web.customer-data-platform-ujet.loic-delaubier.us.spin.dev/store/shop1/customers): - Go to the Customers section - Open Segmentation templates <img width="1189" alt="Screenshot 2023-02-01 at 2 18 18 PM" src="https://user-images.githubusercontent.com/3925905/216141402-c395b9df-8990-4315-84ca-c88607b759fd.png"> - Click outside of the sheet. The sheet should properly close. ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

…pify#8090) [Spin instance](https://polaris.polaris-f45t.emmanuel-etti.us.spin.dev/) ### WHY are these changes introduced? - Closes Shopify#8144 - Modal component closes when a user selects within the modal (for example, in an input field while selecting typed input) but overshoots the modal boundaries and releases their mouse outside the modal (in the backdrop). ### WHAT is this pull request doing? - Removes the `onMouseUp` and `onMouseDown` handles from `Backdrop` component. There is a test that covers this case of explicitly clicking the backdrop to close it and we think that is sufficient behaviour. ### Before https://user-images.githubusercontent.com/78582921/213541746-f3304f21-737f-411b-aea1-a8cc35264e57.mov ### After https://user-images.githubusercontent.com/78582921/214117861-d8866a19-b45d-4d08-a94f-cd467199a3b5.mov ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) Co-authored-by: Jason Kurian <jason.kurian@shopify.com> Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>

@leileu

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris-icons@6.9.0 ### Minor Changes - [Shopify#8139](Shopify#8139) [`b998ca007`](Shopify@b998ca0) Thanks [@leileu](https://github.com/leileu)! - Adding content minor icon for left nav in admin - [Shopify#8094](Shopify#8094) [`94988bc26`](Shopify@94988bc) Thanks [@Tamas-Kisss](https://github.com/Tamas-Kisss)! - Added major and minor icon for Papercheck - [Shopify#8121](Shopify#8121) [`f74e8ffcc`](Shopify@f74e8ff) Thanks [@zecarlostorre](https://github.com/zecarlostorre)! - Added EnterMajor icon ## @shopify/polaris@10.23.0 ### Minor Changes - [Shopify#8134](Shopify#8134) [`8d80691b5`](Shopify@8d80691) Thanks [@mrcthms](https://github.com/mrcthms)! - Removed the focus ring from `Listbox` options ### Patch Changes - [Shopify#8093](Shopify#8093) [`60dd5a0c5`](Shopify@60dd5a0) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Added `borderRadius` style to `TooltipOverlay` - [Shopify#8090](Shopify#8090) [`bdcc291a4`](Shopify@bdcc291) Thanks [@emmanueletti](https://github.com/emmanueletti)! - Replaced mouse up and down events on Backdrop with onClick to close Modal - [Shopify#8131](Shopify#8131) [`6096c3492`](Shopify@6096c34) Thanks [@henryyi](https://github.com/henryyi)! - Fixed Navigation item secondaryActions alignment in mobile when floating actions are enabled - [Shopify#8114](Shopify#8114) [`e6aa9c801`](Shopify@e6aa9c8) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Dismiss index table tooltip on mouse out - [Shopify#8091](Shopify#8091) [`23ee70d13`](Shopify@23ee70d) Thanks [@ginabak](https://github.com/ginabak)! - Added `onBlur` prop to numerical steppers (`Spinner` component of `TextField`) to remove multi focus issue in `TextField`. - Updated dependencies \[[`b998ca007`](Shopify@b998ca0), [`94988bc26`](Shopify@94988bc), [`f74e8ffcc`](Shopify@f74e8ff)]: - @shopify/polaris-icons@6.9.0 ## @shopify/plugin-polaris@0.0.30 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.11.1 ## @shopify/polaris-migrator@0.11.1 ### Patch Changes - Updated dependencies \[[`cd150396b`](Shopify@cd15039)]: - @shopify/stylelint-polaris@5.1.1 ## @shopify/stylelint-polaris@5.1.1 ### Patch Changes - [Shopify#8097](Shopify#8097) [`cd150396b`](Shopify@cd15039) Thanks [@qt314](https://github.com/qt314)! - Fix incorrect unit function categorization ## polaris.shopify.com@0.30.0 ### Minor Changes - [Shopify#8110](Shopify#8110) [`5db7778e4`](Shopify@5db7778) Thanks [@yurm04](https://github.com/yurm04)! - Added New badge pattern guidance for the primary nav ### Patch Changes - [Shopify#8107](Shopify#8107) [`fc30bbd32`](Shopify@fc30bbd) Thanks [@Rmnlly](https://github.com/Rmnlly)! - Adding examples for truncateText and multiple secondary actions and updating props on the documentation site - Updated dependencies \[[`b998ca007`](Shopify@b998ca0), [`60dd5a0c5`](Shopify@60dd5a0), [`bdcc291a4`](Shopify@bdcc291), [`6096c3492`](Shopify@6096c34), [`94988bc26`](Shopify@94988bc), [`e6aa9c801`](Shopify@e6aa9c8), [`8d80691b5`](Shopify@8d80691), [`f74e8ffcc`](Shopify@f74e8ff), [`23ee70d13`](Shopify@23ee70d)]: - @shopify/polaris-icons@6.9.0 - @shopify/polaris@10.23.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…hopify#8237)  ### WHY are these changes introduced? I believe Shopify#8090 introduced a regression where the `onClick` prop of the `Backdrop` component is not called when the consumer of the component also doesn't provide `setClosing` as a prop (released in Polaris `10.23.0`). There are various occurrences of Web Admin using `Backdrop` without providing `setClosing` as a prop, so some sheets are not properly closing when clicking outside. For example in Customer Segmentation: <img width="1428" alt="Screenshot 2023-02-01 at 1 40 45 PM" src="https://user-images.githubusercontent.com/3925905/216133586-92b760bf-57c2-4bfd-9f46-3382d8f935c2.png"> You can reproduce the issue in this [CodeSandbox](https://codesandbox.io/s/condescending-cannon-7om9z7?file=/App.js:115-155). Switching to a version of Polaris < `10.23.0` fixes the issue. ### WHAT is this pull request doing? This PR ensures `onClick` is always called, regardless of `setClosing` being provided. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)  See Playground code. After opening the backdrop, clicking on it again should properly close it. <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React, {useState} from 'react'; import {Backdrop, Button, Page} from '../src'; export function Playground() { const [open, setOpen] = useState(false); return ( <Page title="Playground"> <div style={{ position: "absolute", zIndex: 999999 }}> <Button onClick={() => setOpen(!open)}>Toggle backdrop</Button> </div> {open && ( <Backdrop setClosing={() => {}} onClick={() => { console.log("onClick from Backdrop"); setOpen(false); }} /> )} </Page> ); } ``` </details> You can also 🎩 directly in Admin using this [Spin instance](https://admin.web.customer-data-platform-ujet.loic-delaubier.us.spin.dev/store/shop1/customers): - Go to the Customers section - Open Segmentation templates <img width="1189" alt="Screenshot 2023-02-01 at 2 18 18 PM" src="https://user-images.githubusercontent.com/3925905/216141402-c395b9df-8990-4315-84ca-c88607b759fd.png"> - Click outside of the sheet. The sheet should properly close. ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

emmanueletti assigned JaKXz and emmanueletti Jan 18, 2023

JaKXz force-pushed the rfc/removing-mouse-up-and-down-from-modal branch from 04940d9 to ab27b15 Compare January 18, 2023 23:32

JaKXz changed the title ~~RFC: Removing mouse up and mouse down from modal~~ RFC: Removing mouse up and mouse down from modal Backdrop component Jan 18, 2023

JaKXz requested a review from kyledurand January 19, 2023 21:11

JaKXz marked this pull request as ready for review January 19, 2023 21:12

emmanueletti requested a review from danielle-dsouza January 19, 2023 21:49

emmanueletti force-pushed the rfc/removing-mouse-up-and-down-from-modal branch from ab27b15 to f83f85a Compare January 19, 2023 22:39

JaKXz reviewed Jan 20, 2023

View reviewed changes

.changeset/gentle-chefs-hear.md Outdated Show resolved Hide resolved

danielle-dsouza reviewed Jan 21, 2023

View reviewed changes

emmanueletti and others added 2 commits January 23, 2023 17:55

Removing mouse up and mouse down from modal

fda21d1

Co-authored-by: Jason Kurian <jason.kurian@shopify.com>

Adding animation to Modal closing icon

5d19dcf

emmanueletti force-pushed the rfc/removing-mouse-up-and-down-from-modal branch from f83f85a to 5d19dcf Compare January 23, 2023 17:56

emmanueletti requested review from JaKXz and danielle-dsouza January 23, 2023 18:03

Adds passing test

a74a21b

emmanueletti changed the title ~~RFC: Removing mouse up and mouse down from modal Backdrop component~~ Removing mouse up and mouse down from modal Backdrop component Jan 23, 2023

Changes contribution from major to patch

6cc4cd5

JaKXz approved these changes Jan 23, 2023

View reviewed changes

danielle-dsouza approved these changes Jan 24, 2023

View reviewed changes

kyledurand approved these changes Jan 24, 2023

View reviewed changes

kyledurand reviewed Jan 24, 2023

View reviewed changes

.changeset/gentle-chefs-hear.md Outdated Show resolved Hide resolved

Update .changeset/gentle-chefs-hear.md

283ee44

Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>

emmanueletti merged commit bdcc291 into main Jan 24, 2023

emmanueletti deleted the rfc/removing-mouse-up-and-down-from-modal branch January 24, 2023 17:00

shopify-github-actions-access bot mentioned this pull request Jan 24, 2023

Version Packages #8103

Merged

cdmo added the #gsd:26204 label Jan 24, 2023

loic-d mentioned this pull request Feb 1, 2023

[Backdrop] Call onClick regardless of setClosing prop presence #8237

Merged

5 tasks

gwyneplaine mentioned this pull request Feb 14, 2023

8248/fix toc layout #8338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removing mouse up and mouse down from modal `Backdrop` component #8090

Removing mouse up and mouse down from modal `Backdrop` component #8090

Uh oh!

emmanueletti commented Jan 18, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Jan 18, 2023 •

edited

Loading

Uh oh!

Uh oh!

danielle-dsouza left a comment

Uh oh!

emmanueletti commented Jan 23, 2023 •

edited

Loading

Uh oh!

JaKXz left a comment

Uh oh!

danielle-dsouza left a comment

Uh oh!

kyledurand left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Removing mouse up and mouse down from modal Backdrop component #8090

Removing mouse up and mouse down from modal Backdrop component #8090

Uh oh!

Conversation

emmanueletti commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Before

After

🎩 checklist

Uh oh!

github-actions bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Uh oh!

danielle-dsouza left a comment

Choose a reason for hiding this comment

Uh oh!

emmanueletti commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaKXz left a comment

Choose a reason for hiding this comment

Uh oh!

danielle-dsouza left a comment

Choose a reason for hiding this comment

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Removing mouse up and mouse down from modal `Backdrop` component #8090

Removing mouse up and mouse down from modal `Backdrop` component #8090

emmanueletti commented Jan 18, 2023 •

edited

Loading

github-actions bot commented Jan 18, 2023 •

edited

Loading

emmanueletti commented Jan 23, 2023 •

edited

Loading