Based on the reftest for bug 588739.

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 ;-) )

Shouldn't removing the empty span retrigger bidi resolution? I think that would fix this bug...

No, it already does. Was that not clear from my previous comment?

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?

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.

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.

nsCSSFrameConstructor is slightly unfamiliar territory for me, but this seems to work.

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()?

https://hg.mozilla.org/integration/mozilla-inbound/rev/f29c4f349276 https://hg.mozilla.org/integration/mozilla-inbound/rev/33b309643e39

https://hg.mozilla.org/mozilla-central/rev/f29c4f349276 https://hg.mozilla.org/mozilla-central/rev/33b309643e39

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 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.

Thanks for approval - pushed this to mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/3b32861a5cf7

Removing qawanted since this is fixed -- will be verified in Firefox 11.0b6

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.