STR: (1) In a new tab, load a simple dir=auto testcase: data:text/html,<div dir="ltr"><div contenteditable dir="auto" style="border:1px solid;padding:2px;width:200px;height:2em"></div></div> (2) Click in the contenteditable <div>, and type a few Hebrew or Arabic letters. Directionality becomes RTL, as expected. (3) From elsewhere (e.g. here), select and copy to the clipboard a typical phone-number string (not including any strong-directionality characters): +1(800)555-1212 (4) Click back in the (currently RTL) contenteditable <div>. Choose Select All (Cmd-A) to select the text you just typed. (Drag-selecting or double-clicking the text will NOT do.) (5) Paste the copied phone number over it. Note how the directionality incorrectly remains RTL. (6) Type an additional RTL letter; directionality remains RTL (as expected). (7) Backspace to remove the RTL letter just added; suddenly the directionality switches to LTR (as it should have been immediately it was pasted in).

I can't reproduce with these STR

Argh, sorry... it seems that it doesn't work if you copy/paste from the bugzilla comment, because you get some kind of rich-text copy. Try with a plain-text source, or try simply overtyping the RTL text with (western) digits after doing Select-All.

Braindump before I get on a plane, I'll continue tomorrow: Regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd4cf30428b0&tochange=e33c2011643e This appears to have been exposed by bug 894137, but the root cause is that, for some reason not clear to me, doing Select All and then typing new text makes the text node unbind from and then rebind to its parent, unlike doing double click on the text and then typing, which just sets the text node to an empty string and then sets the new text. The result of that difference is that in the second case TextNodeChangedDirection is called from nsGenericDOMDataNode::SetTextInternal but in the first case it isn't, so the direction doesn't get reset. In the case of new text with strong directionality, this is not such a problem, because we then call SetAncestorDirectionIfAuto from SetDirectionFromNewTextNode, but if the new text is neutral (dir != eDir_NotSet) that doesn't happen. I thought that adding if (dir == eDir_NotSet) { dir = eDir_LTR; } in SetDirectionFromNewTextNode, as in SetDirectionalityFromValue, might help, but that causes regressions in a case like <div dir="auto"><span>12345</span><span>אבגדה</span></div> -- when the second text node gets added the direction is "already set" to LTR, so it doesn't get reset to RTL as it should. Care will be needed for a solution here not to regress a solution to bug 1103011 and vice versa!

