Bug 130657 - Link underline thickness should be uniform
Summary: Link underline thickness should be uniform
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-23 21:06 PDT by Martin Schaus
Modified: 2016-09-18 18:03 PDT (History)
27 users (show)

See Also:


Attachments
Patch (7.98 KB, patch)
2014-04-07 07:51 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (536.67 KB, application/zip)
2014-04-07 09:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (584.42 KB, application/zip)
2014-04-07 09:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (532.97 KB, application/zip)
2014-04-07 10:28 PDT, Build Bot
no flags Details
Patch (4.75 KB, patch)
2014-04-12 07:48 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2014-04-12 23:19 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (665.83 KB, application/zip)
2014-04-13 00:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (685.40 KB, application/zip)
2014-04-13 00:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (685.31 KB, application/zip)
2014-04-13 01:48 PDT, Build Bot
no flags Details
Patch (16.43 KB, patch)
2014-04-13 08:20 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Patch (21.42 KB, patch)
2014-04-25 00:35 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Patch (21.29 KB, patch)
2014-04-25 05:03 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2014-05-26 14:13 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (496.99 KB, application/zip)
2014-05-26 19:24 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Schaus 2014-03-23 21:06:56 PDT
Following HTML results in a wrong underlining while hovering:

<style>
 a{text-decoration:none;font-size:40px} 
 a:hover{text-decoration:underline}
</style>
<a href="#">
    <span style="font-size:2em">Large Text</span>
    <span style="font-size;:1.1em">Small Text</span>
</a>

The expected result would be to use the font-size given with the A tag to be used for the underline of the whole content while hovering.

IE and Firefox behave correctly in standards mode.
Comment 1 Jeongeun Kim 2014-04-06 04:38:36 PDT
Hi, If there is nobody to look into this at this moment, Can I look into it?
Comment 2 Jeongeun Kim 2014-04-07 07:51:00 PDT
Created attachment 228734 [details]
Patch
Comment 3 Build Bot 2014-04-07 09:28:33 PDT
Comment on attachment 228734 [details]
Patch

Attachment 228734 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6646158927593472

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-svg.html
Comment 4 Build Bot 2014-04-07 09:28:38 PDT
Created attachment 228740 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-04-07 09:57:44 PDT
Comment on attachment 228734 [details]
Patch

Attachment 228734 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5771366833848320

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-svg.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
Comment 6 Build Bot 2014-04-07 09:57:48 PDT
Created attachment 228742 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-04-07 10:28:40 PDT
Comment on attachment 228734 [details]
Patch

Attachment 228734 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5489891857137664

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-svg.html
Comment 8 Build Bot 2014-04-07 10:28:47 PDT
Created attachment 228743 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Myles C. Maxfield 2014-04-07 12:37:31 PDT
We can't do this properly unless we have the concept of a decorating box in WebKit, which we don't right now.

Your current approach is not good because we don't want to pollute RenderStyle with a various attributes that should be used in different cases. The better solution to this is to implement decoration boxes.

