712600 - incorrect rendering of opposite-direction run with a <bdi> inside it

Status: RESOLVED FIXED

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

Description

[Aharon (Vladimir) Lanin]

For the purposes of applying the bidirectional algorithm to a paragraph-level container containing a unicode-bidi:isolate element like <bdi>, the isolate element is supposed to be treated like a neutral character.

Thus, in <div dir=rtl>a <bdi dir=rtl>&#x05D0;</bdi> b</div> (try it at data:text/html,%3Cdiv%20dir%3Drtl%3Ea%20%3Cbdi%20dir%3Drtl%3E%26%23x05D0%3B%3C%2Fbdi%3E%20b%3C%2Fdiv%3E), the result is supposed to be displayed as
a א b
because the a and the b should form an LTR run with the whitespace and the <bdi> (i.e. neutral) between them. Instead, the result in 11.0a1 is instead:
 bאa 

In other words, while the directional run is formed (note that the spaces are laid out LTR, the part before the <bdi> is displayed where the part after the <bdi> belongs and vice-versa. The proper display is only achieved when the <bdi> gets dir=ltr (which certainly should not affect the display of the content around the <bdi>).

I am attaching a test case being submitted to the HTML5 test suite in which this was discovered, as well as two more test cases that are also failing probably due to the same bug.

Comment 1

[Aharon (Vladimir) Lanin]

Attachment: test case (being submitted to HTML5 test suite)

Comment 2

[Aharon (Vladimir) Lanin]

Attachment: a more complicated test case (nested <bdi>s) - may be related

Comment 3

[Aharon (Vladimir) Lanin]

Attachment: a more complicated test case (wrapped <bdi>) - may be related

Comment 4

[Simon Montagu :smontagu]

Attachment: w-i-p patch

This patch fixes the testcases attached here. It does NOT fix all the testcases in unicode-bidi-isolate-basic.html and unicode-bidi-isolate-aharon.html from bug 706194. I'll attach samples of failing tests.

Comment 5

[Simon Montagu :smontagu]

Attachment: Tests from bug 710694

Comment 6

[Aharon (Vladimir) Lanin]

I have created a separate bug, https://bugzilla.mozilla.org/show_bug.cgi?id=717811, that was exposed by the test case from bug 706194. Actually, it turns out that it was also exposed by other tests we are submitting to the W3C's HTML5 tests suite, which I have attached to 717811. I don't know how I overlooked them failing.

Comment 7

[Aharon (Vladimir) Lanin]

(In reply to Simon Montagu from comment #4)
> Created attachment 587958 [details] [diff] [review]
> w-i-p patch
> 
> This patch fixes the testcases attached here. It does NOT fix all the
> testcases in unicode-bidi-isolate-basic.html and
> unicode-bidi-isolate-aharon.html from bug 706194. I'll attach samples of
> failing tests.

Is this patch ready for review? Can anyone review it? This bug makes <bdi> almost impossible to use :-(

Comment 8

[Simon Montagu :smontagu]

I was holding off on review request because fixing the remaining issues might require a complete rewrite of the patch. Aharon, do you think this is worth checking in in spite of that?

Comment 9

[Aharon (Vladimir) Lanin]

IMO, it's worthwhile, unless the full fix is really around the corner.

Comment 10

[(no longer active)]

Comment on attachment 587958 [details] [diff] [review]
w-i-p patch

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

This looks good overall, but I'm not too comfortable reviewing the nsBidiPresUtils.cpp parts.  Simon, can you please ask someone else to take a look at those changes as well?  Thanks!

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 587958 [details] [diff] [review]
w-i-p patch

Rob, can you take a look when you have time?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you summarize the changes, preferably in the patch log message?

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch with changes summarized in checkin comment

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 605543 [details] [diff] [review]
Patch with changes summarized in checkin comment

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

::: layout/base/nsBidiPresUtils.cpp
@@ +415,5 @@
>  }
>  
>  static nsresult
> +SplitInlineAncestors(nsIFrame*     aFrame,
> +                     bool          splitBefore)

Might as well fix indent while you're here.

Better rename this parameter to something more meangingful, and something starting with 'a'. It would also be good if this was an enum {BEFORE, AFTER} instead of a bool. Actually though, why not just pass the frame to split after directly as the parameter here?

@@ +433,5 @@
>      if (NS_FAILED(rv)) {
>        return rv;
>      }
>      
>      // Split the child list after |frame|.

"before or after"

@@ +865,5 @@
>              child = parent;
>              parent = child->GetParent();
>            }
>            if (parent && IsBidiSplittable(parent))
> +            SplitInlineAncestorsAfter(child);

{} around the if body, while you're here

@@ +1209,3 @@
>  }
> +nsBidiLevel
> +nsBidiPresUtils::GetFrameEmbeddingLevel(nsIFrame* aFrame)

need blank line

Comment 15

[Simon Montagu :smontagu]

Attachment: Addressed comments

Comment 16

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c771785221
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2cb3d55cb4

Comment 17

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/fb2cb3d55cb4

Comment 18

[Mounir Lamouri (:mounir)]

and tests:
https://hg.mozilla.org/mozilla-central/rev/d6c771785221