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.
This issue is also happened in 537+ (r139280).
Created attachment 182514 [details] Test case result (above) and expected (below)
(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.
Could someone assign me on this issue?
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.
Created attachment 187570 [details] Patch
Could someone review this Patch? I will add layout test for this issue later.
Created attachment 188100 [details] Patch
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 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
Created attachment 188700 [details] Patch
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.
Created attachment 188702 [details] Patch
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 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.
> 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.
Yes it is BOM for UTF-8. I have removed it and been preparing for next patch with meta tag to UTF-8.
Created attachment 188757 [details] Patch
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
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.
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 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
Created attachment 189920 [details] Patch
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?
(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 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 on attachment 189920 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
I've filed a corresponding Chromium bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=1421450 . Firefox does not have this bug.