Bug 124057 – Characters too close together in complex Arabic text

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

Characters too close together in complex Arabic text

[Myles C. Maxfield]

Created attachment 216415 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/14944133>

[Tim Horton]

Comment on attachment 216415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216415&action=review

> LayoutTests/fast/text/complex-grapheme-cluster-with-initial-advance-expected.html:7
> +<span style="font-size: 100pt; background: red;">&#1604;&#1575;&#1611; <span>&#1609;</span> z</span></body>

please don't use red in the pass case of a layout test

[Myles C. Maxfield]

Created attachment 216419 [details]
Patch

[Dean Jackson]

Comment on attachment 216419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216419&action=review

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:586
> +        } else {
> +            firstInitialAdvanceDelta = complexTextRun.initialAdvance().width;
>          }

No braces here.

Comment on attachment 216419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216419&action=review

> Source/WebCore/ChangeLog:15
> +        which we shouldn't do. Instead, we can bake the initial advance
> +        into the current character's advance.

I don’t understand why this is correct. The initial advance comes before the first glyph. Adding it to the first glyph’s advance means that it will occur after the first glyph.

[Myles C. Maxfield]

Created attachment 216427 [details]
Patch

Comment on attachment 216427 [details]
Patch

I don’t quite follow this patch. When we talked about this, you said that there was a bug in that Font::drawGlyphBuffer() was ignoring the x component of the initial advance, but I don’t see that being addressed here. It also doesn’t look like your test covers the offsetOfPosition() part of the patch.

[Myles C. Maxfield]

mitz:
It is true that Font::drawGlyphBuffer() doesn't use glyphBuffer.initialAdvance().width() directly, however, upon more inspection I realized that the point that is passed into Font::drawGlyphBuffer() has already been adjusted by Font::drawComplexText(). getGlyphsAndAdvancesForComplexText() calculates the initial advance by subtracting the runWidthSoFar from the total width of the run (which this patch modifies to include the initial offset), and then returns it to Font::drawComplexText().

You're right that the offsetOfPosition() part isn't tested. I'll add another test for that.

[Myles C. Maxfield]

Created attachment 216515 [details]
Patch

[Darin Adler]

Comment on attachment 216515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216515&action=review

review- because this puts a Mac-specific result into a platform-independent test result folder.

> Source/WebCore/ChangeLog:3
> +        Characters too close together in complex Arabic text

Bug title should say [Mac] since this is a bug and fix in Mac-specific code.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:197
> +            CGFloat adjustedAdvance = 0;
> +            if (!index)
> +                adjustedAdvance += complexTextRun.initialAdvance().width;
> +            adjustedAdvance += m_adjustedAdvances[index].width;

Since addition is commutative, even for floating point, I’d write it like this instead:

    CGFloat adjustedAdvance = m_adjustedAdvances[index].width;
    if (!index)
        adjustedAdvance += complexTextRun.initialAdvance().width;

> LayoutTests/ChangeLog:21
> +        doesn't crash when there is an initial advance. HOWEVER: Note that with kerning and
> +        ligatures turned off, CoreText doesn't appear to ever create text runs with nonzero
> +        initial advances. This actually causes glyphs to be drawn differently (Without kerning
> +        and ligatures, the accent in this test is drawn on the left side of the character).
> +        Therefore, this code change can't actually be tested without kerning and ligatures.
> +        It's possible to to turn on kerning and ligatures by writing defaults such that
> +        [[NSUserDefaults standardUserDefaults] boolForKey:@"WebKitKerningAndLigaturesEnabledByDefault"]
> +        returns true in DumpRenderTree.

We need a test that actually runs, not one that an expert could run in some special way. If necessary, we will need to add a way to turn on kerning and ligatures from inside a test. Or figure out some good strategy to make sure that we test both code paths.

> LayoutTests/ChangeLog:27
> +        * fast/text/complex-grapheme-cluster-with-initial-advance-expected.html: Added.
> +        * fast/text/complex-grapheme-cluster-with-initial-advance.html: Added.
> +        * fast/text/selection-in-initial-advance-region-expected.txt: added
> +        * fast/text/selection-in-initial-advance-region.html: added

Is it really OK to put these tests in across platforms if the fix is only for the Mac? Do these tests pass on the other platforms? We certainly can’t expect the render tree result to work on other platforms, so it should not be checked into the fast/text directory; instead it should be in platform/mac/fast/text, but there may be a better approach.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:2
> +<div id="div" dir="RTL" style="font-size: 100pt;"><span id="c" style="background: yellow;">&#1604;&#1575;&#1611;</span></div>

Seems strange to use "pt" here instead of "px".

> LayoutTests/fast/text/selection-in-initial-advance-region.html:3
> +<ul id="console"></ul>

