Bug 4844 – Render "simple" Hebrew using the CG codepath

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

The CG codepath can handle Hebrew runs that don't contain niqqud or other "diacritics".

It seems that Hebrew was switched over to ATSUI over rdar://problem/3456887 , which was actually 
bug 3618 in the CG codepath, which is now fixed. While the CG codepath (still) can't handle combined 
characters and Hebrew presentation forms, it works fine for "simple" Hebrew. An overwhelming 
majority of Hebrew websites use nothing but "simple" Hebrew.

Advantages of using CG for "simple" Hebrew:
* improved performance
* support for layout features that are missing from the ATSUI codepath (bug 3922).

Disadvantage:
* if there's sporadic use of niqqud on a page, then adjacent text runs may end up using different 
codepaths. If, on top of that, the page uses text-align:justify (or word- or letter-spacing), the 
appearance will be weird. Considering that anything with text-align:justify already looks weird, I don't 
think this is a major issue.

The only code that needs to change is the range checking in shouldUseATSU().

Created attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

This patch causes 4 bidi layout tests to fail with benign horizontal shifting
of text.

[Darin Adler]

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Seems fine. r=me

Can we add some more Hebrew test cases to layout tests?

Created attachment 3772 [details]
patch expected layout test results

Created attachment 3773 [details]
updated expected pixel test results

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Patch causes a problem where shouldUseATSU returns YES for a run (when passed
from=0, to=length) but NO for some of its subruns (typically words), because CG
and ATSU don't give the same width even for CG-safe runs.

Of the Hebrew fonts I have, I'm seeing differences between CG and ATSU in width of Hebrew text only in 
Lucida Grande.

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Now that ATSU does the rounding hack it's safe to make this change. I'm not
sure the updated test results are still valid, though.

[Darin Adler]

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

r=me -- we're assuming this won't affect any layout tests now

(In reply to comment #9)

> r=me -- we're assuming this won't affect any layout tests now

Unfortunately, it will, for two reasons:
1. There can still be a <1px difference in the position of individual glyphs between CG and ATSU (since the 
CG code path applies the rounding in logical order and ATSU in left-to-right order), even though the total 
width remains the same.
2. The fix for bug 4940 didn't address fallback fonts that don't have the same rendering mode as the 
original font. This happens in tests that use Times (since the fallback is Lucida Grande).

[Darin Adler]

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

OK. Issue (1) is not a concern, but issue (2) is. I think we should put this on
hold again until that's dealt with.

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

> OK. Issue (1) is not a concern, but issue (2) is. I think we should put this on
> hold again until that's dealt with.

I think the exact opposite in this case. The first time I pulled the patch was
because it made things worse for lines mixing "simple" Hebrew with non-"simple"
Hebrew (say, one word with niqqud), since they went from all-ATSU lines to
mixed ATSU/CG, and there was a problem with mixed ATSU/CG.

Issue (2) currently affects (certain) mixed Latin/Hebrew lines, since they
currently mix ATSU with CG, and there is (still) a problem with mixed ATSU/CG,
but this time, changing Hebrew to CG will actually make things better for these
lines.

[Darin Adler]

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

OK.

Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

I changed my mind (again). The phenomenon described in comment #6 does remain a
problem in too many cases, due to issue (2). Need to figure out a solution for
that. Sorry about the mess.

Created attachment 5240 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

At last, making this change really affects only rendering. Pixel results are
expected to change for the following tests:
fast/text/international/bidi-AN-after-L
fast/text/international/bidi-L2-run-reordering
fast/text/international/bidi-LDB-2-CSS
fast/text/international/bidi-LDB-2-HTML 
fast/text/international/bidi-LDB-2-formatting-characters
fast/text/international/bidi-explicit-embedding
fast/text/international/bidi-fallback-font-weight
fast/text/international/bidi-ignored-for-first-child-inline
fast/text/international/bidi-innertext
fast/text/international/bidi-layout-across-linebreakfast/text/international/bidi-override

[Maciej Stachowiak]

Comment on attachment 5240 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Patch looks simple enough. I will take it on faith that it is conceptually
right as well.