Align user-select behavior more with other UAs

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on 1 bug, {dev-doc-complete})

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

Posted file Testcase
Florian pointed this out on the intent email thread for bug 1492739.
Priority: -- → P3
Assignee: nobody → emilio
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?
Flags: needinfo?(emilio)
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.
Flags: needinfo?(emilio)
Summary: Make user-select an inherited property. → Align user-select behavior more with other UAs
Depends on: 1509535
Blocks: 1509535
No longer depends on: 1509535
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.
See Also: → 1509650
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1575904619b5
Align user-select behavior more with other UAs. r=mats
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e88421b280c
Align user-select behavior more with other UAs. r=mats
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/8e88421b280c
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Duplicate of this bug: 1509535
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)
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)
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: 1524266

I've cleaned up the docs on this a bit more:

Setting to dev-doc-complete

Duplicate of this bug: 1449936
You need to log in before you can comment on or make changes to this bug.