[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).
Created attachment 387194 [details] test case
Created attachment 387195 [details] [screenshot] GTK port
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.
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.
Created attachment 387197 [details] [Screenshot] WinCairo port
FontCascade::getGlyphsAndAdvancesForComplexText is calculating initialAdvance by subtracting RTL text's advances from totalWidth. It should do same for y-axis.
Created attachment 387203 [details] WIP Patch
Created attachment 387207 [details] WIP Patch
Created attachment 387209 [details] WIP Patch
Created attachment 387210 [details] WIP Patch
Created attachment 387211 [details] WIP Patch
Created attachment 387310 [details] WIP patch
Created attachment 387314 [details] WIP patch
Created attachment 388011 [details] Patch to add a test
Created attachment 388019 [details] Patch
Created attachment 388020 [details] Patch
Created attachment 388021 [details] Patch
Created attachment 388022 [details] Patch
Created attachment 388209 [details] Patch
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.
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.
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)
Created attachment 388271 [details] Patch
Created attachment 388274 [details] Patch
I'm going to land this patch if nobody objects for a few day.
Comment on attachment 388274 [details] Patch Looks good. I agree you should land it.
Comment on attachment 388274 [details] Patch Clearing flags on attachment: 388274 Committed r254898: <https://trac.webkit.org/changeset/254898>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58782109>