Closed Bug 1506547 Opened 2 years ago Closed 2 years ago
Align user-select behavior more with other UAs
Florian pointed this out on the intent email thread for bug 1492739.
There's a few subtle behavior changes here, which I'll try to break down in the commit message. The biggest one is the EditableDescendantCount stuff going away. I wish Ehsan was accepting review requests so I could submit that separately, oh well. This was added in bug 1181130, to prevent clicking on the non-editable div from selecting the editable div inside. This is problematic for multiple reasons: * First, I don't think non-editable regions of an editable element should be user-select: all, but right now they need to be to avoid bug 1132768. I think it's not the right fix, but it is our current behavior. I can fix this in a follow-up if wanted by removing that rule and avoiding putting the caret in a non-editable element. * Second, it just doesn't work in Shadow DOM (the editable descendant count is not kept up-to-date when not in the uncomposed doc), so nested contenteditables behave differently inside vs. outside a Shadow Tree. * Third, I think it's user hostile to just entirely disable selection if you have a contenteditable descendant as a child of a user-select: all thing. WebKit behaves like this patch in the following test-case (though not Blink): https://crisal.io/tmp/user-select-all-contenteditable-descendant.html Edge doesn't seem to support user-select: all at all (no pun intended). But we don't allow to select anything at all which looks wrong. * Fourth, it's not tested at all (which explains how we broke it in Shadow DOM and not even notice...). In any case I've verified that this doesn't regress the editor from that bug. If this regresses anything we can fix it as outlined in the first bullet point above, which should also make us more compatible with other UAs in that test-case. The other change is `all` not overriding everything else. So, something like: <div style="-webkit-user-select: all">All <div style="-webkit-user-select: none">None</div></div> Totally ignores the -webkit-user-select: none declaration in Firefox before this change. This doesn't match any other UA nor the spec, and this patch aligns us with WebKit / Blink. This in turn makes us not need -moz-text anymore, whose only purpose was to avoid this.
https://drafts.csswg.org/css-ui-4/#propdef-user-select says "Inherited: no" but your patch is making it inherited. Is there a spec bug about this?
Adding Masayuki for visibility...
Sigh... For some reason I thought the spec editor had checked when pointing out: > I would like to note however that the basic way the property works (inherited vs not, initial value of auto, different between 'auto' and 'text') is different in the spec and in your implementation. The difference is intentional, and necessary to support the `contain` value and to have a model that explains what happens on editable elements. In https://groups.google.com/forum/#!topic/mozilla.dev.platform/XfKl9Jt7ZQ8. I'll preserve this bug to align more with other UAs but keep the reset property. Which is roughly equivalent (modulo display: contents and other bits) as long as we keep auto as the initial value. I'll file spec issues regarding the value computation that almost always inherits but not quite. I think it should be an used-value time thing, because otherwise it's a bummer perf-wise.
Summary: Make user-select an inherited property. → Align user-select behavior more with other UAs
Attachment #9027019 - Attachment description: Bug 1506547 - Make user-select inherited, and simplify a bunch its implementation. → Bug 1506547 - Align user-select behavior more with other UAs.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1575904619b5 Align user-select behavior more with other UAs. r=mats
Backed out changeset 1575904619b5 (Bug 1506547) for mochitest failures on test_reftests_with_caret.html. Backout: https://hg.mozilla.org/integration/autoland/rev/14d532b8d89432c1a4b01d5007960295a13022da Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1575904619b592d61f38ce67ef94faa313108a2e&selectedJob=213771918 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=213771918&repo=autoland&lineNumber=8015
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/8e88421b280c Align user-select behavior more with other UAs. r=mats
I have tried to summarise the behavior changes caused by this bug, see: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#CSS However I find this confusing and I'm really not sure if I've got it right. Can you review it for me? Also, let me know if you think anything needs to be documented on the user-select reference page. Thanks!
There's one bit at least which is a bit inaccurate: > contenteditable elements nested inside elements with user-select: all set on them are now selectable. That should read: > non-contenteditable elements nested inside contenteditable elements are now selectable.
Oh, also the -moz-text value was removed on this bug. The rest looks great, thank you!
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13) > Oh, also the -moz-text value was removed on this bug. The rest looks great, > thank you! All updated. Thanks Emilio!
Depends on: 1526393
You need to log in before you can comment on or make changes to this bug.