Bug 78957 – Underlines are split when ruby base is expanded because of long ruby text

• Status: ASSIGNED
• Component: Layout and Rendering

[Koji Ishii]

Created attachment 127701 [details]
Ruby with underline and base text is expanded because of long ruby text

When ruby text is long, its base text are expanded.

But even in that case, underlines should not be split.

STEPS:
1. Open attached html

RESULT:
The underline is split into 4 lines.

EXPECTED:
The underline is not split.

Note that in this sample, ruby text has its own underline, but this is a separate issue, filed in bug 71266.

[Zheng Xu]

This issue is also happened in 537+ (r139280).

[Koji Ishii]

Created attachment 182514 [details]
Test case result (above) and expected (below)

[Zheng Xu]

(In reply to comment #2)
> Created an attachment (id=182514) [details]
> Test case result (above) and expected (below)

Thank you for additional information.
I am trying this issue now.

[Zheng Xu]

Could someone assign me on this issue?

[Zheng Xu]

The text underline decoration is rendered at following progress of InlineTextBox::paintDecoration() @ InlineTextBox.cpp
  context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1), width, isPrinting); 

And the argument "width" define length of underline.

As I have not find any code considering Ruby in the source code segment, so it seems that the reason is above "width" does not include length of Ruby inline text.

[Zheng Xu]

Created attachment 187570 [details]
Patch

[Zheng Xu]

Could someone review this Patch?
I will add layout test for this issue later.

[Zheng Xu]

Created attachment 188100 [details]
Patch

[WebKit Review Bot]

Attachment 188100 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/text/decorations-with-ruby-unrderline-expand.html', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/mac/fast/text/decorations-with-ruby-unrderline-expand-expected.png', u'LayoutTests/platform/mac/fast/text/decorations-with-ruby-unrderline-expand-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h']" exit_code: 1
LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 188100 [details]
Patch

Attachment 188100 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16491981

New failing tests:
fast/text/decorations-with-ruby-unrderline-expand.html

[Zheng Xu]

Created attachment 188700 [details]
Patch

[WebKit Review Bot]

Attachment 188700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h']" exit_code: 1
LayoutTests/platform/mac/TestExpectations:1417:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zheng Xu]

Created attachment 188702 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188702 [details]
Patch

Attachment 188702 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16604318

New failing tests:
fast/text/decorations-with-ruby-underline-expand.html

[Benjamin Poulain]

Comment on attachment 188702 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:491
> -
> +    

Do not add spaces on blank lines.

> Source/WebCore/rendering/InlineTextBox.cpp:530
> -
> +    

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1012
> +    

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1013
> +    // Decoration should be expend to size of whole RubyRun if cb is RubyBase

Period at the end of the sentence.

> Source/WebCore/rendering/InlineTextBox.cpp:1014
> +    RenderBlock* cb = renderer()->containingBlock();

"cb" is not a good variable name. Something more descriptive would be better.

> LayoutTests/ChangeLog:22418
> -	Revert accidental global expectation override.
> +    Revert accidental global expectation override.

Don't modify other people changelogs as part of your patch.

> LayoutTests/ChangeLog:50114
> -	Expectations in chromium-mac were removed because they were added by mistake.
> -	[ Win Linux ] filter was removed because I need to add Mac but [ Mac Win Linux ] is equivalent to nothing.
> +    Expectations in chromium-mac were removed because they were added by mistake.
> +    [ Win Linux ] filter was removed because I need to add Mac but [ Mac Win Linux ] is equivalent to nothing.

Ditto.

> LayoutTests/fast/text/decorations-with-ruby-underline-expand.html:1
> +<!DOCTYPE html>

Are the caracters before the doctype intentional?

> LayoutTests/fast/text/decorations-with-ruby-underline-expand.html:4
> +<title>Untitled Page</title>

Better have a real title.

> LayoutTests/fast/text/decorations-with-ruby-underline-expand.html:8
> +<body>
> +<p><u>ããª<ruby>æ¼¢<rt>abcdefg</rt>å­<rt>abcdefg</rt></ruby>ã¾ãã</u></p>
> +</body>

Ideally, tests should have description of what is being tested. You should also explain what is the expected output when the test pass, so that someone can manually inspect the result when the test fails.

[Sangwhan Moon]

> Are the caracters before the doctype intentional?

BOM. Probably not intentional, but not harmful either. Unless there is a policy against it, it shouldn't affect the review. I'd personally strip it out just for the sake of saving a couple bytes.

[Zheng Xu]

Yes it is BOM for UTF-8.
I have removed it and been preparing for next patch with meta tag to UTF-8.

[Zheng Xu]

Created attachment 188757 [details]
Patch

[Build Bot]

Comment on attachment 188757 [details]
Patch

Attachment 188757 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16587827

New failing tests:
media/video-controls-captions-trackmenu.html

[Zheng Xu]

Thank you Benjamin and Moon for your kind advice and review.
I have add new Patch (https://bugs.webkit.org/attachment.cgi?id=188757)  regard comment(https://bugs.webkit.org/show_bug.cgi?id=78957#c15).
Appreciate for reviewing the new patch.

About new failure reported at comment 19(https://bugs.webkit.org/show_bug.cgi?id=78957#c19). I have checked the test html and can not find any relation with the newest patch.

[WebKit Review Bot]

Attachment 188757 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/text/decorations-with-ruby-underline-expand.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/fast/text/decorations-with-ruby-underline-expand-expected.png', u'LayoutTests/platform/mac/fast/text/decorations-with-ruby-underline-expand-expected.txt', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h']" exit_code: 1
LayoutTests/platform/mac/fast/text/decorations-with-ruby-underline-expand-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 188757 [details]
Patch

Attachment 188757 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16598955

New failing tests:
fast/text/decorations-with-ruby-underline-expand.html

[Zheng Xu]

Created attachment 189920 [details]
Patch

[Yuki Sekiguchi]

Created attachment 194419 [details]
The text under ruby base have margin.

This patch may break margin behavior.

In the attached content, 1st line have ruby and 2nd line don't have ruby.
These text should look the same.
However, this patch draw under line to margin portion.
I tested it on Safari for Mac.

Is this limitation or regression to be fixed?

[Zheng Xu]

(In reply to comment #24)
> Is this limitation or regression to be fixed?

Thank you Yuki. It seems as a regression and I am looking it.

[Koji Ishii]

(In reply to comment #25)
> (In reply to comment #24)
> > Is this limitation or regression to be fixed?
> 
> Thank you Yuki. It seems as a regression and I am looking it.

In my understanding, both lines of the test case should draw underlines including the margins. The current WebKit not drawing underlines to the margin region is a bug.

text-decoration-line is non-inherit, so child elements are not affected. It draws a single line from the beginning of the applied element to the end of it. From that definition, it should draw a line from "b" of "before" to "r" of "after".

If in doubt, it's probably best to ask www-style or [e-mail].

[Andreas Kling]

Comment on attachment 189920 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Domenic Denicola]

I've filed a corresponding Chromium bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=1421450 .

Firefox does not have this bug.