Skip to content

Commit

Permalink
[Fast text codepath] CharactersTreatedAsSpace is overly prescriptive
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=260843
rdar://114607926

Reviewed by Cameron McCormack.

When we lay out web content, we make multiple passes over the text,
chopping it up multiple different ways. Because of this, we have an
invariant that the width of a space can't change depending on the
bounds of how we're chopping up the text. Kerning can affect the
width of a space, so we enforce the invariant by having a postprocess
that we perform after shaping to retroactively go back and adjust
the width of any glyphs associated with space characters, to reset
their widths. This is called "CharactersTreatedAsSpace."

However, CharactersTreatedAsSpace actually does something else, too:
it also adjusts the width of the character directly before the space
to be whatever it was before shaping. However, this is kind of
bogus - it's totally valid for shaping to adjust the width of
whichever character happens to be before the space.

Now that we're trying to make the fast text codepath handle complex
character clusters, we're hitting the case more often where shaping
is adjusting the non-space glyphs to be correct, but then our
postprocess is coming along and messing them up. This patch removes
that logic, just for the glyphs before the space characters. The
logic about trying to reset the space characters' width is still
there.

* Source/WebCore/platform/graphics/WidthIterator.cpp:
(WebCore::OriginalAdvancesForCharacterTreatedAsSpace::OriginalAdvancesForCharacterTreatedAsSpace):
(WebCore::WidthIterator::applyFontTransforms):
(WebCore::WidthIterator::advanceInternal):

Canonical link: https://commits.webkit.org/267541@main
  • Loading branch information
