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 event handling with react-beautiful-dnd #5842

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix event handling with react-beautiful-dnd #5842

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
264109d
chore: add test cases
itsdouges Jan 9, 2024
df2d44c
fix: fixes control bailing out of an event if it has had prevent defa…
itsdouges Jan 9, 2024
5e04ec6
chore: add tests covering alternative event flows
itsdouges Jan 9, 2024
67e005f
chore: add changeset
itsdouges Jan 9, 2024
649eec7
chore: fix lock
itsdouges Jan 9, 2024
d3fd7f7
chore: nit
itsdouges Jan 9, 2024
16b4be7
chore: fix tests
itsdouges Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/small-steaks-decide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'react-select': patch
---

Select no longer stops the control mouse down event from firing if someone listening to the event calls `.preventDefault()`. This resolves issues seen with react-select interop-ing with other libraries, such as react-beautiful-dnd.
14 changes: 13 additions & 1 deletion cypress/fixtures/selectors.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,17 @@
"multiSelectInput": "#react-select-multi-select-input",
"placeHolderMulti": "#multi-select .react-select__placeholder",
"toggleMenuMulti": "#multi-select .react-select__dropdown-indicator",
"focusedOption": ".react-select__option--is-focused"
"focusedOption": ".react-select__option--is-focused",
"preventDefaultTest": {
"valueContainer": ".prevent-default-test__value-container",
"indicator": ".prevent-default-test__indicator",
"menu": ".prevent-default-test__menu"
},
"bubblingTest": {
"valueContainer": ".bubbling-test__value-container",
"indicator": ".bubbling-test__indicator",
"menu": ".bubbling-test__menu",
"datePickerInput": "#date-picker",
"datePickerMenu": ".react-datepicker__tab-loop"
}
}
50 changes: 50 additions & 0 deletions cypress/integration/events.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import selector from '../fixtures/selectors.json';
import cypressJson from '../../cypress.json';

describe('event propagation', () => {
before(() => {
cy.visit(cypressJson.baseUrl);
cy.title().should('equal', 'React-Select');
cy.get('h1').should('contain', 'Test Page for Cypress');
});

beforeEach(() => {
cy.reload();
});

it('should open select via value container after a parent prevent defaulted the event', () => {
cy.get(selector.preventDefaultTest.valueContainer)
.click()
.get(selector.preventDefaultTest.menu)
.should('exist');
});

it('should open select via indicator after a parent prevent defaulted the event', () => {
cy.get(selector.preventDefaultTest.indicator)
.click()
.get(selector.preventDefaultTest.menu)
.should('exist');
});

it('should close react calendar when interacting with the select indicator', () => {
cy.get(selector.bubblingTest.datePickerInput)
.click()
.get(selector.bubblingTest.indicator)
.click()
.get(selector.bubblingTest.menu)
.should('exist')
.get(selector.bubblingTest.datePickerMenu)
.should('not.exist');
});

it('should close react calendar when interacting with the select value container', () => {
cy.get(selector.bubblingTest.datePickerInput)
.click()
.get(selector.bubblingTest.valueContainer)
.click()
.get(selector.bubblingTest.menu)
.should('exist')
.get(selector.bubblingTest.datePickerMenu)
.should('not.exist');
});
});
38 changes: 38 additions & 0 deletions docs/Tests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import React, {
} from 'react';

import Select, { MenuPlacement } from 'react-select';
import DatePicker from 'react-datepicker';
import 'react-datepicker/dist/react-datepicker.css';
import { H1, Note } from './styled-components';
import { colourOptions, groupedOptions, optionLength } from './data';

