Closed
Bug 1506547
Opened 5 years ago
Closed 5 years ago
Align user-select behavior more with other UAs
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
Florian pointed this out on the intent email thread for bug 1492739.
Updated•5 years ago
|
Priority: -- → P3
Updated•5 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → emilio
Assignee | ||
Comment 1•5 years ago
|
||
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.
Comment 2•5 years ago
|
||
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?
Flags: needinfo?(emilio)
Comment 3•5 years ago
|
||
Adding Masayuki for visibility...
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(emilio)
Summary: Make user-select an inherited property. → Align user-select behavior more with other UAs
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
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.
Assignee | ||
Comment 5•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d18b8f765358be1d0b9c28aeeff147e98400927
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1575904619b5 Align user-select behavior more with other UAs. r=mats
Comment 7•5 years ago
|
||
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
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e88421b280c Align user-select behavior more with other UAs. r=mats
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(emilio)
Comment 9•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e88421b280c
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 11•5 years ago
|
||
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!
Flags: needinfo?(emilio)
Assignee | ||
Comment 12•5 years ago
|
||
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.
Flags: needinfo?(emilio)
Assignee | ||
Comment 13•5 years ago
|
||
Oh, also the -moz-text value was removed on this bug. The rest looks great, thank you!
Comment 14•5 years ago
|
||
(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!
Comment 15•4 years ago
|
||
I've cleaned up the docs on this a bit more:
- Edited https://developer.mozilla.org/en-US/docs/Web/CSS/user-select to remove mention of -moz-none
- Submitted a PR to add removal data for -moz-none to our compat data: https://bugzilla.mozilla.org/show_bug.cgi?id=1506547
Setting to dev-doc-complete
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•