Closed Bug 177148 Opened 18 years ago Closed 16 years ago

Presence of hebrew character changes layout

Categories

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

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.3alpha

People

(Reporter: ian, Assigned: smontagu)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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.
reassigning per smontagu
Assignee: mkaply → smontagu
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
Attached patch Patch (obsolete) — Splinter Review
This patch would fix this bug, but perhaps it is WONTFIX per
http://www.unicode.org/unicode/reports/tr20/#Bidi ?
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?
direction: rtl certainly does, and so does any HTML element with dir="rtl". I'm
not sure about unicode-bidi: embed. Testcases coming.
Target Milestone: --- → mozilla1.3alpha
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.
Blocks: 288453
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.
(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, עברית, 中文.
Attached patch Updated patchSplinter Review
I've decided that it's best to just fix this. The patch is the same as before
against current trunk.
Attachment #104393 - Attachment is obsolete: true
Attachment #179702 - Flags: superreview?
Attachment #179702 - Flags: review?
Attachment #179702 - Flags: superreview?(roc)
Attachment #179702 - Flags: superreview?
Attachment #179702 - Flags: review?(roc)
Attachment #179702 - Flags: review?
(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?
(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

...
> 
> 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.

*** Bug 288453 has been marked as a duplicate of this bug. ***
Attachment #179702 - Flags: superreview?(roc)
Attachment #179702 - Flags: superreview+
Attachment #179702 - Flags: review?(roc)
Attachment #179702 - Flags: review+
Attachment #179702 - Flags: approval1.8b2?
Comment on attachment 179702 [details] [diff] [review]
Updated patch

a=asa
Attachment #179702 - Flags: approval1.8b2? → approval1.8b2+
Checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
Blocks: 244383
You need to log in before you can comment on or make changes to this bug.