r-
Comment 10 Myles C. Maxfield 2014-04-07 12:38:41 PDT
(In reply to comment #9)
> We can't do this properly unless we have the concept of a decorating box in WebKit, which we don't right now.
> 
> Your current approach is not good because we don't want to pollute RenderStyle with a various attributes that should be used in different cases. The better solution to this is to implement decoration boxes.
> 
> r-

See http://www.w3.org/TR/css-text-decor-3/ for a description of Decorating Boxes
Comment 11 Jeongeun Kim 2014-04-08 03:21:07 PDT
Hi Myles C. Maxfield, 
Thank you for review and feedback.
I thought it's just one of bugs but I realized that we need new design and implementation for decoration box after getting feedback from you.
It seems that we need new design for renderer, not CSS layer.
Am I right? I'll reconsider it.
Thanks,
Comment 12 Myles C. Maxfield 2014-04-08 13:24:40 PDT
(In reply to comment #11)
> It seems that we need new design for renderer, not CSS layer.

I am not sure what you mean. Could you elaborate?
Thanks,
Myles
Comment 13 Jeongeun Kim 2014-04-08 15:50:08 PDT
(In reply to comment #12)
> I am not sure what you mean. Could you elaborate?

I mean 
RenderStyle and other files for CSS is just for CSS properties and
A decorating box is handled in classes for rendering including RenderInline, Inlinebox.

"....When specified on or propagated to an inline box, that box becomes a decorating box for that decoration, applying the decoration to all its fragments." -http://www.w3.org/TR/css-text-decor-3/#decorating-box-

So, I'm looking into RenderInline and InlineBox, InlineTextBox, etc...
If you have some idea or suggestion, please let me know.

Thanks,
Jeongeun
Comment 14 Myles C. Maxfield 2014-04-08 17:17:24 PDT
You could start by giving InlineBoxes some knowledge of what their decorating box is.
Comment 15 Ryosuke Niwa 2014-04-08 17:44:50 PDT
(In reply to comment #9)
> We can't do this properly unless we have the concept of a decorating box in WebKit, which we don't right now.
> 
> Your current approach is not good because we don't want to pollute RenderStyle with a various attributes that should be used in different cases. The better solution to this is to implement decoration boxes.
> 
> r-

Please don't set r- flag on a patch based on an unofficial review.
Comment 16 Jeongeun Kim 2014-04-12 07:48:45 PDT
Created attachment 229205 [details]
Patch
Comment 17 Jeongeun Kim 2014-04-12 08:51:43 PDT
There is a similar working to search decoration box in RenderObject::getTextDecorationColors().
So, I think we don't need additional room to keep decoration box and we can use RenderObject::getTextDecorationColors() if we just change return type from it.
Please give me some feedback.
Comment 18 Darin Adler 2014-04-12 13:15:53 PDT
Comment on attachment 229205 [details]
Patch

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

A bug fix requires a regression test. We need a test that shows what was wrong before and the fact that it’s now right. review- because of the lack of test or explanation of why a test is lacking

> Source/WebCore/rendering/InlineTextBox.cpp:1011
> +    RenderObject* decorationBox = renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true);

It is wrong to name this “decorationBox” when the return value will not necessarily be a box.

> Source/WebCore/rendering/InlineTextBox.cpp:1013
> +        decorationBox = renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true, true);

I’m not sure it’s right to overwrite the value we got from the first call with the value from the second call. We need test cases to demonstrate this is good behavior.

> Source/WebCore/rendering/RenderObject.cpp:2146
> +RenderObject* RenderObject::getTextDecorationColors(int decorations, Color& underline, Color& overline,
>                                             Color& linethrough, bool quirksMode, bool firstlineStyle)

Should merge this into one line, rather than just messing up the indentation. (The person who original wrote this shouldn’t have lined it up in a way that would get messed up if we changed the return value.)

> Source/WebCore/rendering/RenderObject.h:719
> -    void getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, bool quirksMode = false, bool firstlineStyle = false);
> +    RenderObject* getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, bool quirksMode = false, bool firstlineStyle = false);

This function now needs a comment to explain the otherwise-mysterious return value.

I think it would be better to have the function return a RenderStyle& rather than a RenderObject* since all the caller does is get at the style, and the function inside is already coming up with a style. I’m also not sure if there’s ever a good reason to return null for the style. Worst case we return the style for the this object itself.
Comment 19 Darin Adler 2014-04-12 13:17:12 PDT
I think that a good type of test to use would be a reference test. The reference HTML could have more explicit specification of font size, where the test HTML would rely on the inheritance scaling rules. I think that could demonstrate what this is fixing, and that the fix works.
Comment 20 Jeongeun Kim 2014-04-12 19:07:49 PDT
(In reply to comment #18)
Drain, Thank you for your feedback and I'll create a new regression test case.

> > Source/WebCore/rendering/InlineTextBox.cpp:1013
> > +        decorationBox = renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true, true);
> 
> I’m not sure it’s right to overwrite the value we got from the first call with the value from the second call. We need test cases to demonstrate this is good behavior.
>

Actually I think we don't need to call additionally 'getTextDecorationColors' only for firstline.
It's duplicate.
The 6th param is for FirstLine and we can use it like
"renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true, isFirstLine());"
When text positions at the first line, it will take first line style.
Or not, it will take normal style.
So, I'll remove the second call. How do you think?
Comment 21 Jeongeun Kim 2014-04-12 23:19:08 PDT
Created attachment 229220 [details]
Patch
Comment 22 Build Bot 2014-04-13 00:27:41 PDT
Comment on attachment 229220 [details]
Patch

