Closed Bug 1103348 Opened 5 years ago Closed 4 years ago

Directionality of dir=auto element is not updated when replacing strong-directional contents with all-neutral characters after Select All

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
blocking-b2g 2.2+
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jfkthame, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(2 files)

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!
Keywords: regression
(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.
Blocks: 1161807
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.
Flags: needinfo?(smontagu)
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.
Flags: needinfo?(smontagu)
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
I think I may have a fix for this.  I'm going to test it on the try server.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
No longer blocks: 1161807
Duplicate of this bug: 1161807
Blocks: 1156200
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?
Flags: needinfo?(mhenretty)
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.
Attachment #8603129 - Flags: review?(smontagu) → review+
Attached patch Fuller reftestSplinter Review
Attachment #8603176 - Flags: review?(ehsan)
Comment on attachment 8603176 [details] [diff] [review]
Fuller reftest

Review of attachment 8603176 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  I'll land this with my patch.
Attachment #8603176 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/a54020a22808
https://hg.mozilla.org/mozilla-central/rev/8f67276f2c37
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.
Flags: needinfo?(mhenretty)
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.
Flags: needinfo?(pcheng)
Keywords: verifyme
Duplicate of this 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.
Attachment #8603129 - Flags: approval-mozilla-b2g37?
(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:
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(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
blocking-b2g: --- → 2.2+
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.
Attachment #8603129 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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.
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pcheng) → needinfo?(ktucker)
Keywords: verifyme
Jonathan, can you verify that the originally written issue here as been fixed?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(jfkthame)
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.
Flags: needinfo?(jfkthame)
Blocks: 1169267
Depends on: 1377826
You need to log in before you can comment on or make changes to this bug.