Bug 205990 – A partially selected RTL text is placed at a wrong vertical position if it has a vertical initial advance

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

[Fujii Hironori]

[HarfBuzz] A partially selected RTL text is placed at a wrong vertical position if it has a vertical initial advance

GTK port, trunk@254184

1. Go to https://ar.wikipedia.org/wiki/%D8%B3%D9%88%D8%B1%D8%A9_%D8%A7%D9%84%D8%A5%D8%AE%D9%84%D8%A7%D8%B5
2. Select a text which has Arabic diacritics by drugging mouse cursor slowly

You can observe glyphs are dancing vertically.

This issue still happens even after r254190 fixing other initial advance bug (Bug 118221).

[Fujii Hironori]

Created attachment 387194 [details]
test case

[Fujii Hironori]

Created attachment 387195 [details]
[screenshot] GTK port

[Fujii Hironori]

I put a break point at the end of FontCascade::getGlyphsAndAdvancesForComplexText,
and observed 'glyphBuffer' variable.

When drawing only latter 3 glyphs of "قُ ".

(gdb) p glyphBuffer.size()
$22 = 3
(gdb) p *glyphBuffer.glyphs(0)@3
$23 = {3, 1400, 1387}
(gdb) p *glyphBuffer.advances(0)@3
$24 = {{m_width = 21.53125, m_height = 3.90625}, {m_width = 0.46875, m_height = -3.90625}, {m_width = 62, m_height = -0}}
(gdb) p glyphBuffer.initialAdvance()
$25 = (const WebCore::GlyphBufferAdvance &) @0x7ffe17333190: {m_width = -0.46875, m_height = -3.90625}

When drawing all 5 glyphs,

(gdb) p glyphBuffer.size()
$26 = 5
(gdb) p *glyphBuffer.glyphs(0)@5
$28 = {1400, 1387, 3, 1400, 1387}
(gdb) p *glyphBuffer.advances(0)@5
$29 = {{m_width = 0.46875, m_height = -3.90625}, {m_width = 62, m_height = -0}, {m_width = 21.53125, m_height = 3.90625}, {m_width = 0.46875, m_height = -3.90625}, {m_width = 62, m_height = -0}}
(gdb) p glyphBuffer.initialAdvance()
$30 = (const WebCore::GlyphBufferAdvance &) @0x7ffe17332cd0: {m_width = -0.46875, m_height = -3.90625}

The glyph 1400 is painted on y=-3.90625 for the all glyphs case, but y=0 for latter 3 glyphs case.

[Fujii Hironori]

This issue will happen for WinCairo port if it'd use ComplexTextController by applying the patch of Bug 204884.

All 5 glyphs painting case:

glyphs: 757 981 3 757 981
initial advance: 9.0 4.0
advances: (-9,4) (47,0) (31,-4) (-9,4) (47,0)

Only latter 3 glyphs painting case:

glyphs: 3 757 981
initial advance: 9.0 4.0
advances: (31,-4) (-9,4) (47,0)

The glyph 757 is painted at y=4 for the first case,
and y=0 for the second case.

[Fujii Hironori]

Created attachment 387197 [details]
[Screenshot] WinCairo port

[Fujii Hironori]

FontCascade::getGlyphsAndAdvancesForComplexText is calculating
initialAdvance by subtracting RTL text's advances from totalWidth.

It should do same for y-axis.

[Fujii Hironori]

Created attachment 387203 [details]
WIP Patch

[Fujii Hironori]

Created attachment 387207 [details]
WIP Patch

[Fujii Hironori]

Created attachment 387209 [details]
WIP Patch

[Fujii Hironori]

Created attachment 387210 [details]
WIP Patch

[Fujii Hironori]

Created attachment 387211 [details]
WIP Patch

[Fujii Hironori]

Created attachment 387310 [details]
WIP patch

[Fujii Hironori]

Created attachment 387314 [details]
WIP patch

[Fujii Hironori]

Created attachment 388011 [details]
Patch to add a test

[Fujii Hironori]

Created attachment 388019 [details]
Patch

[Fujii Hironori]

