| Summary: | Link underline thickness should be uniform | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Martin Schaus <martin.schaus> |
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED DUPLICATE | ||
| Severity: | Normal | CC: | aboxhall, ahmad.saleem792, allan.jensen, apinheiro, ap, bfulgham, buildbot, cfleizach, commit-queue, darin, dino, dmazzoni, enrica, esprehn+autocc, glenn, gyuyoung.kim, jcraig, jdiggs, je00julie.kim, je_julie.kim, jonlee, kondapallykalyan, macpherson, mario, menard, mmaxfield, rniwa, samuel_white, simon.fraser, thorton, zalan |
| Priority: | P3 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Attachments: | |||
Hi, If there is nobody to look into this at this moment, Can I look into it? Created attachment 228734 [details]
Patch
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 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 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 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 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 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
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- (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 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, (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 (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 You could start by giving InlineBoxes some knowledge of what their decorating box is. (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. Created attachment 229205 [details]
Patch
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 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. 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. (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? Created attachment 229220 [details]
Patch
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 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 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 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 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 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
Created attachment 229233 [details]
Patch
(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 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. 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, Created attachment 230147 [details]
Patch
Created attachment 230169 [details]
Patch
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. 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 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"> </span> is that semicolon supposed to be there? 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 Created attachment 232097 [details]
Patch
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 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 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 on attachment 232097 [details]
Patch
It seems that nightly build had a problem and now it turned to green on all bots.
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.
(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. 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. I am able to reproduce this bug in Safari 15.5 on macOS 12.4 and I have converted the test case from Description into below JSFiddle: Link - https://jsfiddle.net/ah2xgnds/show All browsers behave as follow: *** Safari 15.5 **** Split underline (for Large and Small text) *** Chrome Canary 105 **** Single underline but follow thickness of small *** Firefox Nightly 104 **** Single underline but follow thickness of large *** This bug has been marked as a duplicate of bug 135353 *** |
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.