litherum committed Sep 1, 2023
1 parent 19378f7 commit 6566fb8
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ layer at (0,0) size 800x600
text run at (2,2) width 16: " "
RenderTableCell {TD} at (24,54) size 74x24 [bgcolor=#0000FF] [border: (1px inset #808080)] [r=1 c=1 rs=2 cs=2]
RenderBlock {DIV} at (2,2) size 70x20 [color=#008000]
RenderText {#text} at (15,0) size 55x19
text run at (15,0) width 55: " FAIL "
RenderText {#text} at (16,0) size 54x19
text run at (16,0) width 54: " FAIL "
RenderTableRow {TR} at (0,54) size 100x24
RenderTableCell {TD} at (2,54) size 96x24 [bgcolor=#008000] [border: (1px inset #808080)] [r=2 c=0 rs=1 cs=3]
RenderBlock {DIV} at (2,2) size 92x20
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,5 @@ layer at (0,0) size 800x600
RenderTableRow {TR} at (0,80) size 186x24
RenderTableCell {TD} at (2,80) size 182x24 [bgcolor=#008000] [border: (1px inset #808080)] [r=3 c=0 rs=1 cs=4]
RenderBlock {DIV} at (2,2) size 178x20 [color=#008000]
RenderText {#text} at (139,0) size 39x19
text run at (139,0) width 39: "FAIL "
RenderText {#text} at (140,0) size 38x19
text run at (140,0) width 38: "FAIL "
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ layer at (0,0) size 800x600
RenderText {#text} at (2,42) size 130x19
text run at (2,42) width 130: "4633 Ocean Avenue"
RenderBR {BR} at (131,42) size 1x19
RenderText {#text} at (2,62) size 173x19
RenderText {#text} at (2,62) size 172x19
text run at (2,62) width 103: "San Francisco, "
text run at (104,62) width 31: "CA "
text run at (134,62) width 41: "94132"
RenderBR {BR} at (174,62) size 1x19
text run at (104,62) width 30: "CA "
text run at (133,62) width 41: "94132"
RenderBR {BR} at (173,62) size 1x19
RenderTableCell {TD} at (252,1) size 25x5 [border: (1px inset #808080)] [r=0 c=2 rs=1 cs=1]
RenderImage {IMG} at (2,2) size 20x1
RenderTableCell {TD} at (277,1) size 148x38 [border: (1px inset #808080)] [r=0 c=3 rs=1 cs=1]
Expand Down Expand Up @@ -122,10 +122,10 @@ layer at (0,0) size 800x600
RenderText {#text} at (2,42) size 131x19
text run at (2,42) width 131: "Real Estate Services"
RenderBR {BR} at (132,42) size 1x19
RenderText {#text} at (2,62) size 71x19
text run at (2,62) width 31: "CA "
text run at (32,62) width 41: "00000"
RenderBR {BR} at (72,62) size 1x19
RenderText {#text} at (2,62) size 70x19
text run at (2,62) width 30: "CA "
text run at (31,62) width 41: "00000"
RenderBR {BR} at (71,62) size 1x19
RenderText {#text} at (2,82) size 100x19
text run at (2,82) width 100: "(000) 000-0000"
RenderTableCell {TD} at (252,112) size 25x5 [border: (1px inset #808080)] [r=2 c=2 rs=1 cs=1]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ layer at (0,0) size 800x2685
RenderTableSection {TBODY} at (1,1) size 473x106
RenderTableRow {TR} at (0,2) size 473x24
RenderTableCell {TH} at (2,2) size 69x24 [border: (1px inset #808080)] [r=0 c=0 rs=1 cs=1]
RenderText {#text} at (12,2) size 44x19
text run at (12,2) width 44: "Color "
RenderText {#text} at (13,2) size 43x19
text run at (13,2) width 43: "Color "
RenderTableCell {TH} at (72,2) size 399x24 [border: (1px inset #808080)] [r=0 c=1 rs=1 cs=1]
RenderText {#text} at (166,2) size 66x19
text run at (166,2) width 66: "Meaning "
Expand Down
20 changes: 7 additions & 13 deletions Source/WebCore/platform/graphics/WidthIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,16 @@ WidthIterator::WidthIterator(const FontCascade& font, const TextRun& run, HashSe
}

struct OriginalAdvancesForCharacterTreatedAsSpace {
explicit OriginalAdvancesForCharacterTreatedAsSpace(GlyphBufferStringOffset stringOffset, bool isSpace, float advanceBefore, float advanceAt)
explicit OriginalAdvancesForCharacterTreatedAsSpace(GlyphBufferStringOffset stringOffset, bool isSpace, float advance)
: stringOffset(stringOffset)
, characterIsSpace(isSpace)
, advanceBeforeCharacter(advanceBefore)
, advanceAtCharacter(advanceAt)
, advance(advance)
{
}

GlyphBufferStringOffset stringOffset;
bool characterIsSpace;
float advanceBeforeCharacter;
float advanceAtCharacter;
GlyphBufferStringOffset stringOffset { 0 };
bool characterIsSpace { false };
float advance { 0 };
};

inline auto WidthIterator::applyFontTransforms(GlyphBuffer& glyphBuffer, unsigned lastGlyphCount, const Font& font, CharactersTreatedAsSpace& charactersTreatedAsSpace) -> ApplyFontTransformsResult
Expand Down Expand Up @@ -107,9 +105,7 @@ inline auto WidthIterator::applyFontTransforms(GlyphBuffer& glyphBuffer, unsigne
if (iterator == charactersTreatedAsSpace.end() || iterator->stringOffset != characterIndex)
continue;
const auto& originalAdvances = *iterator;
if (i && !originalAdvances.characterIsSpace)
setWidth(*glyphBuffer.advances(i - 1), originalAdvances.advanceBeforeCharacter);
setWidth(*glyphBuffer.advances(i), originalAdvances.advanceAtCharacter);
setWidth(*glyphBuffer.advances(i), originalAdvances.advance);
}
charactersTreatedAsSpace.clear();

Expand Down Expand Up @@ -381,7 +377,6 @@ inline void WidthIterator::advanceInternal(TextIterator& textIterator, GlyphBuff

UChar32 character = 0;
float width = 0;
float previousWidth = 0;
unsigned clusterLength = 0;
// We are iterating in string order, not glyph order. Compare this to ComplexTextController::adjustGlyphsAndAdvances()
if (!textIterator.consume(character, clusterLength))
Expand Down Expand Up @@ -442,12 +437,11 @@ inline void WidthIterator::advanceInternal(TextIterator& textIterator, GlyphBuff
continue;
}

previousWidth = width;
width = advanceInternalState.nextRangeFont->widthForGlyph(glyph, Font::SyntheticBoldInclusion::Exclude); // We apply synthetic bold after shaping, in applyCSSVisibilityRules().
advanceInternalState.widthOfCurrentFontRange += width;

if (FontCascade::treatAsSpace(characterToWrite))
advanceInternalState.charactersTreatedAsSpace.constructAndAppend(advanceInternalState.currentCharacterIndex, characterToWrite == space, previousWidth, characterToWrite == tabCharacter ? width : advanceInternalState.nextRangeFont->spaceWidth(Font::SyntheticBoldInclusion::Exclude));
advanceInternalState.charactersTreatedAsSpace.constructAndAppend(advanceInternalState.currentCharacterIndex, characterToWrite == space, characterToWrite == tabCharacter ? width : advanceInternalState.nextRangeFont->spaceWidth(Font::SyntheticBoldInclusion::Exclude));

if (m_accountForGlyphBounds) {
bounds = advanceInternalState.nextRangeFont->boundsForGlyph(glyph);
Expand Down

0 comments on commit 6566fb8

Please sign in to comment.