1097242 - -moz-user-select on contenteditable=false regions inside a contenteditable region gets ignored

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Boris Lykah]

Attachment: testcase.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/38.0.2125.111 Chrome/38.0.2125.111 Safari/537.36

Steps to reproduce:

Open the attached file. Click once on "click here".


Actual results:

Division with text "click here" is selected.


Expected results:

The selection should remain the same and the div with "click here" should get focus.

Comment 1

[Roan Kattouw]

http://jsfiddle.net/8ea7aff2/ (from bug 1132768) exposes this too. Note that this only happens when the selection moves within the same ce=true container, and that even -moz-user-select: none; is ignored in this case.

Comment 2

[(no longer active)]

Hmm, Roan, this is exactly like bug 1097242, isn't it?  I'm not sure if I understand the distinction between the two.

Comment 3

[(no longer active)]

Err, I meant to say bug 1132768.  :-)

Comment 4

[Roan Kattouw]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Hmm, Roan, this is exactly like bug 1097242, isn't it?  I'm not sure if I
> understand the distinction between the two.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Err, I meant to say bug 1132768.  :-)

They're similar but they are different. I'm sorry for not making the distinction clearer.

When you click on an element that is cE=false, there are two cases.

Case 1: the selection was previously in the same cE=true container that contains the element you clicked (i.e $(selection.anchorNode).closest('[contentEditable=true]') is the same as $(clickedNode).closest('[contentEditable=true]')). In this case, all of clickedNode is selected, even if it has -moz-user-select: none; . That's this bug: cE=false is respected, but the selection behavior is weird and -moz-user-select is ignored.

