REGRESSION: missing underline under CJK text

• Component: Text
• Status: RESOLVED FIXED

[Philippe Wittenbergh]

Created attachment 223048 [details]
screenshot of Japanese text

Split from bug 128127

Underline under CJK text is sometimes missing, or only partly drawn

Screenshot from: http://www.jma.go.jp/jma/index.html
non-retina display

I can’t find an example right now of underline crossing characters in a worse way that Safari 7 / 10.9.1

I think it was on yahoo.co.jp

[Myles C. Maxfield]

This is caused by https://bugs.webkit.org/show_bug.cgi?id=127331

Setting this depending on https://bugs.webkit.org/show_bug.cgi?id=128127 because moving underlines farther from text might solve this problem.

[Myles C. Maxfield]

Thanks for filing this, Philippe.

[Jon Lee]

Consider this: https://bugzilla.mozilla.org/show_bug.cgi?id=770780#c29

[Myles C. Maxfield]

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

[Jon Lee]

<rdar://problem/16471190>

[Myles C. Maxfield]

Created attachment 231862 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 231862 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.cpp:1087
> +    UBlockCode blockCode = ublock_getCode(baseCharacter);

Add comment about why we're using ublock_getCode (perhaps as opposed to u_getIntPropertyValue and UCHAR_IDEOGRAPHIC)

In addition, this ICU call may be too slow to call for every glyph in every underlined text run. Perhaps we should cache the output of this function?

> Source/WebCore/platform/graphics/WidthIterator.cpp:276
> +        int currentCharacter = textIterator.currentCharacter();

Could perhaps move this up above the other calls to currentCharacter()

> Source/WebCore/platform/graphics/mac/FontMac.mm:549
> +        Path path;
> +        std::pair<float, float> extents;

These can be moved into the switch block, provided you add extra scoping {}s

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:104
>  class SVGGlyphToPathTranslator final : public GlyphToPathTranslator {

This code path seems untested.

[Myles C. Maxfield]

Comment on attachment 231862 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:1087
>> +    UBlockCode blockCode = ublock_getCode(baseCharacter);
> 
> Add comment about why we're using ublock_getCode (perhaps as opposed to u_getIntPropertyValue and UCHAR_IDEOGRAPHIC)
> 
> In addition, this ICU call may be too slow to call for every glyph in every underlined text run. Perhaps we should cache the output of this function?

Fixed.

I'm not sure, but it seems that implementing a cache is outside the scope of this patch. I've added a FIXME and filed https://bugs.webkit.org/show_bug.cgi?id=133529 regarding this.

>> Source/WebCore/platform/graphics/WidthIterator.cpp:276
>> +        int currentCharacter = textIterator.currentCharacter();
> 
> Could perhaps move this up above the other calls to currentCharacter()

Fixed.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:549
>> +        std::pair<float, float> extents;
> 
> These can be moved into the switch block, provided you add extra scoping {}s

Fixed.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:104
>>  class SVGGlyphToPathTranslator final : public GlyphToPathTranslator {
> 
> This code path seems untested.

Fixed.

[Myles C. Maxfield]

Created attachment 232515 [details]
Patch

[Darin Adler]

Comment on attachment 232515 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.cpp:1057
> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const UChar* end, UChar32& baseCharacter)

We already have U16_NEXT, so I don’t think we need this.

> Source/WebCore/platform/graphics/Font.cpp:1076
> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const TextRun& textRun, const GlyphBuffer& glyphBuffer, int index)

Not sure I understand the meaning of the word “shared” here.

> Source/WebCore/platform/graphics/Font.cpp:1079
> +    // We want to SkipDescenders in general. However, skipping descenders on CJK characters leads to undesirable renderings,
> +    // so we want to draw through CJK characters (on a character-by-character basis).

Please say “skip descenders” rather than SkipDescenders.

> Source/WebCore/platform/graphics/Font.cpp:1086
> +        if (!advanceByConsumingBaseCharacterInSequence(characterPointer, textRun.characters16() + textRun.length(), baseCharacter))
> +            return GlyphToPathTranslator::DrawOverGlyph;

Normally we just use U16_NEXT to implement something like this. We’d write:

    unsigned offset = glyphBuffer.offsetInString(index);
    U16_NEXT(textRun.characters16(), offset, textRun.length(), baseCharacter);

> Source/WebCore/platform/graphics/Font.cpp:1091
> +    // FIXME: we may need to cache the output of this ICU call. https://bugs.webkit.org/show_bug.cgi?id=133529

This seems like a strange comment; makes me feel fear but doesn’t give me information to know if I should be afraid or not. Did you do performance testing yet?

> Source/WebCore/platform/graphics/Font.h:103
> +    enum GlyphUnderlineType {
> +        SkipDescenders,
> +        SkipGlyph,
> +        DrawOverGlyph
> +    };

This would read nicely all on one line.

