[GPU Process] [iOS] Vertical text is incorrectly displaced

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Radar WebKit Bug Importer]

<rdar://problem/85483031>

[Said Abou-Hallawa]

Created attachment 456209 [details]
Patch

[Said Abou-Hallawa]

*** Bug 236907 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 236908 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 236911 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 236916 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 236921 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Comment on attachment 456209 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:-260
> -        // Keep this in sync as the inverse of `fillVectorWithVerticalGlyphPositions`.

I think I should keep this comment. The new code below does exactly what it says.

[Myles C. Maxfield]

Comment on attachment 456209 [details]
Patch

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

r=me because this is a step in the right direction, but I think there's more work to make this function exactly equal to the inverse of fillVectorWithVerticalGlyphPositions().

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:259
> +    auto position = textMatrix.mapPoint(positions[0]);

nit: a better name could be "initialPenPosition" or something

>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:-260
>> -        // Keep this in sync as the inverse of `fillVectorWithVerticalGlyphPositions`.
> 
> I think I should keep this comment. The new code below does exactly what it says.

Agreed. It's important to keep the reference to fillVectorWithVerticalGlyphPositions().

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:271
> +    m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, textMatrix).data(), count, position, m_smoothingMode);

This doesn't use rotateLeftTransform(). fillVectorWithVerticalGlyphPositions() does use rotateLeftTransform(), and this is supposed to be the inverse of that function. So I think there's more work that needs to be done here.

[Said Abou-Hallawa]

Comment on attachment 456209 [details]
Patch

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

>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:259
>> +    auto position = textMatrix.mapPoint(positions[0]);
> 
> nit: a better name could be "initialPenPosition" or something

The name was changed.

>>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:-260
>>> -        // Keep this in sync as the inverse of `fillVectorWithVerticalGlyphPositions`.
>> 
>> I think I should keep this comment. The new code below does exactly what it says.
> 
> Agreed. It's important to keep the reference to fillVectorWithVerticalGlyphPositions().

The comment was put back.

>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:271
>> +    m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, textMatrix).data(), count, position, m_smoothingMode);
> 
> This doesn't use rotateLeftTransform(). fillVectorWithVerticalGlyphPositions() does use rotateLeftTransform(), and this is supposed to be the inverse of that function. So I think there's more work that needs to be done here.

A FIXME comment was added.

[Said Abou-Hallawa]

Created attachment 456297 [details]
Patch

[Said Abou-Hallawa]

Created attachment 456301 [details]
Patch

[EWS]

Committed r292198 (249101@main): <https://commits.webkit.org/249101@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456301 [details].

[Myles C. Maxfield]

Continuing work on this in https://bugs.webkit.org/show_bug.cgi?id=239484