1506547 - Align user-select behavior more with other UAs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

Florian pointed this out on the intent email thread for bug 1492739.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1506547 - Align user-select behavior more with other UAs.

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

[Mats Palmgren (inactive)]

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?

Comment 3

[(no longer active)]

Adding Masayuki for visibility...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d18b8f765358be1d0b9c28aeeff147e98400927

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1575904619b5
Align user-select behavior more with other UAs. r=mats

Comment 7

[Cristian Brindusan [:cbrindusan]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e88421b280c
Align user-select behavior more with other UAs. r=mats

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8e88421b280c

Comment 11

[Chris Mills [:cmills]]

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!

Comment 12

[Emilio Cobos Álvarez (: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.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Oh, also the -moz-text value was removed on this bug. The rest looks great, thank you!

Comment 14

[Chris Mills [:cmills]]

(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

[Chris Mills [:cmills]]

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