The default bug view has changed. See this FAQ.

Inconsistent layout with bidi, removing span that separates numerals

RESOLVED FIXED in Firefox 11

Status

()

Core
Layout: Text
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: smontagu)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla12
x86_64
Mac OS X
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11+ verified)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 588694 [details]
testcase (clicking should have no effect)

Based on the reftest for bug 588739.
(Assignee)

Comment 1

5 years ago
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...
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 591460 [details] [diff] [review]
Patch

nsCSSFrameConstructor is slightly unfamiliar territory for me, but this seems to work.
Attachment #591460 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 591461 [details] [diff] [review]
Reftests
Assignee: nobody → smontagu
Attachment #591461 - Flags: review?(roc)
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()?
Attachment #591460 - Flags: review?(roc) → review+
Attachment #591461 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29c4f349276
https://hg.mozilla.org/integration/mozilla-inbound/rev/33b309643e39
Flags: in-testsuite+
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/f29c4f349276
https://hg.mozilla.org/mozilla-central/rev/33b309643e39
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 722617
(Reporter)

Updated

5 years ago
Blocks: 373610
(Assignee)

Comment 12

5 years ago
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
Attachment #591460 - Flags: approval-mozilla-beta?
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.
Attachment #591460 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Keywords: qawanted
Thanks for approval - pushed this to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/3b32861a5cf7
Duplicate of this bug: 730671

Updated

5 years ago
Blocks: 731594

Updated

5 years ago
status-firefox11: --- → fixed
tracking-firefox11: --- → +
Removing qawanted since this is fixed -- will be verified in Firefox 11.0b6
Keywords: qawanted
Whiteboard: [qa+]
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.
status-firefox11: fixed → verified
Depends on: 745991
You need to log in before you can comment on or make changes to this bug.