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

552 fix #583

Closed
wants to merge 10 commits into from
Closed

552 fix #583

wants to merge 10 commits into from

Conversation

sh0ji
Copy link
Contributor

@sh0ji sh0ji commented Jan 23, 2018

Fixes #552, as recommended by @erikkroes.

To test with a keyboard: open a dialog, move focus to a non-editable element, and press ctrl-a to select all. Inspect the contents of that selection.

While this does technically disable user-selection when a dialog is active, it also adds coupling to the HTML structure outside of the dialog, which concerns me. Specifically, prior to this PR, dialogs operated as standalone units that didn't affect other elements on the page. With this PR, user-select: none; needs to be applied to everything that we don't want to be selected. That's just the #base_window_layer here, but that's only a coincidence of how the HTML is structured.

The only other alternative I can imagine would be messing about with selection listeners, and I don't like that very much either.

I'd be okay with delaying this or not merging it.

@mcking65
Copy link
Contributor

Based on discussion with Evan and other members of the task force in the February 5, 2018 meeting, closing this PR with a decision not to fix issue #552.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants