Last Comment Bug 712600 - incorrect rendering of opposite-direction run with a <bdi> inside it
: incorrect rendering of opposite-direction run with a <bdi> inside it
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 775687 779003 793233
Blocks: html5bidi
  Show dependency treegraph
 
Reported: 2011-12-21 04:10 PST by Aharon (Vladimir) Lanin
Modified: 2012-11-05 08:54 PST (History)
9 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (being submitted to HTML5 test suite) (2.69 KB, text/html)
2011-12-21 04:16 PST, Aharon (Vladimir) Lanin
no flags Details
a more complicated test case (nested <bdi>s) - may be related (2.62 KB, text/html)
2011-12-21 04:19 PST, Aharon (Vladimir) Lanin
no flags Details
a more complicated test case (wrapped <bdi>) - may be related (3.73 KB, text/html)
2011-12-21 04:19 PST, Aharon (Vladimir) Lanin
no flags Details
w-i-p patch (23.13 KB, patch)
2012-01-11 23:24 PST, Simon Montagu :smontagu
ehsan: review+
Details | Diff | Splinter Review
Tests from bug 710694 (1.20 KB, text/html)
2012-01-11 23:36 PST, Simon Montagu :smontagu
no flags Details
Patch with changes summarized in checkin comment (23.91 KB, patch)
2012-03-13 14:33 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Addressed comments (24.16 KB, patch)
2012-03-19 15:05 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review

Description Aharon (Vladimir) Lanin 2011-12-21 04:10:25 PST
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 2011-12-21 04:16:49 PST
Created attachment 583449 [details]
test case (being submitted to HTML5 test suite)
Comment 2 Aharon (Vladimir) Lanin 2011-12-21 04:19:02 PST
Created attachment 583450 [details]
a more complicated test case (nested <bdi>s) - may be related
Comment 3 Aharon (Vladimir) Lanin 2011-12-21 04:19:58 PST
Created attachment 583451 [details]
a more complicated test case (wrapped <bdi>) - may be related
Comment 4 Simon Montagu :smontagu 2012-01-11 23:24:52 PST
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.
Comment 5 Simon Montagu :smontagu 2012-01-11 23:36:44 PST
Created attachment 587960 [details]
Tests from bug 710694
Comment 6 Aharon (Vladimir) Lanin 2012-01-12 17:46:35 PST
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 2012-02-04 23:47:48 PST
(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 2012-02-07 11:12:30 PST
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 2012-02-07 12:01:46 PST
IMO, it's worthwhile, unless the full fix is really around the corner.
Comment 10 :Ehsan Akhgari 2012-03-01 12:59:08 PST
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 2012-03-01 13:01:41 PST
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) (Exited; email my personal email if necessary) 2012-03-08 14:55:38 PST
Can you summarize the changes, preferably in the patch log message?
Comment 13 Simon Montagu :smontagu 2012-03-13 14:33:06 PDT
Created attachment 605543 [details] [diff] [review]
Patch with changes summarized in checkin comment
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 16:30:26 PDT
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 2012-03-19 15:05:44 PDT
Created attachment 607330 [details] [diff] [review]
Addressed comments
Comment 17 Mounir Lamouri (:mounir) 2012-03-20 03:48:45 PDT
https://hg.mozilla.org/mozilla-central/rev/fb2cb3d55cb4
Comment 18 Mounir Lamouri (:mounir) 2012-03-20 03:49:10 PDT
and tests:
https://hg.mozilla.org/mozilla-central/rev/d6c771785221

Note You need to log in before you can comment on or make changes to this bug.