User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36 Steps to reproduce: 1. Visit https://jsfiddle.net/oleq/hfLfjd55 or open attached sample 2. Focus the innermost contenteditable="true" element. 3. Click to place a caret in the middle of the text. 4. Use arrow keys to move the caret (optional). Actual results: When clicked: The caret moves either to the very beginning of the innermost contenteditable="true" element (nested editable) or both contenteditable="false" and contenteditable="true" child element (nested editable) are selected. When navigating with keyboard arrows: The caret doesn't move within nested editable. This way or another it's impossible to edit anything in nested editable. Expected results: The caret should appear directly where clicked. It should be possible to move it with arrow keys. Content editing in nested contenteditables should be possible. This *recent* regression completely breaks nested editables, one of the key features of CKEditor * sample http://sdk.ckeditor.com/samples/captionedimage.html * issue http://dev.ckeditor.com/ticket/13507#comment:1 * question on SO http://stackoverflow.com/questions/31257123/unexpected-behaviour-of-nested-contenteditable-divs-in-firefox

This is a critical bug for many users, because it makes it impossible to edit text inside what we call "nested editable". Two bug reports within few hours prove that.

Narrowed down the regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9e73726c6d30&tochange=99419a04c349 and the culprit is Bug 1132768.

[Tracking Requested - why for this release]: a site compatibility regression of Firefox 39 affecting the popular CKEditor text editor.

The root cause of this bug is that -moz-user-select: all on a parent element overrides it on all children, so once an editable region ends up having a contenteditable=false parent, the parent's -moz-user-select ends up applying to the child. I'm working on a fix, but I don't think it's going to be safe for beta. We should probably back out bug 1132768 on beta...

Tracking for 40, 41, and 42 because regression. Need info'ing Liz about 39.

