Last Comment Bug 718236 - Inconsistent layout with bidi, removing span that separates numerals
: Inconsistent layout with bidi, removing span that separates numerals
Status: RESOLVED FIXED
[qa+]
: testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Simon Montagu :smontagu
:
:
Mentors:
: 730671 (view as bug list)
Depends on: 722617 745991
Blocks: stirdom refdyn 731594
  Show dependency treegraph
 
Reported: 2012-01-14 14:58 PST by Jesse Ruderman
Modified: 2012-04-16 15:43 PDT (History)
9 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
testcase (clicking should have no effect) (537 bytes, text/html)
2012-01-14 14:58 PST, Jesse Ruderman
no flags Details
Patch (1.57 KB, patch)
2012-01-25 07:26 PST, Simon Montagu :smontagu
roc: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Reftests (3.60 KB, patch)
2012-01-25 07:27 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-01-14 14:58:46 PST
Created attachment 588694 [details]
testcase (clicking should have no effect)

Based on the reftest for bug 588739.
Comment 1 Simon Montagu :smontagu 2012-01-16 02:46:22 PST
I'm not sure if this is a bug in bidi resolution or in frame destruction. We start off with a frame tree like this:
  line 0x121e0b4b8: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x41] {0,0,0,0} <
    Text(0)"\u062a"@0x121e0b278 [run=0x0][0,1,T]  next=0x121e0b2e8 {0,0,0,0} [state=0000000010000402] [content=0x12614f780] sc=0x121e0ae20 pst=:-moz-non-element
    Inline(span)(1)@0x121e0b2e8 {0,0,0,0} [state=0000000000000402] [content=0x12614f800] [sc=0x121e0b098]<
      Text(0)"1"@0x121e0b360 [run=0x0][0,1,T]  next=0x121e0b3d0 {0,0,0,0} [state=0000000000000402] [content=0x12614f880] sc=0x121e0b138 pst=:-moz-non-element
      Inline(span)(1)@0x121e0b3d0 next=0x121e0b448 {0,0,0,0} [state=0000000000000402] [content=0x12614f900] [sc=0x121e0b1d8]<>
      Text(2)"3"@0x121e0b448 [run=0x0][0,1,T]  {0,0,0,0} [state=0000000000000402] [content=0x12614fa00] sc=0x121e0b138 pst=:-moz-non-element
    >
  >

and then after bidi resolution it looks like this, with the outer Inline(span) split into three because its three subframes have different directions (the empty span is treated as a neutral character, and so is RTL between the two LTR digits, by rule N1 of the UBA)
  line 0x121e0b4b8: count=4 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x41] {0,0,0,0} <
    Text(0)"\u062a"@0x121e0b278 [run=0x0][0,1,T]  next=0x121e0b2e8 {0,0,0,0} [state=0000000010020402] [content=0x12614f780] sc=0x121e0ae20 pst=:-moz-non-element
    Inline(span)(1)@0x121e0b2e8 next=0x121e0b7f8 next-continuation=0x121e0b7f8 {0,0,0,0} [state=0000000000000402] [content=0x12614f800] [sc=0x121e0b098]<
      Text(0)"1"@0x121e0b360 [run=0x0][0,1,T]  {0,0,0,0} [state=0000000000020402] [content=0x12614f880] sc=0x121e0b138 pst=:-moz-non-element
    >
    Inline(span)(1)@0x121e0b7f8 next=0x121e0b870 prev-continuation=0x121e0b2e8 next-continuation=0x121e0b870 {0,0,0,0} [state=0000000000000402] [content=0x12614f800] [sc=0x121e0b098]<
      Inline(span)(1)@0x121e0b3d0 {0,0,0,0} [state=0000000000000402] [content=0x12614f900] [sc=0x121e0b1d8]<>
    >
    Inline(span)(1)@0x121e0b870 prev-continuation=0x121e0b7f8 {0,0,0,0} [state=0000000000000402] [content=0x12614f800] [sc=0x121e0b098]<
      Text(2)"3"@0x121e0b448 [run=0x0][0,1,T]  {0,0,0,0} [state=0000000000020402] [content=0x12614fa00] sc=0x121e0b138 pst=:-moz-non-element
    >
  >

Now, when the empty span is removed, its containing span is not removed, so we have a frame tree like this:
  line 0x121e0b4b8: count=4 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x141] {0,0,1574,1260} vis-overflow={-60,0,1694,1260} scr-overflow={0,0,1574,1260} <
    Text(0)"\u062a"@0x121e0b278 [run=0x12614fc80][0,1,T]  next=0x121e0b2e8 {960,0,614,1260} [state=00000000d0220000] [content=0x12614f780] [vis-overflow=-60,0,734,1260] [scr-overflow=0,0,614,1260] sc=0x121e0ae20 pst=:-moz-non-element
    Inline(span)(1)@0x121e0b2e8 next=0x121e0b7f8 next-continuation=0x121e0b7f8 {480,180,480,960} [state=0000000000a00000] [content=0x12614f800] [vis-overflow=-60,0,600,960] [scr-overflow=0,0,480,960] [sc=0x121e0b098]<
      Text(0)"1"@0x121e0b360 [run=0x12614fd00][0,1,T]  {0,0,480,960} [state=00000000c0020000] [content=0x12614f880] [vis-overflow=-60,0,600,960] [scr-overflow=0,0,480,960] sc=0x121e0b138 pst=:-moz-non-element
    >
    Inline(span)(1)@0x121e0b7f8 next=0x121e0b870 prev-continuation=0x121e0b2e8 next-continuation=0x121e0b870 {480,900,0,0} [state=0000000000201000] [content=0x12614f800] [sc=0x121e0b098]<>
    Inline(span)(1)@0x121e0b870 prev-continuation=0x121e0b7f8 {0,180,480,960} [state=0000000000600000] [content=0x12614f800] [vis-overflow=-60,0,600,960] [scr-overflow=0,0,480,960] [sc=0x121e0b098]<
      Text(1)"3"@0x121e0b448 [run=0x12614fd00][0,1,T]  {0,0,480,960} [state=00000000c0420000] [content=0x12614fa00] [vis-overflow=-60,0,600,960] [scr-overflow=0,0,480,960] sc=0x121e0b138 pst=:-moz-non-element
    >
  >

