Skip to content

[DataGridPro] Prevent text selection when reordering rows #16568

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

Merged
merged 6 commits into from
May 20, 2025

Conversation

jyash97
Copy link
Contributor

@jyash97 jyash97 commented Feb 13, 2025

Closes #16303

Debug Info on the same

@zannager zannager added the scope: data grid Changes or issues related to the data grid product label Feb 14, 2025
@zannager zannager requested a review from arminmeh March 14, 2025 08:55
@arminmeh arminmeh added type: bug A bug or unintended behavior in the components. feature: Reordering Related to the data grid Reordering feature needs cherry-pick The PR should be auto-cherry-picked to a version branch after merge—add a version label as target v7.x labels Mar 14, 2025
@arminmeh arminmeh changed the title [DataGrid]: prevent text selection when reordering rows [DataGrid] Prevent text selection when reordering rows Mar 14, 2025
@arminmeh
Copy link
Contributor

Thanks for the contribution @jyash97
Can you run prettier script to format the code properly?

Also, can you use the approach used in cell selection to disable the selection?

@jyash97
Copy link
Contributor Author

jyash97 commented Mar 14, 2025

Hey @arminmeh

Thanks for the review, give me sometime will apply the changes

@jyash97
Copy link
Contributor Author

jyash97 commented Mar 24, 2025

Hey @arminmeh I didnt get chance to look into these yet, I will try to get to these by Wednesday.

@jyash97
Copy link
Contributor Author

jyash97 commented Mar 28, 2025

@arminmeh after making the requested changes, I see the issue happening again, I can confirm that the root class is being applied properly.

Didnt get chance to look further into these yet

@arminmeh
Copy link
Contributor

@jyash97 thanks for the update. I will take a quick look and check what is different from your initial solution.
Thanks for the contribution so far

@jyash97
Copy link
Contributor Author

jyash97 commented Mar 28, 2025

I will take a quick look and check what is different from your initial solution.

Awesome, looking forward to know what caused the regression.

Looking forward to contribute more once I get some bandwidth

@arminmeh arminmeh force-pushed the yash/fix/row-reorder branch from 83a0d8f to 0385855 Compare April 23, 2025 09:40
@mui-bot
Copy link

mui-bot commented Apr 23, 2025

Deploy preview: https://deploy-preview-16568--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 7913daf

@arminmeh arminmeh requested a review from a team April 23, 2025 13:54
@arminmeh
Copy link
Contributor

The root cause for this behavior is that, when the text is selected, event happens on the svg element rather on the div
Event handler picks it up because of the event bubbling, but because svg element is not actually moved dragStop event is called immediately causing timeout clearance, etc (explained in the debug info above)

I have tried other ways to prevent event on happening on the svg element, but none of them feels elegant enough.

From the last update from @jyash97 I have only moved the class append/remove logic into the same file, and tweaked the styles to apply on all elements once appended (before it was only on the regular cells)

@arminmeh arminmeh enabled auto-merge (squash) April 24, 2025 06:20
@arminmeh arminmeh disabled auto-merge April 24, 2025 06:20
[`&.${c['root--disableUserSelection']} .${c.cell}`]: {
[`&.${c['root--disableUserSelection']}`]: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a class for this? It feels more verbose than it needs to be, we could just set style.userSelect = '...' directly.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but since we already have this class, I don't mind using it in this scenario too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how did we handle this in the past. Can we remove a class in the minor release?

Copy link
Member

@KenanYusuf KenanYusuf May 20, 2025

Choose a reason for hiding this comment

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

I think we have stopped applying styles to classes internally, but not removed. Perhaps we can add to #17846

@cherniavskii cherniavskii added the CLA: required See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 label Apr 28, 2025
@cherniavskii
Copy link
Member

Hi @jyash97
Thank you for this pull request! Your changes seem to impact the commercially licensed code. For any changes of this nature, we require contributors to sign the MUI’s Contributor License Agreement (CLA). However, I can’t find a CLA signed that could cover these changes. Please follow the steps at https://mui-org.notion.site/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 to sign the document.

@jyash97
Copy link
Contributor Author

jyash97 commented May 2, 2025

Hey @cherniavskii

Can you check again? I might have missed it, I just filled the form

@zannager
Copy link
Member

zannager commented May 2, 2025

@jyash97 can you please complete the step 3? - Forward the “form submission confirmation” email (check also your spam folder) to [email protected] to confirm your identity. Also, include the link to the pull request it corresponds to. Thanks.

@zannager zannager added CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 and removed CLA: required See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 labels May 2, 2025
@zannager
Copy link
Member

zannager commented May 2, 2025

@cherniavskii CLA signed, thanks.

@arminmeh arminmeh changed the title [DataGrid] Prevent text selection when reordering rows [DataGridPro] Prevent text selection when reordering rows May 5, 2025
@arminmeh arminmeh requested a review from a team May 19, 2025 07:03
Comment on lines 81 to 86
const onMouseDown = React.useCallback(() => {
// Prevent text selection as it will block all the drag events. More context: https://github.com/mui/mui-x/issues/16303
apiRef.current.rootElementRef?.current?.classList.add(
gridClasses['root--disableUserSelection'],
);
}, [apiRef]);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we remove this class on onMouseUp?
onDragEnd doesn't necessarily get fired after each onMouseDown.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.
I did some testing and depending on the interaction, sometimes only onDragEnd is called, sometimes only onMouseUp and sometimes I see both.

Copy link
Member

Choose a reason for hiding this comment

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

Surprised onMouseUp only sometimes fires 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember adding mouse up event to clear user selection, but was facing issue, don't remember the exact reason at the moment, but had to resort back to drag end event to clear it

@arminmeh arminmeh force-pushed the yash/fix/row-reorder branch from 718048d to 4ad6412 Compare May 20, 2025 07:37
@arminmeh arminmeh requested a review from MBilalShafi May 20, 2025 07:48
[`&.${c['root--disableUserSelection']} .${c.cell}`]: {
[`&.${c['root--disableUserSelection']}`]: {
Copy link
Member

Choose a reason for hiding this comment

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

I agree, but since we already have this class, I don't mind using it in this scenario too.

Comment on lines 81 to 86
const onMouseDown = React.useCallback(() => {
// Prevent text selection as it will block all the drag events. More context: https://github.com/mui/mui-x/issues/16303
apiRef.current.rootElementRef?.current?.classList.add(
gridClasses['root--disableUserSelection'],
);
}, [apiRef]);
Copy link
Member

Choose a reason for hiding this comment

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

Surprised onMouseUp only sometimes fires 🤔

@arminmeh arminmeh requested a review from KenanYusuf May 20, 2025 08:01
@arminmeh arminmeh merged commit 0e6b079 into mui:master May 20, 2025
23 checks passed
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 feature: Reordering Related to the data grid Reordering feature needs cherry-pick The PR should be auto-cherry-picked to a version branch after merge—add a version label as target scope: data grid Changes or issues related to the data grid product type: bug A bug or unintended behavior in the components. v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Order icon changes on drag when text is selected
8 participants