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

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: Aharon (Vladimir) Lanin, Assigned: smontagu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 613154
(Reporter)

Comment 1

6 years ago
Created attachment 583449 [details]
test case (being submitted to HTML5 test suite)
(Reporter)

Comment 2

6 years ago
Created attachment 583450 [details]
a more complicated test case (nested <bdi>s) - may be related
(Reporter)

Comment 3

6 years ago
Created attachment 583451 [details]
a more complicated test case (wrapped <bdi>) - may be related
(Assignee)

Updated

6 years ago
Attachment #583449 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

6 years ago
Attachment #583450 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

6 years ago
Attachment #583451 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 587960 [details]
Tests from bug 710694
(Reporter)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

5 years ago
IMO, it's worthwhile, unless the full fix is really around the corner.
(Assignee)

Updated

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

Comment 11

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

Comment 13

5 years ago
Created attachment 605543 [details] [diff] [review]
Patch with changes summarized in checkin comment
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
(Assignee)

Comment 15

5 years ago
Created attachment 607330 [details] [diff] [review]
Addressed comments
Attachment #605543 - Attachment is obsolete: true
Attachment #605543 - Flags: review?(roc)
Attachment #607330 - Flags: review?(roc)
Attachment #607330 - Flags: review?(roc) → review+
(Assignee)

Comment 16

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
and tests:
https://hg.mozilla.org/mozilla-central/rev/d6c771785221

Updated

5 years ago
Depends on: 775687

Updated

5 years ago
Depends on: 779003
Depends on: 793233
You need to log in before you can comment on or make changes to this bug.