Expand Down Expand Up @@ -235,6 +237,42 @@ export default function Tests() {
options={optionLength}
/>
</div>
<h3>Event handling with bubbling</h3>
<label htmlFor="bubbling-test">Select bubbling</label>
<Select
instanceId="bubbling-test"
inputId="bubbling-test"
classNamePrefix="bubbling-test"
options={[
{ value: 'chocolate', label: 'Chocolate' },
{ value: 'strawberry', label: 'Strawberry' },
{ value: 'vanilla', label: 'Vanilla' },
]}
onChange={console.log}
id="bubbling-select"
/>
<label htmlFor="date-picker">Date picker</label>
<div>
<DatePicker id="date-picker" onChange={() => {}} />
</div>

<h3>Event handling with pevent default</h3>
itsdouges marked this conversation as resolved.
Show resolved Hide resolved
<label htmlFor="prevent-default-test">Select inside capture phase</label>
<div onMouseDownCapture={(e) => e.preventDefault()}>
<Select
instanceId="prevent-default-test"
inputId="prevent-default-test"
options={[
{ value: 'chocolate', label: 'Chocolate' },
{ value: 'strawberry', label: 'Strawberry' },
{ value: 'vanilla', label: 'Vanilla' },
]}
classNamePrefix="prevent-default-test"
onChange={console.log}
id="prevent-default-select"
/>
</div>
<div style={{ height: 500 }} />
</div>
);
}
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@types/node": "^16.11.68",
"@types/pretty-proptypes": "^1.1.0",
"@types/react": "^16.14.2",
"@types/react-datepicker": "^4.19.5",
"@types/react-dom": "^16.9.10",
"@types/react-transition-group": "^4.4.0",
"@typescript-eslint/eslint-plugin": "^4.14.0",
Expand All @@ -57,6 +58,7 @@
"jest": "^25.1.0",
"jest-in-case": "^1.0.2",
"prettier": "^2.2.1",
"react-datepicker": "^4.25.0",
"style-loader": "^0.23.1",
"typescript": "^4.1.3",
"user-agent-data-types": "^0.4.2"
Expand Down
4 changes: 0 additions & 4 deletions packages/react-select/src/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,10 +1293,6 @@ export default class Select<
onControlMouseDown = (
event: React.MouseEvent<HTMLDivElement> | React.TouchEvent<HTMLDivElement>
) => {
// Event captured by dropdown indicator
Copy link

Choose a reason for hiding this comment

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

is there a test for this particular use case - dropdown indicator preventing default event (

react-select/packages/react-select/src/Select.tsx

Line 1345 in 06e3488

event.preventDefault();
)

Also, not quite clear why stopPropagation is not a case, onMenuMouseDown uses it (uses both) for example.

This code was added here for a reason, and I can see other pieces relying on this one. Basically - all .preventDefault in this file.

What if we try to change the game rules and just rely on a different information source?

const createEventTracker = () => {
  const tracker = new WeakSet();
  return {
       mark: (event) => {
        tracker.add(event.target);
        Promise.resolve().then(() => tracker.delete(event.target);
      },
      isMarked: (event) => tracker.has(event.target);
 }
} 

/// ....
const mouseDownTracker = createEventTracker();

onDropdownIndicatorMouseDown = (e) => {
  // ...
  mouseDownTracker.mark(e);
}

onControlMouseDown = (event) => {
    // Event captured by dropdown indicator
    if (mouseDownTracker.isMarked(event)) {
      return;
    }
}

Might be a little too much code to support a single line, but this code will keep the old behavior and unlock our usecase.

Copy link
Author

Choose a reason for hiding this comment

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

This prevent default was added in #5134 — I can't imagine anything in Select is leaning on it?

We can definitely change the rules like you said if the behaviour is needed. I'm just not sure it is.

Copy link
Author

Choose a reason for hiding this comment

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

The failing tests might point to it being needed I suppose ;).

Copy link

Choose a reason for hiding this comment

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

Right, and you added tests for #5050 case, and they are passing. May be something else in between, including changes in DatePicker resolved the problem in the meanwhile

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you're right, which is what the tests are showing now. I'll dig in soon.

if (event.defaultPrevented) {
return;
}
const { openMenuOnClick } = this.props;
if (!this.state.isFocused) {
if (openMenuOnClick) {
Expand Down
64 changes: 64 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,13 @@
dependencies:
regenerator-runtime "^0.13.4"

"@babel/runtime@^7.21.0":
version "7.23.8"
resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.23.8.tgz#8ee6fe1ac47add7122902f257b8ddf55c898f650"
integrity sha512-Y7KbAP984rn1VGMbGqKmBLio9V7y5Je9GvU4rQPCPinCyNfUcToxIXl06d59URp/F3LwinvODxab5N/G6qggkw==
dependencies:
regenerator-runtime "^0.14.0"

"@babel/template@^7.12.7", "@babel/template@^7.18.10", "@babel/template@^7.3.3":
version "7.18.10"
resolved "https://registry.yarnpkg.com/@babel/template/-/template-7.18.10.tgz#6f9134835970d1dbf0835c0d100c9f38de0c5e71"
Expand Down Expand Up @@ -2308,6 +2315,11 @@
schema-utils "^3.0.0"
source-map "^0.7.3"

"@popperjs/core@^2.11.8", "@popperjs/core@^2.9.2":
version "2.11.8"
resolved "https://registry.yarnpkg.com/@popperjs/core/-/core-2.11.8.tgz#6b79032e760a0899cd4204710beede972a3a185f"
integrity sha512-P1st0aksCrn9sGZhp8GMYwBnQsbvAWsZAX44oXNNvLHGqAOcoVxmjZiohstwQ7SqKnbR47akdNi+uleWD8+g6A==

"@popperjs/core@^2.9.1":
version "2.9.2"
resolved "https://registry.yarnpkg.com/@popperjs/core/-/core-2.9.2.tgz#adea7b6953cbb34651766b0548468e743c6a2353"
Expand Down Expand Up @@ -3709,6 +3721,16 @@
dependencies:
"@types/react" "*"

"@types/react-datepicker@^4.19.5":
version "4.19.5"
resolved "https://registry.yarnpkg.com/@types/react-datepicker/-/react-datepicker-4.19.5.tgz#bab0751c5c227e3fbea5979c20b0da4c0fe3a373"
integrity sha512-tKpuj19p9T4sBQm3Bw13CPuhalo4CFOe/LcSUGJ5z6DmHoiBX3uq33iMKePeSEq7OxyU8O1rh5emAm92nyXZLg==
dependencies:
"@popperjs/core" "^2.9.2"
"@types/react" "*"
date-fns "^2.0.1"
react-popper "^2.2.5"

"@types/react-dom@*":
version "18.0.6"
resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-18.0.6.tgz#36652900024842b74607a17786b6662dd1e103a1"
Expand Down Expand Up @@ -6114,6 +6136,11 @@ class-utils@^0.3.5:
isobject "^3.0.0"
static-extend "^0.1.1"

classnames@^2.2.6:
version "2.5.1"
resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.5.1.tgz#ba774c614be0f016da105c858e7159eae8e7687b"
integrity sha512-saHYOzhIQs6wy2sVxTM6bUDsQO4F50V9RQ22qBpEdCW+I+/Wmke2HOl6lS6dTpdxVhb88/I6+Hs+438c3lfUow==

classnames@^2.3.2:
version "2.3.2"
resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.3.2.tgz#351d813bf0137fcc6a76a16b88208d2560a0d924"
Expand Down Expand Up @@ -7162,6 +7189,13 @@ date-fns@^1.27.2:
resolved "https://registry.yarnpkg.com/date-fns/-/date-fns-1.30.1.tgz#2e71bf0b119153dbb4cc4e88d9ea5acfb50dc05c"
integrity sha512-hBSVCvSmWC+QypYObzwGOd9wqdDpOt+0wl0KbU+R+uuZBS1jN8VsD1ss3irQDknRj5NvxiTF6oj/nDRnN/UQNw==

date-fns@^2.0.1, date-fns@^2.30.0:
version "2.30.0"
resolved "https://registry.yarnpkg.com/date-fns/-/date-fns-2.30.0.tgz#f367e644839ff57894ec6ac480de40cae4b0f4d0"
integrity sha512-fnULvOpxnC5/Vg3NCiWelDsLiUc9bRwAPs/+LfTLNvetFCtCTN+yQz15C/fs4AwX1R9K5GLtLfn8QW+dWisaAw==
dependencies:
"@babel/runtime" "^7.21.0"

date-fns@^2.29.1:
version "2.29.3"
resolved "https://registry.yarnpkg.com/date-fns/-/date-fns-2.29.3.tgz#27402d2fc67eb442b511b70bbdf98e6411cd68a8"
Expand Down Expand Up @@ -14716,6 +14750,18 @@ react-codesandboxer@^3.1.5:
lodash.pick "^4.4.0"
react-node-resolver "^1.0.1"

react-datepicker@^4.25.0:
version "4.25.0"
resolved "https://registry.yarnpkg.com/react-datepicker/-/react-datepicker-4.25.0.tgz#86b3ee8ac764bad1650046d0cf9280837bf6d845"
integrity sha512-zB7CSi44SJ0sqo8hUQ3BF1saE/knn7u25qEMTO1CQGofY1VAKahO8k9drZtp0cfW1DMfoYLR3uSY1/uMvbEzbg==
dependencies:
"@popperjs/core" "^2.11.8"
classnames "^2.2.6"
date-fns "^2.30.0"
prop-types "^15.7.2"
react-onclickoutside "^6.13.0"
react-popper "^2.3.0"

react-docgen-typescript@^2.1.1:
version "2.2.2"
resolved "https://registry.yarnpkg.com/react-docgen-typescript/-/react-docgen-typescript-2.2.2.tgz#4611055e569edc071204aadb20e1c93e1ab1659c"
Expand Down Expand Up @@ -14832,6 +14878,11 @@ react-node-resolver@^1.0.1:
resolved "https://registry.yarnpkg.com/react-node-resolver/-/react-node-resolver-1.0.1.tgz#1798a729c0e218bf2f0e8ddf79c550d4af61d83a"
integrity sha1-F5inKcDiGL8vDo3fecVQ1K9h2Do=

react-onclickoutside@^6.13.0:
version "6.13.0"
resolved "https://registry.yarnpkg.com/react-onclickoutside/-/react-onclickoutside-6.13.0.tgz#e165ea4e5157f3da94f4376a3ab3e22a565f4ffc"
integrity sha512-ty8So6tcUpIb+ZE+1HAhbLROvAIJYyJe/1vRrrcmW+jLsaM+/powDRqxzo6hSh9CuRZGSL1Q8mvcF5WRD93a0A==

react-popper@^2.2.3:
version "2.2.5"
resolved "https://registry.yarnpkg.com/react-popper/-/react-popper-2.2.5.tgz#1214ef3cec86330a171671a4fbcbeeb65ee58e96"
Expand All @@ -14840,6 +14891,14 @@ react-popper@^2.2.3:
react-fast-compare "^3.0.1"
warning "^4.0.2"

react-popper@^2.2.5, react-popper@^2.3.0:
version "2.3.0"
resolved "https://registry.yarnpkg.com/react-popper/-/react-popper-2.3.0.tgz#17891c620e1320dce318bad9fede46a5f71c70ba"
integrity sha512-e1hj8lL3uM+sgSR4Lxzn5h1GxBlpa4CQz0XLF8kx4MDrDRWY0Ena4c97PUeSX9i5W3UAfDP0z0FXCTQkoXUl3Q==
dependencies:
react-fast-compare "^3.0.1"
warning "^4.0.2"

react-refresh@^0.11.0:
version "0.11.0"
resolved "https://registry.yarnpkg.com/react-refresh/-/react-refresh-0.11.0.tgz#77198b944733f0f1f1a90e791de4541f9f074046"
Expand Down Expand Up @@ -15106,6 +15165,11 @@ regenerator-runtime@^0.13.4, regenerator-runtime@^0.13.7:
resolved "https://registry.yarnpkg.com/regenerator-runtime/-/regenerator-runtime-0.13.10.tgz#ed07b19616bcbec5da6274ebc75ae95634bfc2ee"
integrity sha512-KepLsg4dU12hryUO7bp/axHAKvwGOCV0sGloQtpagJ12ai+ojVDqkeGSiRX1zlq+kjIMZ1t7gpze+26QqtdGqw==

regenerator-runtime@^0.14.0:
version "0.14.1"
resolved "https://registry.yarnpkg.com/regenerator-runtime/-/regenerator-runtime-0.14.1.tgz#356ade10263f685dda125100cd862c1db895327f"
integrity sha512-dYnhHh0nJoMfnkZs6GmmhFknAGRrLznOu5nc9ML+EJxGvrx6H7teuevqVqCuPcPK//3eDrrjQhehXVx9cnkGdw==

regenerator-transform@^0.15.0:
version "0.15.0"
resolved "https://registry.yarnpkg.com/regenerator-transform/-/regenerator-transform-0.15.0.tgz#cbd9ead5d77fae1a48d957cf889ad0586adb6537"
Expand Down