A little strange to use a <ul> where we will have a bullet in front of each line in the console.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:12
> +if (window.testRunner) {

This test doesn’t actually use testRunner, so this if statement should say:

if (window.eventSender)

Also, I prefer to put the test inside a function rather than just having it all be top level code.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:28
> +    var steps = 20;

Where does the magic number 20 come from?

> LayoutTests/fast/text/selection-in-initial-advance-region.html:34
> +    eventSender.mouseMoveTo(endx, endy);
> +    eventSender.mouseUp();

This test should do something to verify that appropriate text got selected, to make it harder to accidentally succeed just by not successfully creating a selection at all.

Then you could use dumpAsText and not require a pixel dump for the result. That would make the text runnable on non-Mac platforms without requiring unique results on each.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:38
> +    log("This uses the eventSender to perform a drag select.  To run it manually, click on the left half of the character and drag to the right half of the character.");

Should probably say “slowly drag” since you could miss the crash if you drag to fast. Is that right?

[Darin Adler]

Looks like a great fix!

Comment on attachment 216515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216515&action=review

>> LayoutTests/ChangeLog:21
>> +        returns true in DumpRenderTree.
> 
> We need a test that actually runs, not one that an expert could run in some special way. If necessary, we will need to add a way to turn on kerning and ligatures from inside a test. Or figure out some good strategy to make sure that we test both code paths.

The user default only controls whether kerning and ligatures are enabled by default. The CSS properties -webkit-font-kerning and -webkit-font-variant-ligatures can be used to control these features regardless of their default state.

[Myles C. Maxfield]

Comment on attachment 216515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216515&action=review

>> Source/WebCore/ChangeLog:3
>> +        Characters too close together in complex Arabic text
> 
> Bug title should say [Mac] since this is a bug and fix in Mac-specific code.

Done.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:197
>> +            adjustedAdvance += m_adjustedAdvances[index].width;
> 
> Since addition is commutative, even for floating point, I’d write it like this instead:
> 
>     CGFloat adjustedAdvance = m_adjustedAdvances[index].width;
>     if (!index)
>         adjustedAdvance += complexTextRun.initialAdvance().width;

Done.

>>> LayoutTests/ChangeLog:21
>>> +        returns true in DumpRenderTree.
>> 
>> We need a test that actually runs, not one that an expert could run in some special way. If necessary, we will need to add a way to turn on kerning and ligatures from inside a test. Or figure out some good strategy to make sure that we test both code paths.
> 
> The user default only controls whether kerning and ligatures are enabled by default. The CSS properties -webkit-font-kerning and -webkit-font-variant-ligatures can be used to control these features regardless of their default state.

Done! Thanks so much for that, Mitz!

>> LayoutTests/ChangeLog:27
>> +        * fast/text/selection-in-initial-advance-region.html: added
> 
> Is it really OK to put these tests in across platforms if the fix is only for the Mac? Do these tests pass on the other platforms? We certainly can’t expect the render tree result to work on other platforms, so it should not be checked into the fast/text directory; instead it should be in platform/mac/fast/text, but there may be a better approach.

Used dumpAsText() to circumvent this problem. Done.

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:2
>> +<div id="div" dir="RTL" style="font-size: 100pt;"><span id="c" style="background: yellow;">&#1604;&#1575;&#1611;</span></div>
> 
> Seems strange to use "pt" here instead of "px".

Done.

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:3
>> +<ul id="console"></ul>
> 
> A little strange to use a <ul> where we will have a bullet in front of each line in the console.

Done. I was using an existing test as a model.

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:12
>> +if (window.testRunner) {
> 
> This test doesn’t actually use testRunner, so this if statement should say:
> 
> if (window.eventSender)
> 
> Also, I prefer to put the test inside a function rather than just having it all be top level code.

Done. Just curious: why do you prefer a function?

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:28
>> +    var steps = 20;
> 
> Where does the magic number 20 come from?

Nowhere, I just need some number of steps that the mouse should drag through. Is there a better way of representing this?

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:34
>> +    eventSender.mouseUp();
> 
> This test should do something to verify that appropriate text got selected, to make it harder to accidentally succeed just by not successfully creating a selection at all.
> 
> Then you could use dumpAsText and not require a pixel dump for the result. That would make the text runnable on non-Mac platforms without requiring unique results on each.

Done.

>> LayoutTests/fast/text/selection-in-initial-advance-region.html:38
>> +    log("This uses the eventSender to perform a drag select.  To run it manually, click on the left half of the character and drag to the right half of the character.");
> 
> Should probably say “slowly drag” since you could miss the crash if you drag to fast. Is that right?

Yeah, one of the ticks has to be inside the initial offset region.

Done.

[Myles C. Maxfield]

Created attachment 216590 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 216590 [details]
Patch

Clearing flags on attachment: 216590

Committed r159076: <http://trac.webkit.org/changeset/159076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.