Case 2: the selection was previously in a different cE=true container (i.e $(selection.anchorNode).closest('[contentEditable=true]') is not the same as $(clickedNode).closest('[contentEditable=true]')). In this case, the cursor is placed inside of clickedNode, even though you're not supposed to be able to get the cursor into a cE=false node. That's bug 1132768: cE=false is ignored. But fortunately -moz-user-select *is* respected in this case (insofar as it is ever respected inside a contentEditable div; but that's a different bug), so I was able to work around this by just applying -moz-user-select: none; to all nodes that have cE=false.

Case 3: the selection was previously not in a cE=true container at all; I didn't test this when I first reported the bug, but I just did, and it behaves exactly like case 2.

Comment 5

[(no longer active)]

(In reply to Roan Kattouw from comment #4)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #2)
> > Hmm, Roan, this is exactly like bug 1097242, isn't it?  I'm not sure if I
> > understand the distinction between the two.
> 
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #3)
> > Err, I meant to say bug 1132768.  :-)
> 
> They're similar but they are different. I'm sorry for not making the
> distinction clearer.
> 
> When you click on an element that is cE=false, there are two cases.
> 
> Case 1: the selection was previously in the same cE=true container that
> contains the element you clicked (i.e
> $(selection.anchorNode).closest('[contentEditable=true]') is the same as
> $(clickedNode).closest('[contentEditable=true]')). In this case, all of
> clickedNode is selected, even if it has -moz-user-select: none; . That's
> this bug: cE=false is respected, but the selection behavior is weird and
> -moz-user-select is ignored.

Ah, I see.  The reason why -moz-user-select gets ignored on these elements is that this property <https://hg.mozilla.org/integration/mozilla-inbound/file/ed12a3822cb9/layout/style/contenteditable.css#l13> is marked as !important.

David, can you think of why this and for that matter other -moz-user-select properties in this file need to be !important?  At the first glance, they shouldn't need to be...

> Case 2: the selection was previously in a different cE=true container (i.e
> $(selection.anchorNode).closest('[contentEditable=true]') is not the same as
> $(clickedNode).closest('[contentEditable=true]')). In this case, the cursor
> is placed inside of clickedNode, even though you're not supposed to be able
> to get the cursor into a cE=false node. That's bug 1132768: cE=false is
> ignored. But fortunately -moz-user-select *is* respected in this case
> (insofar as it is ever respected inside a contentEditable div; but that's a
> different bug), so I was able to work around this by just applying
> -moz-user-select: none; to all nodes that have cE=false.
> 
> Case 3: the selection was previously not in a cE=true container at all; I
> didn't test this when I first reported the bug, but I just did, and it
> behaves exactly like case 2.

I just landed the fix for bug 1132768 which will make all of these cases behave identical to case #1.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Ah, I see.  The reason why -moz-user-select gets ignored on these elements
> is that this property
> <https://hg.mozilla.org/integration/mozilla-inbound/file/ed12a3822cb9/layout/
> style/contenteditable.css#l13> is marked as !important.
> 
> David, can you think of why this and for that matter other -moz-user-select
> properties in this file need to be !important?  At the first glance, they
> shouldn't need to be...

I don't know for sure.  But it does seem like it would be odd for a contenteditable region to contain unselectable text -- perhaps that was the intent?  That said, maybe one could assume that the property should apply while contenteditable is true just like it does otherwise.  So it could make sense for the properties to apply as normal.

The style sheet may have been written with the assumption that these properties were standards-track (which they once were), and thus that pages might be likely to be using them.

Maybe these rules were copied from an editor stylesheet that was designed for the editor as a separate tool/mode rather than part of the Web platform, where such overrides might make more sense?


That said, it definitely seems like a bug that these rules are applying to a non-contenteditable region inside a contenteditable one.  Maybe they should have a different mechanism to apply them, like a pseudo-class (maybe even an existing pseudo-class) that applies when something is editable.  (It might be nice to wait until after bug 1086641.)

Comment 7

[Roan Kattouw]

(In reply to David Baron [:dbaron] (UTC-8) from comment #6)
> I don't know for sure.  But it does seem like it would be odd for a
> contenteditable region to contain unselectable text -- perhaps that was the
> intent?  That said, maybe one could assume that the property should apply
> while contenteditable is true just like it does otherwise.  So it could make
> sense for the properties to apply as normal.
> 

Yeah, to be clear, I was only applying -moz-user-select: none; as a workaround for bug 1132768, not because I actually wanted the text to be unselectable. Right now user-select: none; text is unselectable outside of a cE area, but still selectable (as a unit) in cE=false. Technically speaking that's probably a bug, but in practice I don't mind :)

Comment 8

[(no longer active)]

(In reply to David Baron [:dbaron] (UTC-8) from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > Ah, I see.  The reason why -moz-user-select gets ignored on these elements
> > is that this property
> > <https://hg.mozilla.org/integration/mozilla-inbound/file/ed12a3822cb9/layout/
> > style/contenteditable.css#l13> is marked as !important.
> > 
> > David, can you think of why this and for that matter other -moz-user-select
> > properties in this file need to be !important?  At the first glance, they
> > shouldn't need to be...
> 
> I don't know for sure.  But it does seem like it would be odd for a
> contenteditable region to contain unselectable text -- perhaps that was the
> intent?  That said, maybe one could assume that the property should apply
> while contenteditable is true just like it does otherwise.  So it could make
> sense for the properties to apply as normal.
> 
> The style sheet may have been written with the assumption that these
> properties were standards-track (which they once were), and thus that pages
> might be likely to be using them.
> 
> Maybe these rules were copied from an editor stylesheet that was designed
> for the editor as a separate tool/mode rather than part of the Web platform,
> where such overrides might make more sense?

Looking at the history of this code, it seems to come from our original implementation of contenteditable, it's not really clear why things are this way.  I can't think of a good reason why we would want to override a Web developer's choice here though.

> That said, it definitely seems like a bug that these rules are applying to a
> non-contenteditable region inside a contenteditable one.  Maybe they should
> have a different mechanism to apply them, like a pseudo-class (maybe even an
> existing pseudo-class) that applies when something is editable.

We have :-moz-read-write that does that, which is what is being used in that stylesheet.  I'm not sure why you expect that to help though.

> (It might be nice to wait until after bug 1086641.)

Hmm, why wait for that bug?  None of this stuff should be native anonymous content...

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> (In reply to David Baron [:dbaron] (UTC-8) from comment #6)
> > Maybe these rules were copied from an editor stylesheet that was designed
> > for the editor as a separate tool/mode rather than part of the Web platform,
> > where such overrides might make more sense?
> 
> Looking at the history of this code, it seems to come from our original
> implementation of contenteditable, it's not really clear why things are this
> way.  I can't think of a good reason why we would want to override a Web
> developer's choice here though.

Seems reasonable.  (It may well have been copied from some other editor style sheet.)

> > That said, it definitely seems like a bug that these rules are applying to a
> > non-contenteditable region inside a contenteditable one.  Maybe they should
> > have a different mechanism to apply them, like a pseudo-class (maybe even an
> > existing pseudo-class) that applies when something is editable.
> 
> We have :-moz-read-write that does that, which is what is being used in that
> stylesheet.  I'm not sure why you expect that to help though.

Hmmm.  Is :-moz-read-write implemented incorrectly?

> > (It might be nice to wait until after bug 1086641.)
> 
> Hmm, why wait for that bug?  None of this stuff should be native anonymous
> content...

I was just thinking there might be an issue with merge conflicts, although I suppose they'd be easy to resolve.

Comment 10

[(no longer active)]

(In reply to David Baron [:dbaron] (UTC-8) from comment #9)
> > > That said, it definitely seems like a bug that these rules are applying to a
> > > non-contenteditable region inside a contenteditable one.  Maybe they should
> > > have a different mechanism to apply them, like a pseudo-class (maybe even an
> > > existing pseudo-class) that applies when something is editable.
> > 
> > We have :-moz-read-write that does that, which is what is being used in that
> > stylesheet.  I'm not sure why you expect that to help though.
> 
> Hmmm.  Is :-moz-read-write implemented incorrectly?

No, the selector in question looks like:

<https://dxr.mozilla.org/mozilla-central/source/layout/style/contenteditable.css?from=contenteditable.css&case=true#12>

*|*:-moz-read-write :-moz-read-only

which matches non-contenteditable sections inside contenteditable ones.  Nothing unexpected there!

> > > (It might be nice to wait until after bug 1086641.)
> > 
> > Hmm, why wait for that bug?  None of this stuff should be native anonymous
> > content...
> 
> I was just thinking there might be an issue with merge conflicts, although I
> suppose they'd be easy to resolve.

Hmm, that bug seems a bit stalled...  I just checked and there will only be a few conflicts, nothing too hard to resolve.

Comment 11

[(no longer active)]

Attachment: Make sure contenteditable -moz-user-select styles can be overridden by web pages

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02f7725921c

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a02f7725921c