For most new enums we are using enum class.

> Source/WebCore/platform/graphics/Font.h:108
> +    virtual void increment() = 0;

I think advance would be a clearer name for this than increment.

> Source/WebCore/platform/graphics/Font.h:111
> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const TextRun&, const GlyphBuffer&, int index);

Is int the right type for the index? Why not unsigned?

As I asked above, what does “shared” mean in this function name?

> Source/WebCore/platform/graphics/Font.h:359
> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const UChar* end, UChar32& baseCharacter);

If you did want to put this in a header, it definitely wouldn’t be Font.h -- this is just a basic UTF-16 helper function. But luckily, we don’t need this because ICU already has a function for this.

> Source/WebCore/platform/graphics/GlyphBuffer.h:127
> +    void add(Glyph glyph, const SimpleFontData* font, float width, int offsetInString = -1, const FloatSize* offset = 0)

What does -1 mean here?

> Source/WebCore/platform/graphics/GlyphBuffer.h:198
> +        ASSERT(m_offsetsInString.get());

I’m surprised we need the get() here.

> Source/WebCore/platform/graphics/WidthIterator.cpp:175
> +        int currentCharacter = textIterator.currentCharacter();

Is int the right type for this?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:264
> -
> +    

You shouldn’t land this whitespace change.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:272
> +    if (!advanceByConsumingBaseCharacterInSequence(iterator, end, baseCharacter))
> +        return false;

Here’s how to write this using U16_NEXT:

    unsigned i = 0;
    U16_NEXT(iterator, i, end - iterator, baseCharacter);
    if (U_IS_SURROGATE(baseCharacter))
        return false;
    iterator += i;

We can possibly even leave out the U_IS_SURROGATE check.

> Source/WebCore/platform/graphics/mac/FontMac.mm:521
> +    glyphBuffer.saveOffsetsInString();

What guarantees that everyone adding to this glyph buffer will add an offset, rather than passing in -1?

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:362
> +            glyphBuffer->add(glyph, fontData, advance, -1, &size);

This magic -1 is unclear.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:118
> +    AffineTransform getCurrentTransform();

This should be named currentTransform() to follow WebKit naming conventions. Or just transform(). After all, we call it path(), not currentPath().

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:120
> +    const TextRun* m_textRun;

This should be const TextRun* const, I think; m_stoppingPoint, m_scale, and m_isVerticalText are all marked const, so why not this as well?

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:156
> +    

Should not add this whitespace here.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:278
> +    auto translator(createGlyphToPathTranslator(*fontData, nullptr, glyphBuffer, from, numGlyphs, point));

I think this would read better with = than with () syntax:

    auto translator = createGlyphToPathTranslator(*fontData, nullptr, glyphBuffer, from, numGlyphs, point);

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:279
>      while (translator->containsMorePaths()) {

This looks like it should be a for loop. Even if the initialization of the translator is too long to put inside, I think this is better than the while:

    for (; translator->containsMorePaths(); translator->increment()) {

[Myles C. Maxfield]

Comment on attachment 232515 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:1057
>> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const UChar* end, UChar32& baseCharacter)
> 
> We already have U16_NEXT, so I don’t think we need this.

Done.

>> Source/WebCore/platform/graphics/Font.cpp:1076
>> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const TextRun& textRun, const GlyphBuffer& glyphBuffer, int index)
> 
> Not sure I understand the meaning of the word “shared” here.

It was supposed to mean that both (substantial) implementations of GlyphToPathTranslator::underlineType() simply call this function. I had originally tried to move it into the base class, but it didn't work because of the DummyGlyphToPathTranslator. I've renamed this function to something more descriptive.

>> Source/WebCore/platform/graphics/Font.cpp:1079
>> +    // so we want to draw through CJK characters (on a character-by-character basis).
> 
> Please say “skip descenders” rather than SkipDescenders.

Done.

>> Source/WebCore/platform/graphics/Font.cpp:1086
>> +            return GlyphToPathTranslator::DrawOverGlyph;
> 
> Normally we just use U16_NEXT to implement something like this. We’d write:
> 
>     unsigned offset = glyphBuffer.offsetInString(index);
>     U16_NEXT(textRun.characters16(), offset, textRun.length(), baseCharacter);

Done.

>> Source/WebCore/platform/graphics/Font.cpp:1091
>> +    // FIXME: we may need to cache the output of this ICU call. https://bugs.webkit.org/show_bug.cgi?id=133529
> 
> This seems like a strange comment; makes me feel fear but doesn’t give me information to know if I should be afraid or not. Did you do performance testing yet?

I will do this before I commit, and adjust the comment accordingly.

>> Source/WebCore/platform/graphics/Font.h:103
>> +    };
> 
> This would read nicely all on one line.
> 
> For most new enums we are using enum class.

Done.