Attachment 229220 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5474551307698176

New failing tests:
fast/css/text-decorations-on-first-line-and-containing-block.html
Comment 23 Build Bot 2014-04-13 00:27:47 PDT
Created attachment 229221 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 24 Build Bot 2014-04-13 00:48:46 PDT
Comment on attachment 229220 [details]
Patch

Attachment 229220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5437792830095360

New failing tests:
fast/css/text-decorations-on-first-line-and-containing-block.html
Comment 25 Build Bot 2014-04-13 00:48:55 PDT
Created attachment 229223 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 26 Build Bot 2014-04-13 01:48:11 PDT
Comment on attachment 229220 [details]
Patch

Attachment 229220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4941284544348160

New failing tests:
fast/css/text-decorations-on-first-line-and-containing-block.html
Comment 27 Build Bot 2014-04-13 01:48:20 PDT
Created attachment 229225 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 28 Jeongeun Kim 2014-04-13 08:20:40 PDT
Created attachment 229233 [details]
Patch
Comment 29 Jeongeun Kim 2014-04-13 08:31:19 PDT
(In reply to comment #20)
> So, I'll remove the second call. How do you think?
Forget it. I was wrong and it's necessary. I update my patch and I'd like you to look into it.
Thanks,
Comment 30 Simon Fraser (smfr) 2014-04-14 17:28:44 PDT
Comment on attachment 229233 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:2145
> +void RenderObject::getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, HashMap<int, RenderStyle*>& styleMap, bool quirksMode, bool firstlineStyle)

This "int" should more strongly typed. Reading this signature, I have no idea what the int represents.

I'm also not sure that we want the cost of filling in a HashMap with every call to this function.
Comment 31 Jeongeun Kim 2014-04-15 05:03:11 PDT
Simon, Thank you for your feedback. 
(In reply to comment #30)
> > Source/WebCore/rendering/RenderObject.cpp:2145
> > +void RenderObject::getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, HashMap<int, RenderStyle*>& styleMap, bool quirksMode, bool firstlineStyle)
> 
> This "int" should more strongly typed. Reading this signature, I have no idea what the int represents.
"int" meant "enum TextDecoration". For make it clear, Is it OK if I changed it to <TextDecoration, RenderStyle*>?

> I'm also not sure that we want the cost of filling in a HashMap with every call to this function.
It is not different from getting color for decoration. In order to get colors for decorations, getTextDecorationColors() is used and outputs are filled with colors on current implementation. I just added additional inforamtion, RenderStyle*. 
One RenderObject could have underline and overline at the same time with different RenderStyles. If you look into test case created at the patch, there are several examples.
Please check,
Thanks,
Comment 32 Jeongeun Kim 2014-04-25 00:35:21 PDT
Created attachment 230147 [details]
Patch
Comment 33 Jeongeun Kim 2014-04-25 05:03:22 PDT
Created attachment 230169 [details]
Patch
Comment 34 Myles C. Maxfield 2014-05-20 17:36:25 PDT
Comment on attachment 230169 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:1038
> +    getThicknessForDecoration(isFirstLine(), firstLineStyle, style, thickness, textDecorationThickness);

This code, too, has been refactored. Make sure that visual overflow bounds are updated according to this (possibly thicker) underline.

> Source/WebCore/rendering/InlineTextBox.cpp:1039
> +    float wavyOffset = 2.f;

I have changed this. Please update your source tree.

> Source/WebCore/rendering/InlineTextBox.cpp:-1066
> -        float wavyOffset = 2.f;

You should update your source tree; I have changed this code recently.