(In reply to Simon Montagu :smontagu from comment #3) > Braindump before I get on a plane, I'll continue tomorrow: > > Regression range > http://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=fd4cf30428b0&tochange=e33c2011643e > > This appears to have been exposed by bug 894137, but the root cause is that, > for some reason not clear to me, doing Select All and then typing new text > makes the text node unbind from and then rebind to its parent, unlike doing > double click on the text and then typing, which just sets the text node to > an empty string and then sets the new text. IIRC from my debugging session, this isn't strictly accurate -- it doesn't make the text node "unbind from and then rebind to its parent", rather, it unbinds the old text node (and destroys it, I presume), then creates a new one and binds that to the parent in its place. Presumably this is because Select All sets the range of the selection differently from double-clicking the text. Without having checked, I'm guessing the double-click gives a range that points to the text node, with start/end offsets of [0, textLength]; while Select All gives a range that points to the text node's parent, with start/end of [0, numChildren]. So deleting (or replacing) that range causes the child textnode(s) to be deleted, not just emptied.

Comment 3 is wrong: this is simply a regression from bug 894137. See especially bug 894137 comment 19: > I think this is a definite bug with dir=auto, since when tearing down the DOM > we don't need to reset the direction of ancestors of text nodes as we unbind > them. But in the scenario of this bug, we *do* need to reset the direction of ancestors of text nodes as we unbind them.

Simon, this blocks a 2.2 blocker. Is this fixable in the 2.2 timeline, and do we have someone to work on this? Conversely, are there any workarounds we can do in Gaia for bug 1156200? I guess we can always set textContent to a LTR character before setting the real textContent, but I was hoping there would be a better way.

I need help from someone with more expertise on the DOM side to fix this. I can see two possibilities: either improve the fix to bug 894137 or fix bug 905036 (which should mean that bug 894137 can be backed out without regression). A better fix for bug 894137 would require a reliable way to identify when a text node is unbound from the tree that its parent is also being unbound.

I think I may have a fix for this. I'm going to test it on the try server.

Seems to be fairly green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed5e448d5af

Michael, I'm not sure what I should do in order to get this fix to the right places for b2g once it lands on central. Do you know the details?

Comment on attachment 8603129 [details] [diff] [review] Correctly reset the direction of an ancestor that is still in the tree when a text node is removed Review of attachment 8603129 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the code changes. I'll attach an alternative reftest that I already made. ::: dom/base/DirectionalityUtils.cpp @@ +505,5 @@ > // and remove the text node from the map > nsINode* oldTextNode = static_cast<Element*>(aData); > Element* rootNode = aEntry->GetKey(); > nsINode* newTextNode = nullptr; > + if (oldTextNode && rootNode->GetParentNode() && rootNode->HasDirAuto()) { I don't think you need to retain the test for oldTextNode introduced in bug 894137.

Comment on attachment 8603176 [details] [diff] [review] Fuller reftest Review of attachment 8603176 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I'll land this with my patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a54020a22808 https://hg.mozilla.org/integration/mozilla-inbound/rev/8f67276f2c37

https://hg.mozilla.org/mozilla-central/rev/a54020a22808 https://hg.mozilla.org/mozilla-central/rev/8f67276f2c37

Thanks Ehsan, there should be nothing we need to do on the gaia side to benefit from this patch. Specifically, bug 1156200 should be just fixed now, so I am retesting that today.

Sorry, I think I misunderstood comment 11. If this does indeed fix bug 1156200, then we will want to uplift to mozilla-b2g37_v2_2. I'm having bug 1156200 retested by QA, and if it's fixed we will mark it as a duplicate of this one and request uplift here. https://bugzilla.mozilla.org/show_bug.cgi?id=1156200#c20

Adding verifyme to check 2.2 once it has been uplifted since this resolves bug 1156200.

(In reply to Michael Henretty [:mhenretty] from comment #18) > Sorry, I think I misunderstood comment 11. If this does indeed fix bug > 1156200, then we will want to uplift to mozilla-b2g37_v2_2. Yep, that's what I meant. :-)

Can we get this uplifted to 2.2 because 1156200 needs it.

Comment on attachment 8603129 [details] [diff] [review] Correctly reset the direction of an ancestor that is still in the tree when a text node is removed This blocks a blocker. Setting the approval flag to get some attention to this from the release drivers.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #23) > Comment on attachment 8603129 [details] [diff] [review] > Correctly reset the direction of an ancestor that is still in the tree when > a text node is removed > > This blocks a blocker. Setting the approval flag to get some attention to > this from the release drivers. Hi Ehsan, Please fill in approval request form. Thanks. NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Hi Ehsan, Just a soft reminder. Still waiting for the approval request form. Thanks

(In reply to Josh Cheng [:josh] from comment #25) > Hi Ehsan, > Just a soft reminder. > Still waiting for the approval request form. Thanks I'm not sure what to say there. This patch is not low risk, and as far as I know this bug has existed for as long as we have implemented dir=auto, so there is no regressing bug #. The user impact is in the blocker bug. FWIW, normally I would not submit this for uplift to Aurora/Beta for Firefox, but I don't know how strict b2g is in terms of the riskiness of the patches uplifted.

(to summarize, this is a risky patch which I wouldn't want to uplift for Firefox, but it blocks a b2g blocker, and I don't know how easy the other approaches to fixing the blocker bug by working around this in gaia are. Someone more familiar with the gaia side of things should probably provide a more detailed risk analysis.)

2.2+ As this blocks a blocker

Comment on attachment 8603129 [details] [diff] [review] Correctly reset the direction of an ancestor that is still in the tree when a text node is removed Approving and requesting QA verify. Please NI for blackout if this causing any issue on 2.2.

Josh, we don't have the ability to apply a Gecko patch. We can only test Gaia patches. I think you will need to needinfo someone with the ability to test Gecko patches to test this.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c16c18d97daa https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8ad517de41ac

Bug 1156200 is verified fixed on Flame 2.2. The '+' symbol is located on left side of phone number on the SMS notification in Arabic. Not marking this bug as verified because I don't know how to verify this bug. The STR does not look like it's for manual testing of a mobile device. Device: Flame (full flashed, 319MB, KK) BuildID: 20150522002508 Gaia: 9acbac7e6d4a2e3913af4aa202ea403501967fcd Gecko: a1e8f172523d Gonk: bd9cb3af2a0354577a6903917bc826489050b40d Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Jonathan, can you verify that the originally written issue here as been fixed?

I can no longer reproduce the issue exactly as described. However, I can still reproduce a glitch using slightly modified STR, so I think we still have some remaining bugginess in this area. I will file a followup bug with details.