Bug 51267 – Text emphasis marks should not appear over characters that have ruby annotations

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Text emphasis marks should not appear over characters that have ruby annotations.

<rdar://problem/8783318>

Created attachment 78078 [details]
Suppress emphasis marks over text that has ruby annotations

[Darin Adler]

Comment on attachment 78078 [details]
Suppress emphasis marks over text that has ruby annotations

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

> WebCore/rendering/InlineTextBox.cpp:405
> +bool InlineTextBox::getEmphasisMarkPosition(RenderStyle* style, TextEmphasisPosition& emphasisPosition) const

This function could use a tiny bit of “why” comments. Otherwise, you can see those return true and false and it’s hard to judge if they are right or wrong. Some brief statement of what significance the presence or absence of a line box has, and what significance the ruby has, and also why we needn’t even check for a ruby run if the container is not a ruby base.

> WebCore/rendering/InlineTextBox.cpp:409
> +    TextEmphasisMark emphasisMark = style->textEmphasisMark();
> +    if (emphasisMark == TextEmphasisMarkNone)
> +        return false;

I don’t think the local variable is needed here.

> WebCore/rendering/InlineTextBox.cpp:423
> +    RenderRubyRun* rubyRun = static_cast<RenderRubyRun*>(containingBlock->parent());
> +    RenderRubyText* rubyText = rubyRun->rubyText();

No need for the local variable rubyRun here. The expression containingBlock->parent() is used twice, but you chose to repeat it. If you did make a local variable, I suggest it be before the typecast, but none is OK with me.

Committed r75257.
http://trac.webkit.org/changeset/75257

[WebKit Review Bot]

http://trac.webkit.org/changeset/75257 might have broken GTK Linux 64-bit Debug and Qt Linux Release
The following tests are not passing:
fast/text/emphasis-avoid-ruby.html

[Ryosuke Niwa]

It seems like Qt and GTK have slightly different result:
http://build.webkit.org/results/Qt%20Linux%20Release/r75282%20(26174)/fast/text/emphasis-avoid-ruby-pretty-diff.html
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r75260%20(9393)/fast/text/emphasis-avoid-ruby-pretty-diff.html

FontQt doesn’t implement Font::emphasisMark{Ascent,Descent,Height} nor emphasis mark drawing.

[Csaba Osztrogonác]

(In reply to comment #7)
> FontQt doesn’t implement Font::emphasisMark{Ascent,Descent,Height} nor emphasis mark drawing.

new bug on Qt fail: https://bugs.webkit.org/show_bug.cgi?id=52155