>> Source/WebCore/platform/graphics/Font.h:108
>> +    virtual void increment() = 0;
> 
> I think advance would be a clearer name for this than increment.

Done.

>> Source/WebCore/platform/graphics/Font.h:111
>> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const TextRun&, const GlyphBuffer&, int index);
> 
> Is int the right type for the index? Why not unsigned?
> 
> As I asked above, what does “shared” mean in this function name?

The GlyphToPathTranslators already use an int to determine where in the string they are translating (m_index in both implementations). The code was already using ints before I started refactoring it. https://bugs.webkit.org/show_bug.cgi?id=133594

>> Source/WebCore/platform/graphics/Font.h:359
>> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const UChar* end, UChar32& baseCharacter);
> 
> If you did want to put this in a header, it definitely wouldn’t be Font.h -- this is just a basic UTF-16 helper function. But luckily, we don’t need this because ICU already has a function for this.

Done.

>> Source/WebCore/platform/graphics/GlyphBuffer.h:127
>> +    void add(Glyph glyph, const SimpleFontData* font, float width, int offsetInString = -1, const FloatSize* offset = 0)
> 
> What does -1 mean here?

I've changed this to used a constant with a descriptive name.

>> Source/WebCore/platform/graphics/GlyphBuffer.h:198
>> +        ASSERT(m_offsetsInString.get());
> 
> I’m surprised we need the get() here.

We don't. Done.

>> Source/WebCore/platform/graphics/WidthIterator.cpp:175
>> +        int currentCharacter = textIterator.currentCharacter();
> 
> Is int the right type for this?

int is the return type of currentCharacter(). https://bugs.webkit.org/show_bug.cgi?id=133592

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:264
>> +    
> 
> You shouldn’t land this whitespace change.

I blame Xcode. Done.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:272
>> +        return false;
> 
> Here’s how to write this using U16_NEXT:
> 
>     unsigned i = 0;
>     U16_NEXT(iterator, i, end - iterator, baseCharacter);
>     if (U_IS_SURROGATE(baseCharacter))
>         return false;
>     iterator += i;
> 
> We can possibly even leave out the U_IS_SURROGATE check.

Now that I know about U16_NEXT, changing this function at all is unrelated to this patch. I'll save simplifying this function for another patch. https://bugs.webkit.org/show_bug.cgi?id=133591

>> Source/WebCore/platform/graphics/mac/FontMac.mm:521
>> +    glyphBuffer.saveOffsetsInString();
> 
> What guarantees that everyone adding to this glyph buffer will add an offset, rather than passing in -1?

I updated the platform-independent Font::getGlyphsAndAdvancesForSimpleText, and I updated the mac implementation of Font::getGlyphsAndAdvancesForComplexTest. However, you're right, I can't guarantee that every port will update these indices correctly. In order to combat that, I've added a guard to the code that uses these indices to take an early out if we don't get a valid offset.

>> Source/WebCore/platform/graphics/win/UniscribeController.cpp:362
>> +            glyphBuffer->add(glyph, fontData, advance, -1, &size);
> 
> This magic -1 is unclear.

Done.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:118
>> +    AffineTransform getCurrentTransform();
> 
> This should be named currentTransform() to follow WebKit naming conventions. Or just transform(). After all, we call it path(), not currentPath().

I've elected for transform(). Done.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:120
>> +    const TextRun* m_textRun;
> 
> This should be const TextRun* const, I think; m_stoppingPoint, m_scale, and m_isVerticalText are all marked const, so why not this as well?

Done.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:156
>> +    
> 
> Should not add this whitespace here.

Done.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:278
>> +    auto translator(createGlyphToPathTranslator(*fontData, nullptr, glyphBuffer, from, numGlyphs, point));
> 
> I think this would read better with = than with () syntax:
> 
>     auto translator = createGlyphToPathTranslator(*fontData, nullptr, glyphBuffer, from, numGlyphs, point);

Done.

Is there some guide about when to use which syntax?

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:279
>>      while (translator->containsMorePaths()) {
> 
> This looks like it should be a for loop. Even if the initialization of the translator is too long to put inside, I think this is better than the while:
> 
>     for (; translator->containsMorePaths(); translator->increment()) {

Done. Putting the initialization in the for() line doesn't make it that unreadable, and it limits the scope of the variable, so I've chosen to do that.

[Myles C. Maxfield]

Comment on attachment 232515 [details]
Patch

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

>>> Source/WebCore/platform/graphics/Font.cpp:1091
>>> +    // FIXME: we may need to cache the output of this ICU call. https://bugs.webkit.org/show_bug.cgi?id=133529
>> 
>> This seems like a strange comment; makes me feel fear but doesn’t give me information to know if I should be afraid or not. Did you do performance testing yet?
> 
> I will do this before I commit, and adjust the comment accordingly.

Looks like there isn't a performance hit with the current code. I'll remove the comment.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169715