Bug 232917 - [GPU Process] [iOS] Vertical text is incorrectly displaced
Summary: [GPU Process] [iOS] Vertical text is incorrectly displaced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 236907 236908 236911 236916 236921 (view as bug list)
Depends on: 232645
Blocks: 239484
  Show dependency tree / graph
 
Reported: 2021-11-09 17:21 PST by Devin Rousso
Modified: 2022-04-20 15:12 PDT (History)
8 users (show)

See Also:

Attachments
Patch (9.00 KB, patch)
2022-03-31 01:00 PDT, Said Abou-Hallawa 		mmaxfield: review+
Details | Formatted Diff | Diff
Patch (9.04 KB, patch)
2022-03-31 17:02 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (8.66 KB, patch)
2022-03-31 17:57 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-11-09 17:21:43 PST 
.
Comment 1 Radar WebKit Bug Importer 2021-11-16 17:22:21 PST 
<rdar://problem/85483031>
Comment 2 Said Abou-Hallawa 2022-03-31 01:00:50 PDT 
Created attachment 456209 [details]
Patch
Comment 3 Said Abou-Hallawa 2022-03-31 01:02:11 PDT 
*** Bug 236907 has been marked as a duplicate of this bug. ***
Comment 4 Said Abou-Hallawa 2022-03-31 01:03:36 PDT 
*** Bug 236908 has been marked as a duplicate of this bug. ***
Comment 5 Said Abou-Hallawa 2022-03-31 01:06:45 PDT 
*** Bug 236911 has been marked as a duplicate of this bug. ***
Comment 6 Said Abou-Hallawa 2022-03-31 01:08:10 PDT 
*** Bug 236916 has been marked as a duplicate of this bug. ***
Comment 7 Said Abou-Hallawa 2022-03-31 01:30:02 PDT 
*** Bug 236921 has been marked as a duplicate of this bug. ***
Comment 8 Said Abou-Hallawa 2022-03-31 13:02:01 PDT 
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.
Comment 9 Myles C. Maxfield 2022-03-31 16:12:50 PDT 
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.
Comment 10 Said Abou-Hallawa 2022-03-31 17:01:01 PDT 
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.
Comment 11 Said Abou-Hallawa 2022-03-31 17:02:41 PDT 
Created attachment 456297 [details]
Patch
Comment 12 Said Abou-Hallawa 2022-03-31 17:57:50 PDT 
Created attachment 456301 [details]
Patch
Comment 13 EWS 2022-03-31 20:41:05 PDT 
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].
Comment 14 Myles C. Maxfield 2022-04-20 15:12:44 PDT 
Continuing work on this in https://bugs.webkit.org/show_bug.cgi?id=239484