Closed Bug 712600 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: aharon, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

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.
Blocks: html5bidi
Attachment #583449 - Attachment mime type: text/plain → text/html
Attachment #583450 - Attachment mime type: text/plain → text/html
Attachment #583451 - Attachment mime type: text/plain → text/html
Attached patch w-i-p patch (obsolete) — Splinter Review
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.
Attached file Tests from bug 710694
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.
(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 :-(
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?
IMO, it's worthwhile, unless the full fix is really around the corner.
Attachment #587958 - Flags: review?(ehsan)
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!
Attachment #587958 - Flags: review?(ehsan) → review+
Comment on attachment 587958 [details] [diff] [review]
w-i-p patch

Rob, can you take a look when you have time?
Attachment #587958 - Flags: review?(roc)
Can you summarize the changes, preferably in the patch log message?
Assignee: nobody → smontagu
Attachment #587958 - Attachment is obsolete: true
Attachment #587958 - Flags: review?(roc)
Attachment #605543 - Flags: review?(roc)
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
Attachment #605543 - Attachment is obsolete: true
Attachment #605543 - Flags: review?(roc)
Attachment #607330 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c771785221
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2cb3d55cb4
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/fb2cb3d55cb4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 775687
Depends on: 779003
You need to log in before you can comment on or make changes to this bug.