Bug 232645 – REGRESSION(r283863): `<attachment>` with a long `action` do not render correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/84558377>

[Devin Rousso]

Created attachment 443141 [details]
Patch

[Myles C. Maxfield]

I'll review this today.

[Myles C. Maxfield]

I didn't get to it today; I'll review it tomorrow.

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:321
> +        if (m_deriveFontFromContext == DeriveFontFromContext::No)
> +            ctmFixup *= currentTextMatrix;

I'm pretty sure this is wrong, but I'm still investigating to figure out what's right. I'm discussing with Devin offline.

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:321
>> +            ctmFixup *= currentTextMatrix;
> 
> I'm pretty sure this is wrong, but I'm still investigating to figure out what's right. I'm discussing with Devin offline.

I think I know what's wrong. It's the line below "m_owner.drawGlyphsAndCacheFont(...)". It's passing the wrong matrix into computeAdvancesFromPositions() and mapPoint().

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

> LayoutTests/fast/attachment/attachment-truncated-action.html:4
> +<attachment action="this is very very very very very very very very very very long action" title="title"></attachment>

I ran this test and it passed with no code change. Am I doing something wrong?

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:326
>          inverseCTMFixup = inverse.value();

I believe the fix is for line 336 to change from

m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, currentTextMatrix).data(), count, currentTextMatrix.mapPoint(positions[0]), m_smoothingMode);

to

m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, m_originalTextMatrix).data(), count, m_originalTextMatrix.mapPoint(positions[0]), m_smoothingMode);

but I haven't tested this.

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:326
>>          inverseCTMFixup = inverse.value();
> 
> I believe the fix is for line 336 to change from
> 
> m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, currentTextMatrix).data(), count, currentTextMatrix.mapPoint(positions[0]), m_smoothingMode);
> 
> to
> 
> m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, m_originalTextMatrix).data(), count, m_originalTextMatrix.mapPoint(positions[0]), m_smoothingMode);
> 
> but I haven't tested this.

I just verified that this fix solves the problem. Please update the test to make sure it fails with no code change before uploading the next patch.

[Myles C. Maxfield]

Comment on attachment 443141 [details]
Patch

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

>>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:326
>>>          inverseCTMFixup = inverse.value();
>> 
>> I believe the fix is for line 336 to change from
>> 
>> m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, currentTextMatrix).data(), count, currentTextMatrix.mapPoint(positions[0]), m_smoothingMode);
>> 
>> to
>> 
>> m_owner.drawGlyphsAndCacheFont(font, glyphs, computeAdvancesFromPositions(positions, count, m_originalTextMatrix).data(), count, m_originalTextMatrix.mapPoint(positions[0]), m_smoothingMode);
>> 
>> but I haven't tested this.
> 
> I just verified that this fix solves the problem. Please update the test to make sure it fails with no code change before uploading the next patch.

I'd also suggest adding a comment above this line:

// When replaying, drawGlyphs() transforms the glyph positions according to fillVectorWithHorizontalGlyphPositions(). Here, we apply the inverse transformation to make the positions round-trip correctly.

I also don't think this specific one-line change is sufficient, because drawGlyphs() can call fillVectorWithVerticalGlyphPositions() instead of fillVectorWithHorizontalGlyphPositions(). We would likely need to determine here which of those two would be called, and apply the inverse transformation of whichever the right one is.

[Devin Rousso]

Created attachment 443860 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 443860 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:340
> +    // positions by reversing the operations applied to the original advances.

Nit: I'd somehow work in the word "inverse" rather than "reverse".

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:344
> +        // FIXME: <https://webkit.org/b/232917> (`DrawGlyphsRecorder` should be able to record+replay vertical text)

🤔

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:121
> +static void fillVectorWithHorizontalGlyphPositions(Vector<CGPoint, 256>& positions, CGContextRef context, const CGSize* advances, unsigned count, const FloatPoint& point)

heh cool

[Devin Rousso]

Comment on attachment 443860 [details]
Patch

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

> LayoutTests/fast/attachment/attachment-truncated-action.html:4
> +<attachment action="this is very very very very very very very very very very long action" title="title" style="clip-path: inset(45% 25% 40% 50%)"></attachment>

rather than try to emulate the structure/style of `<attachment>` using CSS, this just clips out everything before the "...", meaning that without this patch it would appear as empty space and with this patch it would have the text "...ng action"

[Devin Rousso]

Created attachment 443861 [details]
Patch

[EWS]

Committed r285620 (244121@main): <https://commits.webkit.org/244121@main>

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