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

[core] Handle selection edge case #7350

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes component: data grid This is the name of the generic UI component, not the React module! labels Dec 30, 2022
@mui-bot
Copy link

mui-bot commented Dec 30, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-7350--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 634.3 997.9 869.1 808.9 142.687
Sort 100k rows ms 607.7 1,140.9 957.3 934.06 176.019
Select 100k rows ms 212.8 286.5 249.2 243.32 26.672
Deselect 100k rows ms 163.9 348.8 195.4 221.64 66.249

Generated by 🚫 dangerJS against 6e28144

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Is it really a core PR ?

@oliviertassinari
Copy link
Member Author

I think that it has barely any impact, so most people wouldn't care.

@oliviertassinari oliviertassinari changed the title [core] Handle selecton edge case [core] Handle selection edge case Dec 30, 2022
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@@ -26,15 +26,15 @@ function writeToClipboardPolyfill(data: string) {
}

function hasNativeSelection(element: HTMLInputElement) {
if (window.getSelection()?.toString() !== '') {
Copy link
Member Author

Choose a reason for hiding this comment

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

@cherniavskii I think that this wasn't exactly correct. Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

If the object accessed or function called is undefined or null, it returns undefined instead of throwing an error.

So when the selection returns null, the chaining returns undefined which we then compare to ''. it's different, so hasNativeSelection say there is a selection. But there is no selection in this case, in theory, per https://developer.mozilla.org/en-US/docs/Web/API/Window/getSelection.

When called on an <iframe> that is not displayed (e.g., where display: none is set) Firefox will return null

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Nice catch!

@oliviertassinari oliviertassinari merged commit b9f1560 into mui:next Jan 11, 2023
@oliviertassinari oliviertassinari deleted the selection-polish branch January 11, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants