177148 - Presence of hebrew character changes layout

Status: RESOLVED FIXED

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

Description

[Hixie (not reading bugmail)]

The only difference between these two test cases is that the second contains a
hebrew character:
http://www.hixie.ch/tests/adhoc/css/text/white-space/normal/line-breaking/bidi/001.html
http://www.hixie.ch/tests/adhoc/css/text/white-space/normal/line-breaking/bidi/002.html

They render differently.

Note: they only render differently because of another bug, so if this bug is not
fixed before the other one, new test cases will have to be made.

Comment 1

[Hixie (not reading bugmail)]

reassigning per smontagu

Comment 2

[Hixie (not reading bugmail)]

This testcase renders incorrectly. It has no hebrew characters. If I add one, it
renders correctly.
   http://www.hixie.ch/tests/adhoc/unicode/bidi/001.html

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch

This patch would fix this bug, but perhaps it is WONTFIX per
http://www.unicode.org/unicode/reports/tr20/#Bidi ?

Comment 4

[Hixie (not reading bugmail)]

The problem with using markup to do this is that sometimes there is no markup
which matches what you are doing, for example in this test case:
   http://www.hixie.ch/tests/adhoc/css/box/inline/bidi/001.html

Does the use of the 'direction' and 'unicode-embed' properties trigger bidi too?

Comment 5

[Simon Montagu :smontagu]

direction: rtl certainly does, and so does any HTML element with dir="rtl". I'm
not sure about unicode-bidi: embed. Testcases coming.

Comment 6

[Simon Montagu :smontagu]

Another interesting question is: at what point does the overhead of making all
these tests for content that needs bidi processing become higher than just doing
bidi processing for all documents? The ICU implementation of the Bidi algorithm
which we use is optimized to bail out very soon when all content has the same
directionality.

Comment 7

[Peter Kirk]

See also bug 288453, which is probably a special case of this and so can
probably be marked as a duplicate - but see its test cases. The problem there is
that the Unicode characters RLM, RLE and RLO are not taken as content triggering
bidi processing - although their entire intention is to control bidi processing.

Concerning comment #6, there must be a serious inefficiency if the system needs
to check an entire document, potentially megabytes, for bidi trigger characters
before it can even start to lay it out.

Comment 8

[Simon Montagu :smontagu]

(In reply to comment #7)

> Concerning comment #6, there must be a serious inefficiency if the system needs
> to check an entire document, potentially megabytes, for bidi trigger characters
> before it can even start to lay it out.

That doesn't follow. We start to lay out the document on the assumption that it
doesn't require bidi processing. If an RTL character appears megabytes later, it
would trigger a reflow. I don't think this would make a huge difference to the
overall layout time, but it's worth experimenting with, especially since it is
not so unlikely a scenario: many documents (e.g. Wikipedia pages) have a list at
the end of other languages in which the document is available, often under their
own native names, e.g. Français, Deutsch, עברית, 中文.

Comment 9

[Simon Montagu :smontagu]

Attachment: Updated patch

I've decided that it's best to just fix this. The patch is the same as before
against current trunk.

Comment 10

[Peter Kirk]

(In reply to comment #9)
> Created an attachment (id=179702) [edit]
> Updated patch
> 
> I've decided that it's best to just fix this. The patch is the same as before
> against current trunk.

Thank you, Simon. But while you are looking at this, is there a good reason for
apparently excluding U+07xx and U+08xx from the test? These are also RTL scripts
or reserved for them. Or is this somewhere else in the code, not in the patch?

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #10)
> But while you are looking at this, is there a good reason for
> apparently excluding U+07xx and U+08xx from the test?

I made that change on the last cycle, at your suggestion. See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsBidiUtils.h&rev=1.10&mark=225-242#225

Comment 12

[Peter Kirk]

...
> 
> I made that change on the last cycle, at your suggestion. See
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsBidiUtils.h&rev=1.10&mark=225-242#225
> 
Thanks! Your memory is better than mine.

Comment 13

[Peter Kirk]

*** Bug 288453 has been marked as a duplicate of this bug. ***

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 179702 [details] [diff] [review]
Updated patch

a=asa

Comment 15

[Simon Montagu :smontagu]

Checked in.