Bug 78957 - Underlines are split when ruby base is expanded because of long ruby text
Summary: Underlines are split when ruby base is expanded because of long ruby text
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zheng Xu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-18 01:58 PST by Koji Ishii
Modified: 2014-02-05 11:13 PST (History)
16 users (show)

See Also:


Attachments
Ruby with underline and base text is expanded because of long ruby text (183 bytes, text/html)
2012-02-18 01:58 PST, Koji Ishii
no flags Details
Test case result (above) and expected (below) (1.46 KB, image/png)
2013-01-13 23:12 PST, Koji Ishii
no flags Details
Patch (4.87 KB, patch)
2013-02-11 07:33 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
Patch (15.90 KB, patch)
2013-02-13 09:39 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2013-02-16 02:06 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
Patch (22.46 KB, patch)
2013-02-16 02:20 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
Patch (38.65 KB, patch)
2013-02-17 01:38 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
Patch (38.56 KB, patch)
2013-02-23 03:35 PST, Zheng Xu
no flags Details | Formatted Diff | Diff
The text under ruby base have margin. (327 bytes, text/html)
2013-03-21 19:40 PDT, Yuki Sekiguchi
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Koji Ishii 2012-02-18 01:58:16 PST
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.
Comment 1 Zheng Xu 2013-01-11 18:16:38 PST
This issue is also happened in 537+ (r139280).
Comment 2 Koji Ishii 2013-01-13 23:12:23 PST
Created attachment 182514 [details]
Test case result (above) and expected (below)
Comment 3 Zheng Xu 2013-01-17 16:57:04 PST
(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.
Comment 4 Zheng Xu 2013-01-18 16:24:23 PST
Could someone assign me on this issue?
Comment 5 Zheng Xu 2013-01-29 08:28:19 PST
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.
Comment 6 Zheng Xu 2013-02-11 07:33:14 PST
Created attachment 187570 [details]
Patch
Comment 7 Zheng Xu 2013-02-11 07:34:53 PST
Could someone review this Patch?
I will add layout test for this issue later.
Comment 8 Zheng Xu 2013-02-13 09:39:57 PST
Created attachment 188100 [details]
Patch
Comment 9 WebKit Review Bot 2013-02-13 09:43:29 PST
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.
Comment 10 WebKit Review Bot 2013-02-13 13:37:42 PST
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
Comment 11 Zheng Xu 2013-02-16 02:06:36 PST
Created attachment 188700 [details]
Patch
Comment 12 WebKit Review Bot 2013-02-16 02:09:28 PST
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.
Comment 13 Zheng Xu 2013-02-16 02:20:07 PST
Created attachment 188702 [details]
Patch
Comment 14 WebKit Review Bot 2013-02-16 08:17:51 PST
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
Comment 15 Benjamin Poulain 2013-02-16 18:04:05 PST
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.
Comment 16 Sangwhan Moon 2013-02-16 23:36:32 PST
> 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.
Comment 17 Zheng Xu 2013-02-17 01:12:55 PST
Yes it is BOM for UTF-8.
I have removed it and been preparing for next patch with meta tag to UTF-8.
Comment 18 Zheng Xu 2013-02-17 01:38:39 PST
Created attachment 188757 [details]
Patch
Comment 19 Build Bot 2013-02-17 05:24:15 PST
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
Comment 20 Zheng Xu 2013-02-17 06:48:13 PST
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.
Comment 21 WebKit Review Bot 2013-02-17 21:27:50 PST
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.
Comment 22 WebKit Review Bot 2013-02-17 23:18:21 PST
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
Comment 23 Zheng Xu 2013-02-23 03:35:51 PST
Created attachment 189920 [details]
Patch
Comment 24 Yuki Sekiguchi 2013-03-21 19:40:44 PDT
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?
Comment 25 Zheng Xu 2013-03-23 07:19:27 PDT
(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.
Comment 26 Koji Ishii 2013-08-10 22:55:50 PDT
(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 public-css-testsuite@w3.org.
Comment 27 Andreas Kling 2014-02-05 11:13:13 PST
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.