(In reply to Kate Glazko from comment #6) > Tracking for 40, 41, and 42 because regression. Need info'ing Liz about 39. This regression is pretty bad. Fortunately, since we also fixed bug 1097242 for Firefox 39, affected websites can override our user agent stylesheets with their own styles. But sadly, this affects CKEditor, which is a very popular editing library used in many different web sites, and because of that in practice, if we don't do anything for 39, I would expect the affected users to be stuck with this bug for 6 weeks. :( It's very difficult to guess how many real websites this breaks and in what ways...

Comment on attachment 8630746 [details] [diff] [review] Backout bug 1132768 in order to fix the regression in bug 1181130 Approval Request Comment [Feature/regressing bug #]: Bug 1132768 [User impact if declined]: Comment 0 and comment 8. [Describe test coverage new/current, TreeHerder]: This is a backout of the fix to bug 1132768. [Risks and why]: This brings us back to before fixing bug 1132768. I'll create a proper fix for trunk. [String/UUID change made/needed]: None.

My approach for a proper fix for this bug is as follows: 1. Introduce a -moz-user-select: -moz-text property. This acts similar to text, except that it won't be overridden by -moz-user-select: all elements in the parent chain. 2. Add a rule to apply this rule to contenteditable elements inside contenteditable=false regions. I have the above working so far, but there is a bad edge case. When you click on "non-editable div" in the test case, we would try to select all of the contenteditable=false region, which of course includes "nested editable div"! That is pretty crappy behavior that we need to fix as well. The best way that I could think of to do that is: 3. Add a new flag to nsINode to indicate whether something in the descendants of the element is editable, and in nsFrame::IsSelectable, check to see if we are on an NS_STYLE_USER_SELECT_ALL frame that has this flag set on its content, and in that case pretend that we are unselectable. We should be able to efficiently set that flag in UpdateEditableState when a node starts to become editable by walking up the parent chain. Unsetting it much more time consuming though. I'm currently thinking that we could deal with that by walking up the parent chain when a node becomes uneditable, and if we find a parent with that flag set, we would walk over all of the descendants of that node and see if anything is editable, but that seems way too expensive... I can't think of anything better at the moment. Boris, can you think of a better way to deal with #3? (One option would be use a property that acts as the counter for the editable sub-nodes, that way we would handle the case where a node becomes uneditable by walking up the parent chain and decrementing this counter, but now we're trading space for time.)

Just to chime in with a real-world site hit by this. I reduced what I was seeing to: http://jsfiddle.net/pkwf39tx/ Experienced same on 41 dev release. 38 fine.

Comment on attachment 8630746 [details] [diff] [review] Backout bug 1132768 in order to fix the regression in bug 1181130 Let's get this fix on Aurora and Beta immediately. We'll look at whether this warrants a 39 point release as well.

This is also breaking all table editing on Wikipedia (using VisualEditor): https://phabricator.wikimedia.org/T103035 +1 for a point release fix.

bug 1180676 looks like a duplicate

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #10) > 3. Add a new flag to nsINode to indicate whether something in the > descendants of the element is editable, and in nsFrame::IsSelectable, check > to see if we are on an NS_STYLE_USER_SELECT_ALL frame that has this flag set > on its content, and in that case pretend that we are unselectable. > > We should be able to efficiently set that flag in UpdateEditableState when a > node starts to become editable by walking up the parent chain. Unsetting it > much more time consuming though. I'm currently thinking that we could deal > with that by walking up the parent chain when a node becomes uneditable, and > if we find a parent with that flag set, we would walk over all of the > descendants of that node and see if anything is editable, but that seems way > too expensive... > > I can't think of anything better at the moment. Boris, can you think of a > better way to deal with #3? > > (One option would be use a property that acts as the counter for the > editable sub-nodes, that way we would handle the case where a node becomes > uneditable by walking up the parent chain and decrementing this counter, but > now we're trading space for time.) I talked to Boris about this on IRC, and we decided to use the second option (the counter option.) The counter would be implemented as a node property. We can use the lack of existence of the counter as being the same as counter == 0, which would mean that we wouldn't need to create the property for nodes until they really need it.

One other note from IRC: we should file a followup bug to just stop using "-moz-user-select:all" for the non-editable bits inside contenteditable. Instead, we should use whatever the default -moz-user-select behavior is for them. That would probably make all these problems go away...

Comment on attachment 8630746 [details] [diff] [review] Backout bug 1132768 in order to fix the regression in bug 1181130 It is likely that we will do a 39.0.1, taking it to be ready.

Tracked as it is potentially a driver for 39.0.1

I was able to reproduce the issue using Firefox 39.0, on Windows 7 x64, Mac OS X 10.9.5, and Ubuntu 12.04 x86, using scenario from comment 0, and the following test pages: - https://jsfiddle.net/oleq/hfLfjd55 - https://bugzilla.mozilla.org/attachment.cgi?id=8630459 - http://sdk.ckeditor.com/samples/captionedimage.html The issue no longer reproduced when using Firefox 40 Beta 6 (BuildID=20150720220238) on the same environments. Behavior is now reverted to the one in Firefox 38, and now you can place the caret at any position, move it around, edit, paste, delete... I did run into an issue where if you first place the caret in the outer editable area and then in the nested editable element, then the caret is placed at the very beginning. This appears to be tracked in bug 1039217, and has been around for quite a while. Marking this as verified for Firefox 40.

This patch takes the approach described in comment 20. Although, instead of updating the counter in UpdateEditableState, we update it in a manner similar to ChangeContentEditableCount because UpdateEditableState will update for every descendant of an editable and we probably want to avoid walking the parent chain for all those nodes when it seems like we really only care about nodes where the editable state has been explicitly changed.

Updated with tests. See comment 25 for what this patch is doing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b20dd3586e1b Also, do parts 1 and 2 need reviews are are they already reviewed?

No, those parts are not reviewed yet. I'll flag them once I look at your patch. Thanks!

Comment on attachment 8638126 [details] [diff] [review] Part 3: Keep track of editable descendants per node and prevent NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. Review of attachment 8638126 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good, but I'd like Boris to review it, as this is touching core Node stuff... ::: editor/libeditor/tests/test_bug1181130.html @@ +16,5 @@ > +var noneditable = document.getElementById("noneditable"); > +var editable = document.getElementById("editable"); > + > +async_test(function(t) { > + window.addEventListener("focus", function() { Don't you want to use waitForFocus() here? @@ +20,5 @@ > + window.addEventListener("focus", function() { > + synthesizeMouseAtCenter(noneditable, {}); > + var s = document.getSelection(); > + t.step(function() { assert_false(s.toString().includes("nested editable div")); }); > + t.done(); I think it would be nice to also test what happens when you click on the other two elements here.

Comment on attachment 8638126 [details] [diff] [review] Part 3: Keep track of editable descendants per node and prevent NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. >+nsINode::ChangeEditableDescendantCount(int32_t aDelta) >+ if (s) { s can't be null here. Which is good, since you already dereferenced it in the previous line. ;) >+ void ResetEditableDescendantCount(); This needs some documentation. Like when one would use this, at the very least. >@@ -550,16 +550,22 @@ nsGenericHTMLElement::BindToTree(nsIDocu So this part is a bit annoying. It means that when we insert a tree we will end up with walks up to the root for every editable node. And we can have quite a number of editable nodes in that subtree... Would it not make more sense to do this in the places where the BintToTree call or corresponding ContentInserted/Appended notification happens on the subtree root and just update the ancestors with the descendant count of the thing being inserted, plus 1 if it itself is editable? Or does that not work because we don't maintain the count when we're not inside the document? Why do we not do that? Also, we don't seem to be checking for anonymous content here in BindToTree. Should we be?

(In reply to Boris Zbarsky [:bz] from comment #29) > Comment on attachment 8638126 [details] [diff] [review] > Part 3: Keep track of editable descendants per node and prevent > NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. > >@@ -550,16 +550,22 @@ nsGenericHTMLElement::BindToTree(nsIDocu > > So this part is a bit annoying. It means that when we insert a tree we will > end up with walks up to the root for every editable node. And we can have > quite a number of editable nodes in that subtree... > > Would it not make more sense to do this in the places where the BintToTree > call or corresponding ContentInserted/Appended notification happens on the > subtree root and just update the ancestors with the descendant count of the > thing being inserted, plus 1 if it itself is editable? Or does that not > work because we don't maintain the count when we're not inside the document? Yes, that's the reason. > Why do we not do that? The editable descendant count is only used for nodes in the document so it wasn't necessary to keep track of it for out-of-document content and it means less work when mutating a detached subtree, but you're right, it sucks when inserting a subtree with many contenteditable nodes. There a trade-off here and it's not clear to me if one way is better than the other. I'd fine with changing the approach if you think it's better. > > Also, we don't seem to be checking for anonymous content here in BindToTree. > Should we be? In the current patch, the editable descendant count is updated in a pretty simple way that happens to work with anonymous content. With the other approach I will probably have to do a check for anonymous content like I do in UnbindFromTree.

> the editable descendant count is updated in a pretty simple way that happens to work with > anonymous conten I don't understand how that works... adding anonymous content that's editable will update the count on the parent, but removing it won't, or am I missing something? For the main non-anonymous case, maybe we can have our cake and eat it too as follows: 1) A node being bound is generally responsible for updating only its own count, unless the BindToTree call changes its parent. In that case it's responsible for updating its ancestors. 2) The update happens after binding kids (or while doing it). Basically, either after binding each kid (preferred, imo) or in a separate walk over the list after binding them all, add to our count the counts of those kids (plus 1 if the kid is editable). If our parent is changing, after updating our count propagate it up the tree (again, plus 1 if we ourselves are editable). Does that make sense?

This issue is causing major problems for our users utilizing our Content Management System. Our current solution for them is to have them use Chrome. It would be a huge affect if the users at our institution all stop using firefox because of issues like this. We need to be sure that we have a stable recommended browser. If a fix isn't introduced soon, we will switch our software requirements. Please update us as to the status of this bug.

The issue will be fixed on August 11 with Firefox 40, but if you are deploying Firefox in your institution, consider using the stable Extended Support Release (ESR) instead: https://www.mozilla.org/en-US/firefox/organizations/

Firefox 38 ESR is not affected by the issue.

(In reply to Boris Zbarsky [:bz] from comment #32) > > the editable descendant count is updated in a pretty simple way that happens to work with > > anonymous conten > > I don't understand how that works... adding anonymous content that's > editable will update the count on the parent, but removing it won't, or am I > missing something? In UnbindFromTree, the patch is using aNullParent as a signal that it is the root of the subtree being unbound. But for anonymous content it works slightly differently, if a binding parent is unbound, it gets unbound with aNullParent == true, and then the anonymous roots also get called with aNullParent == true, but at that point the (binding) parent is already removed from the document and we shouldn't be trying to update its editable descendant count. We don't need the check in BindToTree because the signals I used to detect the subtree root (going from no parent to a new parent) seems to work fine. I've updated the patch to the approach in comment 32.

Comment on attachment 8639392 [details] [diff] [review] Part 3: Keep track of editable descendants per node and prevent NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. v2 >+ if (aContent->IsHTMLElement()) { >+ nsGenericHTMLElement* htmlElem = static_cast<nsGenericHTMLElement*> (aContent); auto htmlElem = nsGenericHTMLElement::FromContent(aContent); if (htmlElem) { >+ return isEditable ? 1 : 0 + EditableDescendantCount(); '+' has higher precedence than "?:" in C/C++, so you need parens around the ?:-expression, no? Or better yet: return EditableDescendantCount() + isEditable; >+ bool hadParent = !!GetParent(); Do we not need to worry about the case of an HTML element that has a non-HTML child inserted that has an HTML editable descendant? Why not? >+ uint32_t editableDescendantChange = InclusiveEditableDescendantCount(); Is there a reason to do this check before checking !hadParent? The latter check should be cheaper, and false quite often. I still don't understand the anon content thing. It looks like when anon content is unbound we don't update the ancestor editable counts. Why is that ok? >+ uint32_t InclusiveEditableDescendantCount(); Document.

(In reply to Boris Zbarsky [:bz] from comment #37) > Comment on attachment 8639392 [details] [diff] [review] > Part 3: Keep track of editable descendants per node and prevent > NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. v2 > Do we not need to worry about the case of an HTML element that has a > non-HTML child inserted that has an HTML editable descendant? Why not? I've moved all the book keeping code into Element and added a test. > I still don't understand the anon content thing. It looks like when anon > content is unbound we don't update the ancestor editable counts. Why is > that ok? I've changed to check to something that makes more sense and isn't anonymous content specific (check if the parent is in the uncomposed document before trying to update the counts in the ancestors). All other review comments addressed. The only other change is that I renamed InclusiveEditableDescendantCount -> EditableInclusiveDescendantCount to better match language used in specs.

Comment on attachment 8640168 [details] [diff] [review] Part 3: Keep track of editable descendants per node and prevent NS_STYLE_USER_SELECT_ALL selection for nodes with editable descendants. v3 Thank you. This makes a lot more sense. r=me

ping?

Comment on attachment 8630777 [details] [diff] [review] Part 1: Add support for -moz-user-select: -moz-text > // For instance, if the frame hierarchy is: >- // AUTO -> _MOZ_ALL -> NONE -> TEXT, the returned value is _MOZ_ALL >- // TEXT -> NONE -> AUTO -> _MOZ_ALL, the returned value is TEXT >- // _MOZ_ALL -> TEXT -> AUTO -> AUTO, the returned value is _MOZ_ALL >- // AUTO -> CELL -> TEXT -> AUTO, the returned value is TEXT >+ // AUTO -> _MOZ_ALL -> NONE -> TEXT, the returned value is _MOZ_ALL >+ // AUTO -> _MOZ_ALL -> NONE -> _MOZ_TEXT, the returned value is _MOZ_TEXT. >+ // TEXT -> NONE -> AUTO -> _MOZ_ALL, the returned value is TEXT >+ // _MOZ_ALL -> TEXT -> AUTO -> AUTO, the returned value is _MOZ_ALL >+ // _MOZ_ALL -> _MOZ_TEXT -> AUTO -> AUTO, the returned value is _MOZ_TEXT. >+ // AUTO -> CELL -> TEXT -> AUTO, the returned value is TEXT Seems like maybe this comment shouldn't have the "_MOZ_" after "the returned value is" given this code below: > // convert internal values to standard values >- if (selectStyle == NS_STYLE_USER_SELECT_AUTO) >+ if (selectStyle == NS_STYLE_USER_SELECT_AUTO || >+ selectStyle == NS_STYLE_USER_SELECT_MOZ_TEXT) > selectStyle = NS_STYLE_USER_SELECT_TEXT; > else > if (selectStyle == NS_STYLE_USER_SELECT_MOZ_ALL) > selectStyle = NS_STYLE_USER_SELECT_ALL; r=dbaron

Comment on attachment 8630779 [details] [diff] [review] Part 2: Mark editable regions inside non-editable regions as -moz-user-select: -moz-text; r=dbaron This seems like an awfully slow selector, since we'll have to test it on every element, and walk up the ancestors of anything that's :-moz-read-write to look for an ancestor that's :-moz-read-only. From reading Element::IntrinsicState it looks like :-moz-read-write and :moz-read-only are actually opposites (or are there other places that do more interesting things and match neither or both?), which means that we might be able to do better. Is it possible to use a child combinator instead? That would mean a lot less searching of ancestors in the selector matching process. With this selector, the current behavior is that we'll walk up the ancestor chain of every :-moz-read-write HTML element until we find a :-moz-read-only element or the root. (Maybe that could be a followup if you want this on branches, though.) It would also mean that this sheet would be overriding -moz-user-select on fewer elements. However, given the explicit *|* is only on the first piece and not the second, it would do weird things in documents that are partly non-HTML, since the selector is implicitly (right now): *|*:-moz-read-only html|*:-moz-read-write (Is that namespace behavior intentional?) I guess I'm ok with this for now if we need it to fix a regression (sorry, didn't realize the review request was urgent), but I think it's worth considering what these selectors should be more carefully.

Comment on attachment 8630779 [details] [diff] [review] Part 2: Mark editable regions inside non-editable regions as -moz-user-select: -moz-text; r=dbaron Actually, given that we've addressed this for aurora/beta via backout, I'd like to understand this selector a little better. Tentatively changing to minus, although definitely willing to reconsider given discussion.

Interestingly, though, if you repaint the whole thing by using a display:none and then resetting the display property (for example), it fixes itself. In this specific file, I'm reusing one I'm trying to build for a different test but I think you can see it working. How to test: 1. Open the page. 2. Try selecting the TextNode below "Code topline". 3. Fail to select the TextNode, the whole cyan block is selected. 4. Click the "Fix iframe" button. 5. Clicking goes back to normal. OR 1. Open the page. 2. Click the "Fix iframe" button. 3. Nothing seems wrong or working in a wrong way (in the realm of this bug) (tested on FF 39.0.3)

AFAIK, :-moz-read-write and :-moz-read-only are opposites. William, do you have cycles to address comment 43? If not, I can take a look next week.

Yeah, I'll look into this today.

(In reply to David Baron [:dbaron] ⏰UTC-4 (busy Aug. 8-Aug. 30) from comment #43) > Comment on attachment 8630779 [details] [diff] [review] > Part 2: Mark editable regions inside non-editable regions as > -moz-user-select: -moz-text; r=dbaron > > Is it possible to use a child combinator instead? Yes, it doesn't look like it was necessary to override -moz-user-select on all the -moz-read-write children. I've updated the patch to use the child combinator. Although, this change means that a frame from a -moz-read-write descendant may end up returning a different value in nsFrame::IsSelectable if one of its ancestors before reaching the -moz-read-only element has set a different -moz-user-select value, but I think that's OK, and it may make more sense in such a case. I also suspect that the rule above in the style sheet can be switched to using a child selector as well. > However, given the explicit *|* is only > on the first piece and not the second, it would do weird things in documents > that are partly non-HTML, since the selector is implicitly (right now): > *|*:-moz-read-only html|*:-moz-read-write > (Is that namespace behavior intentional?) I'm not sure if it's intentional, but now that the rule has been changed to use the child combinator, it seems to be correct because the rule should only match when a child changes its editable state to be different than its parent which should only happen for HTML elements. Non-HTML elements just inherit the same editable state from its parent.

Comment on attachment 8646226 [details] [diff] [review] Part 2: Mark editable regions inside non-editable regions as -moz-user-select: -moz-text v2 Review of attachment 8646226 [details] [diff] [review]: ----------------------------------------------------------------- roc: Can you take over the review? Also, we missed the train for 42, we'll need to uplift the backouts or land + uplift the patches in this bug.

Comment on attachment 8646226 [details] [diff] [review] Part 2: Mark editable regions inside non-editable regions as -moz-user-select: -moz-text v2 ok, r=dbaron, but could you file a followup bug on converting the previous selector from a descendant combinator to a child combinator?

(In reply to William Chen [:wchen] from comment #49) > Also, we missed the train for 42, we'll need to uplift the backouts or land > + uplift the patches in this bug. I think we should uplift the backouts for 42.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfbfd2aeea9 https://hg.mozilla.org/integration/mozilla-inbound/rev/1af7a6cd6a66 https://hg.mozilla.org/integration/mozilla-inbound/rev/9bea8fc16ae2

https://hg.mozilla.org/mozilla-central/rev/ccfbfd2aeea9 https://hg.mozilla.org/mozilla-central/rev/1af7a6cd6a66 https://hg.mozilla.org/mozilla-central/rev/9bea8fc16ae2

Backed out on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/53e23098346e

Is this related to https://bugzilla.mozilla.org/show_bug.cgi?id=1193517?

(In reply to brunoais from comment #55) > Is this related to https://bugzilla.mozilla.org/show_bug.cgi?id=1193517? I don't know why you think that.