Created attachment 388020 [details]
Patch

[Fujii Hironori]

Created attachment 388021 [details]
Patch

[Fujii Hironori]

Created attachment 388022 [details]
Patch

[Fujii Hironori]

Created attachment 388209 [details]
Patch

[Darin Adler]

Comment on attachment 388209 [details]
Patch

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

Looks like the Windows build failed — I couldn't find the error message in the EWS results, though. I retried so maybe it will succeed without any further changes.

> Source/WebCore/platform/graphics/ComplexTextController.h:68
> +    float totalWidth() const { return m_totalAdvance.width(); }

Are there any uses of this outside the class? Can this be a private member function instead? Or maybe we can just write m_totalAdvance.width() at each of the two call sites. I’d like to cut down on proliferation of functions that do nearly the same thing.

> Source/WebCore/platform/graphics/FontCascade.cpp:292
> +    FloatPoint oldStartPoint(startPoint);

I suggest that instead of this, we write:

    float oldStartX = startPoint.x();

> Source/WebCore/platform/graphics/FontCascade.cpp:1438
> +    FloatPoint startPoint(point);

Style wise, I think this is nicer with an "=", but not sure everyone else will agree.

> Source/WebCore/platform/graphics/GlyphBuffer.h:66
> +    operator FloatSize() const { return FloatSize(width(), height()); }

I think it’s a little risky to add an operator that does a lossy conversion from doubles to floats, implicitly and silently. Normally we would like conversions that narrow to be named function calls.

[Fujii Hironori]

Thank you very much for the review.

(In reply to Darin Adler from comment #20)
> Looks like the Windows build failed — I couldn't find the error message in
> the EWS results, though. I retried so maybe it will succeed without any
> further changes.

compile-webkit-tot actually reported the following error:

> Could NOT find interface definition for Node in ./dom/Node.idl at /home/buildbot/worker/Windows-EWS/build/Source/WebCore/bindings/scripts/CodeGenerator.pm line 502.

I think preprocessor.pm, run by Cygin perl, was failing to spawn gcc as a child process.
Bug 84274 is the past attempt to fix the issue.
AppleWin should use Win32 Perl as well as WinCairo does.

[Fujii Hironori]

Comment on attachment 388209 [details]
Patch

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

>> Source/WebCore/platform/graphics/ComplexTextController.h:68
>> +    float totalWidth() const { return m_totalAdvance.width(); }
> 
> Are there any uses of this outside the class? Can this be a private member function instead? Or maybe we can just write m_totalAdvance.width() at each of the two call sites. I’d like to cut down on proliferation of functions that do nearly the same thing.

totalAdvance() is used in FontCascade.
Will remove totalWidth().

>> Source/WebCore/platform/graphics/FontCascade.cpp:292
>> +    FloatPoint oldStartPoint(startPoint);
> 
> I suggest that instead of this, we write:
> 
>     float oldStartX = startPoint.x();

Will fix.

>> Source/WebCore/platform/graphics/FontCascade.cpp:1438
>> +    FloatPoint startPoint(point);
> 
> Style wise, I think this is nicer with an "=", but not sure everyone else will agree.

Will fix.

>> Source/WebCore/platform/graphics/GlyphBuffer.h:66
>> +    operator FloatSize() const { return FloatSize(width(), height()); }
> 
> I think it’s a little risky to add an operator that does a lossy conversion from doubles to floats, implicitly and silently. Normally we would like conversions that narrow to be named function calls.

I'm going to add
inline FloatSize toFloatSize(const GlyphBufferAdvance& a)

[Fujii Hironori]

Created attachment 388271 [details]
Patch

[Fujii Hironori]

Created attachment 388274 [details]
Patch

[Fujii Hironori]

I'm going to land this patch if nobody objects for a few day.

[Darin Adler]

Comment on attachment 388274 [details]
Patch

Looks good. I agree you should land it.

[Fujii Hironori]

Comment on attachment 388274 [details]
Patch

Clearing flags on attachment: 388274

Committed r254898: <https://trac.webkit.org/changeset/254898>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58782109>