> Source/WebCore/rendering/RenderObject.cpp:2127
> +    for (const auto& renderStyle : style) {

Shouldn't this be auto* instead?

In addition, the ":" syntax uses iterators under the hood. However, the style guide says not to use iterators on vectors. You might want to ask someone else on the team about whether or not this style is allowed on Vectors.

> Source/WebCore/rendering/RenderObject.cpp:2128
> +        index = nextIndex;

If you're going to calculate a nextIndex anyway, might as well use a regular for loop (without ":")

> Source/WebCore/rendering/RenderObject.cpp:2131
> +        RenderStyle* baseStyle = (isFirstLine && firstLineStyle->at(index)) ? firstLineStyle->at(index) : renderStyle;

Might want to put firstLineStyle->at(index) into a reference so you don't have to call it twice

> Source/WebCore/rendering/RenderObject.cpp:2152
> +    style.fill(nullptr, TextDecorationRenderStyleEnd);

Rather than do this on the heap, try to do this on the stack (avoiding a malloc()). This is constant size, after all.

It seems like relying on the TextDecorationRenderStyleXXXX values being contiguous and 0-indexed is brittle.

> Source/WebCore/rendering/RenderObject.cpp:2155
>          styleToUse = firstlineStyle ? &curr->firstLineStyle() : &curr->style();

I believe there is a lineStyle() function that does this for you, though I may be mistaken.
Comment 35 Myles C. Maxfield 2014-05-20 17:39:56 PDT
Overall, I think this approach is pretty good, provided:

1) it fits into ToT properly (I have refactored some of this code)
2) visual overflow bounds are properly updated
3) there are no performance problems (possibly because of moving these variables to the heap). Simon thinks that this may be a problem.
Comment 36 Tim Horton 2014-05-20 18:00:28 PDT
Comment on attachment 230169 [details]
Patch

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

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-unified-thickness.html:17
> +        <span style="font-size;:1.1em">&nbsp;</span>

is that semicolon supposed to be there?
Comment 37 Tim Horton 2014-05-20 18:10:36 PDT
Comment on attachment 230169 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:1043
> +    bool linesAreOpaque = !isPrinting && (!(decoration & TextDecorationUnderline) || decorationColors[RenderObject::TextDecorationRenderStyleUnderline].alpha() == 255)

Color has hasAlpha() which returns true if it is not opaque
Comment 38 Jeongeun Kim 2014-05-26 14:13:39 PDT
Created attachment 232097 [details]
Patch
Comment 39 Jeongeun Kim 2014-05-26 14:53:53 PDT
Myles, Simon and Tim,

I updated my patch.
1) Code was rebased.
2) Used pointer correctly.
3) Used a regular loop in order to avoid using iterators with vector.
4) Used a variable to avoid repeated call.
5) Used a vector with a static size to use it on the stack.
6) lineStyle() API is in InlineBox not in RenderObject.
I could add it but I thought that it’s better to add it when it is also used elsewhere in RenderObject. Currently, there is only the part for text decoration.
I’m also considering moving APIs related to text decoration to InlneTextBox after getting feedback from reviewers.
If possible, I’d like to work on that after file a new bug because it’s a kind of refactoring, not a issue.
7) Fixed typo of test case.
8) Used hasAlpha() for checking color.

Please take a look.
Thanks,
Jeongeun
Comment 40 Build Bot 2014-05-26 19:24:13 PDT
Comment on attachment 232097 [details]
Patch

Attachment 232097 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4921543750582272

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 41 Build Bot 2014-05-26 19:24:21 PDT
Created attachment 232102 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 42 Jeongeun Kim 2014-05-27 06:28:17 PDT
Comment on attachment 232097 [details]
Patch

It seems that nightly build had a problem and now it turned to green on all bots.
Comment 43 Michael Catanzaro 2016-09-17 07:15:26 PDT
Comment on attachment 232097 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
Comment 44 Jeongeun Kim 2016-09-18 17:51:55 PDT
(In reply to comment #43)
> Comment on attachment 232097 [details]
> Patch
> 
> Hi,
> 
> Apologies that your patch was not reviewed in a timely manner. Since it's
> now quite old, I am removing it from the review request queue. Please
> consider rebasing it on trunk and resubmitting.
> 
> To increase the chances of getting a review, consider using
> 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who
> might be interested in this bug.

Hi Martin, Thanks for your comment.
As code base has been updated continuously, simple re-base could not work. I'll look into it.
Comment 45 Jeongeun Kim 2016-09-18 18:03:23 PDT
Sorry. I replied to wrong person.
Michael, Thanks for updating and I'll look into it.

(In reply to comment #43)
> Comment on attachment 232097 [details]
> Patch
> 
> Hi,
> 
> Apologies that your patch was not reviewed in a timely manner. Since it's
> now quite old, I am removing it from the review request queue. Please
> consider rebasing it on trunk and resubmitting.
> 
> To increase the chances of getting a review, consider using
> 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who
> might be interested in this bug.

Hi Michael, Thanks for your comment.
As code base has been updated continuously, simple re-base could not work.
I'll look into it.