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.

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

[DataGrid] Fix crash when id has a single-quote #2033

Merged
merged 5 commits into from Jul 10, 2021

Conversation

rbrishabh
Copy link
Contributor

Fixes #1953

Hey @oliviertassinari! How does this look?
Let me know if this needs any changes! Thanks

@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jul 4, 2021
@oliviertassinari oliviertassinari changed the title Fix: When id has a single-quote or newline character checkboxSelection fails [DataGrid] Fix crash when id has a single-quote Jul 7, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 7, 2021

I have used the opportunity to remove the already implicit :scope logic: 7b54f30. It's a distraction.

@oliviertassinari oliviertassinari requested a review from a team July 7, 2021 23:37
@@ -62,7 +62,7 @@ export function getGridColumnHeaderElement(root: Element, field: string) {

export function getGridRowElement(root: Element, id: GridRowId) {
return root.querySelector(
`:scope .${GRID_ROW_CSS_CLASS}[data-id="${escapeOperandAttributeSelector(String(id))}"]`,
`.${GRID_ROW_CSS_CLASS}[data-id="${escapeOperandAttributeSelector(String(id))}"]`,
Copy link
Member

Choose a reason for hiding this comment

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

we already have an escapeRegExp, can't we reuse it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, regexp and attribute selectors are unrelated, they use different alphabets. Why should the escaping be identical?

Copy link
Member

Choose a reason for hiding this comment

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

hence the question can't we reuse it? if they both result in \' or similar we might not need a new method.
Also removing :scope will now allow the header row as a result

Copy link
Member

Choose a reason for hiding this comment

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

OK, I will keep two different methods because the concepts are unrelated.

Regarding :scope, do you have a simple codesandbox to illustrate the issue?

Copy link
Member

Choose a reason for hiding this comment

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

I have reverted the change around :scope as it doesn't need to be in this PR, and it also doesn't seem to be that trivial. Move to #2115.

This reverts commit 7b54f30.
@oliviertassinari oliviertassinari merged commit bcf4a91 into mui:master Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] When id has a single-quote or newline character checkboxSelection fails
4 participants