... and the empty container (0x121e0b7f8 in this example) functions exactly like the empty span, as a neutral character separating the two digits.

So either the empty container should be ignored in bidi resolution (though I don't know what criterion distinguishes it from the empty span), OR when destroying a frame leaves an empty parent which either is or has a non-fluid continuation, then the parent should be destroyed also. (OR some variation of the above ;-) )
Comment 2 :Ehsan Akhgari 2012-01-16 12:03:28 PST
Shouldn't removing the empty span retrigger bidi resolution?  I think that would fix this bug...
Comment 3 Simon Montagu :smontagu 2012-01-16 12:41:33 PST
No, it already does. Was that not clear from my previous comment?
Comment 4 :Ehsan Akhgari 2012-01-16 13:22:23 PST
Hmm, maybe it's me who's confused.  If we do a new bidi resolution, where is the 2nd broken span come from after the second bidi resolution?
Comment 5 Simon Montagu :smontagu 2012-01-16 13:43:47 PST
I'm not sure what you mean by "the 2nd broken span". The last frame tree in comment 1 is the input to the 2nd bidi resolution, and when I said "the empty container (0x121e0b7f8 in this example) functions exactly like the empty span...", I mean that in the second bidi resolution, the empty container is treated the same as the empty span (0x121e0b3d0) in the first frame tree is treated in the first bidi resolution.
Comment 6 :Ehsan Akhgari 2012-01-16 16:55:02 PST
Ah, OK, I see.  What I meant to say is that 0x121e0b7f8 is a frame created because of bidi-resolution.  If the frame tree changes so that it no longer has any children, there should be no way that frame is going to affect bidi resolution (I think).  If that last sentence is correct, then I think we should remove this frame *before* passing the frame tree to the next bidi resolution phase.
Comment 7 Simon Montagu :smontagu 2012-01-25 07:26:54 PST
Created attachment 591460 [details] [diff] [review]
Patch

nsCSSFrameConstructor is slightly unfamiliar territory for me, but this seems to work.
Comment 8 Simon Montagu :smontagu 2012-01-25 07:27:33 PST
Created attachment 591461 [details] [diff] [review]
Reftests
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-25 19:01:47 PST
Comment on attachment 591460 [details] [diff] [review]
Patch

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

r+ with that

::: layout/base/nsCSSFrameConstructor.cpp
@@ +9046,5 @@
>  
> +  // Reconstruct if inflowFrame is parent's only child, and parent is, or has,
> +  // a non-fluid continuation, i.e. it was split by bidi resolution
> +  if (inFlowFrame == parent->GetLastChild(kPrincipalList) &&
> +      inFlowFrame == parent->GetFirstPrincipalChild() &&

!inFlowFrame->GetPrevSibling() && !inFlowFrame->GetNextSibling()?
Comment 12 Simon Montagu :smontagu 2012-02-28 01:08:30 PST
Comment on attachment 591460 [details] [diff] [review]
Patch

[Approval Request Comment]
We should take this on beta, because it fixes bug 730671, which is a bad regression.
Regression caused by (bug #): 698706
User impact if declined: Many comments on Facebook in RTL languages will be unreadable.
Testing completed (on m-c, etc.): This patch has been on trunk and aurora for almost a month with no known regressions.
Risk to taking this patch (and alternatives if risky): Minimal
Comment 13 Alex Keybl [:akeybl] 2012-02-28 15:03:26 PST
Comment on attachment 591460 [details] [diff] [review]
Patch

[Triage Comment]
Regression in Facebook - let's land for Beta 11 and get QA testing around bidirectional text. We'll have the opportunity to back this out depending on testing and beta feedback.
Comment 14 Jonathan Kew (:jfkthame) 2012-02-28 15:10:54 PST
Thanks for approval - pushed this to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/3b32861a5cf7
Comment 15 Jonathan Kew (:jfkthame) 2012-02-28 15:18:38 PST
*** Bug 730671 has been marked as a duplicate of this bug. ***
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-01 13:50:14 PST
Removing qawanted since this is fixed -- will be verified in Firefox 11.0b6
Comment 17 Virgil Dicu [:virgil] [QA] 2012-03-07 02:23:57 PST
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

Verified using testcase in comment 0 on Mac OS 10.6 in Firefox 11 beta 6. Clicking has no effect now, the characters are displayed as expected.

Note You need to log in before you can comment on or make changes to this bug.