Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 9 months ago by h.joshi Modified:
6 years, 2 months ago Reviewers:
leviw_travelin_and_unemployed, Stephen Chennney, eae CC:
blink-reviews, zoltan1, dsinclair, bemjb+rendering_chromium.org, eae+blinkwatch, ed+blinkwatch_opera.com, leviw+renderwatch, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, pdr., rune+blink, f(malita), jbroman Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUnderline thickness is not uniform, due to recent changes to support Underline thickness as per Font Size, thickness is not uniform when we have different Underline Text of different size.
BUG=353907, 356092
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fixing comments and adding new test case #
Total comments: 9
Patch Set 3 : Comment fix #
Created: 6 years, 9 months ago
Messages
Total messages: 23 (0 generated)
Changes to have uniform Underline thickness when we have underline text with different Font size. In current patch, max underline Thickness value is taken to draw underline in current line.
So, if I read the code correctly, you find the biggest thickness among the children during painting, and use that thickness for all decorations in a second paint pass. (IE11 appears to do something similar). Did you consider the approach FF28 Beta [*] appears to take? It takes the thickness of the element which generated the text decoration, and uses that thickness for all children. This approach should yield cleaner code, and has the benefit of not requiring an additional paint: you could just acquire the thickness in RenderObject::getTextDecorations like we currently do with color and style. The drawback is that the decoration thickness might not be very related to the font-weights actually used by (one of) the children (if the author fails at CSS, arguably). Also, this patch should definitely have a reftest! [*] I gather that FF has a bug similar to this one. This is apparently fixed in FF28. https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineBox.cpp (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineBox.cpp:213: These could just appear in the header as: void [getPaint,paint]DecorationStyle( ... ) { } https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:1131: float underlineThickness = 0; It should probably be called decorationThickness, as it does not just apply to underline decorations. https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:1133: TextDecorationStyle style = TextDecorationStyleNone; Is TextDecorationStyle used for anything? https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineTextBox.h (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.h:203: void setUnderlineThickness(float textDecorationThickness) { m_textDecorationThickness = textDecorationThickness; } Again, it should be [get,set]DecorationThickness in my opinion. https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.h:206: void setTextDecoration(TextDecorationStyle textDecorations) { m_textDecorations = textDecorations; } Looks like you ended up not really using these for anything? Also, the plurality of the name "getTextDecorations" gives the wrong impression here ... https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... File Source/core/rendering/style/RenderStyleConstants.h (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... Source/core/rendering/style/RenderStyleConstants.h:366: TextDecorationStyleNone, Are you sure you need this? The decoration style can't be "none" per spec, and I don't think the value is needed internally either. Existence of decorations is controlled by text-decoration-line (enum TextDecoration), not by text-decoration-style. See http://www.w3.org/TR/css-text-decor-3/#text-decoration-style-property
Yes, in code I am finding biggest thickness during painting, and use that thickness for all decorations in a second paint pass. This is done as during painting only we know the decoration thickness (with recent patches now underline thickness changes as per font size). https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineBox.cpp (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineBox.cpp:213: Yes, will make required changes. On 2014/03/19 13:18:12, rude wrote: > These could just appear in the header as: > > void [getPaint,paint]DecorationStyle( ... ) { } https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:1131: float underlineThickness = 0; Will make required changes. On 2014/03/19 13:18:12, rude wrote: > It should probably be called decorationThickness, as it does not just apply to > underline decorations. https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:1133: TextDecorationStyle style = TextDecorationStyleNone; It is not used for now, but added for future reference. On 2014/03/19 13:18:12, rude wrote: > Is TextDecorationStyle used for anything? https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineTextBox.h (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.h:203: void setUnderlineThickness(float textDecorationThickness) { m_textDecorationThickness = textDecorationThickness; } Will make required changes. On 2014/03/19 13:18:12, rude wrote: > Again, it should be [get,set]DecorationThickness in my opinion. https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.h:206: void setTextDecoration(TextDecorationStyle textDecorations) { m_textDecorations = textDecorations; } Will remove "s" from method name, get/set are used in code, when first painting is done then set is used to store value. On 2014/03/19 13:18:12, rude wrote: > Looks like you ended up not really using these for anything? > > Also, the plurality of the name "getTextDecorations" gives the wrong impression > here ... https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... File Source/core/rendering/style/RenderStyleConstants.h (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... Source/core/rendering/style/RenderStyleConstants.h:366: TextDecorationStyleNone, Yes as per CSS doc "none" is not a defined value, I used it as initial value when we do not know the decoration style. Assigning one of the values defined does not looked good to me. Pls suggest On 2014/03/19 13:18:12, rude wrote: > Are you sure you need this? > > The decoration style can't be "none" per spec, and I don't think the value is > needed internally either. Existence of decorations is controlled by > text-decoration-line (enum TextDecoration), not by text-decoration-style. > > See http://www.w3.org/TR/css-text-decor-3/#text-decoration-style-property
https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:1133: TextDecorationStyle style = TextDecorationStyleNone; I'm not sure it's wise to add things just because they may potentially become relevant later. Besides, it's wrong to return a single TextDecorationStyle. A text node may have any number of decorations, and therefore styles (though in our implementation we are currently limited to three). I suggest that you collect the decoration thickness *only*, and don't bother with the TextDecorationStyle at all. (In which case getPaintDecorationStyle should probably be renamed to getPaintDecorationThickness). https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... File Source/core/rendering/style/RenderStyleConstants.h (right): https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... Source/core/rendering/style/RenderStyleConstants.h:366: TextDecorationStyleNone, The correct value in that case is TextDecorationStyleSolid. (See spec). (Although I suggest that you drop this chunk, and don't mess with TextDecorationStyle at all in this patch. See other comment).
New code is added with comment fix and new test case On 2014/03/19 14:39:43, rude wrote: > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... > File Source/core/rendering/InlineFlowBox.cpp (right): > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... > Source/core/rendering/InlineFlowBox.cpp:1133: TextDecorationStyle style = > TextDecorationStyleNone; > I'm not sure it's wise to add things just because they may potentially become > relevant later. > > Besides, it's wrong to return a single TextDecorationStyle. A text node may have > any number of decorations, and therefore styles (though in our implementation we > are currently limited to three). > > I suggest that you collect the decoration thickness *only*, and don't bother > with the TextDecorationStyle at all. (In which case getPaintDecorationStyle > should probably be renamed to getPaintDecorationThickness). > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... > File Source/core/rendering/style/RenderStyleConstants.h (right): > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... > Source/core/rendering/style/RenderStyleConstants.h:366: TextDecorationStyleNone, > The correct value in that case is TextDecorationStyleSolid. (See spec). > > (Although I suggest that you drop this chunk, and don't mess with > TextDecorationStyle at all in this patch. See other comment).
Pls review. On 2014/03/20 09:42:51, h.joshi wrote: > New code is added with comment fix and new test case > > On 2014/03/19 14:39:43, rude wrote: > > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... > > File Source/core/rendering/InlineFlowBox.cpp (right): > > > > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/Inline... > > Source/core/rendering/InlineFlowBox.cpp:1133: TextDecorationStyle style = > > TextDecorationStyleNone; > > I'm not sure it's wise to add things just because they may potentially become > > relevant later. > > > > Besides, it's wrong to return a single TextDecorationStyle. A text node may > have > > any number of decorations, and therefore styles (though in our implementation > we > > are currently limited to three). > > > > I suggest that you collect the decoration thickness *only*, and don't bother > > with the TextDecorationStyle at all. (In which case getPaintDecorationStyle > > should probably be renamed to getPaintDecorationThickness). > > > > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... > > File Source/core/rendering/style/RenderStyleConstants.h (right): > > > > > https://codereview.chromium.org/203273003/diff/1/Source/core/rendering/style/... > > Source/core/rendering/style/RenderStyleConstants.h:366: > TextDecorationStyleNone, > > The correct value in that case is TextDecorationStyleSolid. (See spec). > > > > (Although I suggest that you drop this chunk, and don't mess with > > TextDecorationStyle at all in this patch. See other comment).
Not LGTM for the huge block of code duplication. It seems like you really just want to figure out the underline thickness in advance then still just feed that into the old paint method. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineBox.h (right): https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineBox.h:100: virtual void getPaintDecorationSyle(PaintInfo&, const LayoutPoint&, float *) { }; This function name isn't very informative to me. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1136: float decorationThickness = 0; These should be scoped inside the if. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1139: // Paint our childrens and calculate underline thickness s/childrens/children/, but I don't think this comment adds much. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1144: curr->getPaintDecorationSyle(childInfo, paintOffset, &decorationThickness); Why not have this method return decorationThickness instead of using an out parameter? Then you could do something like max(decorationThickness, curr->getPaintDecocrationStyle) https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1150: // We have underline thickness now, let draw it s/let/lets/, but again I don't think this comment is helpful. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineFlowBox.h (right): https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.h:119: virtual void getPaintDecorationSyle(PaintInfo&, const LayoutPoint&, float *) { }; Why are you overriding an empty method with more empty methods? Also, when overriding a virtual function, use the OVERRIDE keyword. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:757: float textDecorationThickness = styleToUse->fontMetrics().underlineThickness(); Don't copy and paste this block of code. If you need it in both places create a function. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:1105: if (!decorationThickness) { I don't fully understand the case when we have to recalculate this (as in, it's not passed in). Help me understand. https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:1168: void InlineTextBox::paintDecorationStyle(PaintInfo& paintInfo, const LayoutPoint& paintOffset, LayoutUnit lineTop, LayoutUnit lineBottom, float decorationThickness) Whoa... This is a *ton* of code duplication, no?
I'm not lgtm for the over-through-under case. I see no reason to have the line thickness be the same unless the line is contiguous. Right now it looks bad. I would personally also prefer (a priori) the median thickness for the contiguous lines.
I am working on to remove all duplicate code, will also work on the comments given. On 2014/03/21 20:43:07, Levi wrote: > Not LGTM for the huge block of code duplication. It seems like you really just > want to figure out the underline thickness in advance then still just feed that > into the old paint method. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > File Source/core/rendering/InlineBox.h (right): > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineBox.h:100: virtual void > getPaintDecorationSyle(PaintInfo&, const LayoutPoint&, float *) { }; > This function name isn't very informative to me. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > File Source/core/rendering/InlineFlowBox.cpp (right): > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineFlowBox.cpp:1136: float decorationThickness = 0; > These should be scoped inside the if. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineFlowBox.cpp:1139: // Paint our childrens and > calculate underline thickness > s/childrens/children/, but I don't think this comment adds much. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineFlowBox.cpp:1144: > curr->getPaintDecorationSyle(childInfo, paintOffset, &decorationThickness); > Why not have this method return decorationThickness instead of using an out > parameter? Then you could do something like max(decorationThickness, > curr->getPaintDecocrationStyle) > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineFlowBox.cpp:1150: // We have underline thickness > now, let draw it > s/let/lets/, but again I don't think this comment is helpful. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > File Source/core/rendering/InlineFlowBox.h (right): > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineFlowBox.h:119: virtual void > getPaintDecorationSyle(PaintInfo&, const LayoutPoint&, float *) { }; > Why are you overriding an empty method with more empty methods? Also, when > overriding a virtual function, use the OVERRIDE keyword. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > File Source/core/rendering/InlineTextBox.cpp (right): > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineTextBox.cpp:757: float textDecorationThickness = > styleToUse->fontMetrics().underlineThickness(); > Don't copy and paste this block of code. If you need it in both places create a > function. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineTextBox.cpp:1105: if (!decorationThickness) { > I don't fully understand the case when we have to recalculate this (as in, it's > not passed in). Help me understand. > > https://codereview.chromium.org/203273003/diff/20001/Source/core/rendering/In... > Source/core/rendering/InlineTextBox.cpp:1168: void > InlineTextBox::paintDecorationStyle(PaintInfo& paintInfo, const LayoutPoint& > paintOffset, LayoutUnit lineTop, LayoutUnit lineBottom, float > decorationThickness) > Whoa... This is a *ton* of code duplication, no?
Okey, will work on above comments. I had this doubt to apply uniform underline to continuous text or to complete text on one line. Will cross check behavior of Word software and share observations. On 2014/03/21 22:55:36, Stephen Chenney wrote: > I'm not lgtm for the over-through-under case. I see no reason to have the line > thickness be the same unless the line is contiguous. Right now it looks bad. I > would personally also prefer (a priori) the median thickness for the contiguous > lines.
On 2014/03/23 13:25:46, h.joshi wrote: > Okey, will work on above comments. I had this doubt to apply uniform underline > to continuous text or to complete text on one line. Will cross check behavior of > Word software and share observations. What do other browsers do? IE in particular, and Firefox 28 Beta as rude@ suggested? We seek consistency on the web platform. We look at non-web software only when there is no apparent web standard at all.
Checked on IE 11 and Firefox 29 beta. This feature is not present in Firefox 29 beta, 29 beta is behaving same way as current stable release version of Firefox. IE has implemented this, and as suggested in previous comment we need to have line thickness same for continuous underline text, IE seems to be taking maximum thickness into consideration. Will make required changes. On 2014/03/24 14:58:48, Stephen Chenney wrote: > On 2014/03/23 13:25:46, h.joshi wrote: > > Okey, will work on above comments. I had this doubt to apply uniform underline > > to continuous text or to complete text on one line. Will cross check behavior > of > > Word software and share observations. > > What do other browsers do? IE in particular, and Firefox 28 Beta as rude@ > suggested? > > We seek consistency on the web platform. We look at non-web software only when > there is no apparent web standard at all.
New patch is added with comment fix and making line thickness same for continuous decorative text. Pls review. On 2014/03/26 17:48:01, hj wrote: > Checked on IE 11 and Firefox 29 beta. This feature is not present in Firefox 29 > beta, 29 beta is behaving same way as current stable release version of Firefox. > IE has implemented this, and as suggested in previous comment we need to have > line thickness same for continuous underline text, IE seems to be taking maximum > thickness into consideration. > Will make required changes. > > On 2014/03/24 14:58:48, Stephen Chenney wrote: > > On 2014/03/23 13:25:46, h.joshi wrote: > > > Okey, will work on above comments. I had this doubt to apply uniform > underline > > > to continuous text or to complete text on one line. Will cross check > behavior > > of > > > Word software and share observations. > > > > What do other browsers do? IE in particular, and Firefox 28 Beta as rude@ > > suggested? > > > > We seek consistency on the web platform. We look at non-web software only when > > there is no apparent web standard at all.
Pls review this patch, after this patch Chrome and IE-11 have same functionality. On 2014/03/27 09:41:54, h.joshi wrote: > New patch is added with comment fix and making line thickness same for > continuous decorative text. > Pls review. > > On 2014/03/26 17:48:01, hj wrote: > > Checked on IE 11 and Firefox 29 beta. This feature is not present in Firefox > 29 > > beta, 29 beta is behaving same way as current stable release version of > Firefox. > > IE has implemented this, and as suggested in previous comment we need to have > > line thickness same for continuous underline text, IE seems to be taking > maximum > > thickness into consideration. > > Will make required changes. > > > > On 2014/03/24 14:58:48, Stephen Chenney wrote: > > > On 2014/03/23 13:25:46, h.joshi wrote: > > > > Okey, will work on above comments. I had this doubt to apply uniform > > underline > > > > to continuous text or to complete text on one line. Will cross check > > behavior > > > of > > > > Word software and share observations. > > > > > > What do other browsers do? IE in particular, and Firefox 28 Beta as rude@ > > > suggested? > > > > > > We seek consistency on the web platform. We look at non-web software only > when > > > there is no apparent web standard at all.
Pls review, shared comments are fixed with new patch. On 2014/04/03 17:26:37, h.joshi wrote: > Pls review this patch, after this patch Chrome and IE-11 have same > functionality. > > > On 2014/03/27 09:41:54, h.joshi wrote: > > New patch is added with comment fix and making line thickness same for > > continuous decorative text. > > Pls review. > > > > On 2014/03/26 17:48:01, hj wrote: > > > Checked on IE 11 and Firefox 29 beta. This feature is not present in Firefox > > 29 > > > beta, 29 beta is behaving same way as current stable release version of > > Firefox. > > > IE has implemented this, and as suggested in previous comment we need to > have > > > line thickness same for continuous underline text, IE seems to be taking > > maximum > > > thickness into consideration. > > > Will make required changes. > > > > > > On 2014/03/24 14:58:48, Stephen Chenney wrote: > > > > On 2014/03/23 13:25:46, h.joshi wrote: > > > > > Okey, will work on above comments. I had this doubt to apply uniform > > > underline > > > > > to continuous text or to complete text on one line. Will cross check > > > behavior > > > > of > > > > > Word software and share observations. > > > > > > > > What do other browsers do? IE in particular, and Firefox 28 Beta as rude@ > > > > suggested? > > > > > > > > We seek consistency on the web platform. We look at non-web software only > > when > > > > there is no apparent web standard at all.
Pls review, this is now pending for 3 weeks. On 2014/04/08 12:32:35, hj wrote: > Pls review, shared comments are fixed with new patch. > > On 2014/04/03 17:26:37, h.joshi wrote: > > Pls review this patch, after this patch Chrome and IE-11 have same > > functionality. > > > > > > On 2014/03/27 09:41:54, h.joshi wrote: > > > New patch is added with comment fix and making line thickness same for > > > continuous decorative text. > > > Pls review. > > > > > > On 2014/03/26 17:48:01, hj wrote: > > > > Checked on IE 11 and Firefox 29 beta. This feature is not present in > Firefox > > > 29 > > > > beta, 29 beta is behaving same way as current stable release version of > > > Firefox. > > > > IE has implemented this, and as suggested in previous comment we need to > > have > > > > line thickness same for continuous underline text, IE seems to be taking > > > maximum > > > > thickness into consideration. > > > > Will make required changes. > > > > > > > > On 2014/03/24 14:58:48, Stephen Chenney wrote: > > > > > On 2014/03/23 13:25:46, h.joshi wrote: > > > > > > Okey, will work on above comments. I had this doubt to apply uniform > > > > underline > > > > > > to continuous text or to complete text on one line. Will cross check > > > > behavior > > > > > of > > > > > > Word software and share observations. > > > > > > > > > > What do other browsers do? IE in particular, and Firefox 28 Beta as > rude@ > > > > > suggested? > > > > > > > > > > We seek consistency on the web platform. We look at non-web software > only > > > when > > > > > there is no apparent web standard at all.
Pls review, this patch is 1 month old now
On 2014/05/17 07:17:11, h.joshi wrote: > Pls review, this patch is 1 month old now Note this patch, https://codereview.chromium.org/275333002/, and the associated discussion. It would be truly excellent if everyone could work together to fix the width and positioning issues for a common underline under different fonts.
@Stephen Chrnney: I have added one commnet on https://codereview.chromium.org/275333002/ Pls revie this patch.
After taking a look into this, I believe that it is better to compute a single underline position for multiple adjacent TextBoxes but to allow the thickness to vary. In addition, this sort of logic should probably not happen during painting, but during layout. For example it seems the best place to calculate the underline position would be in InlineFlowBox::computeLogicalBoxHeights and RootInlineBox::ascentAndDescentForBox. This is for two reasons. First, these already have the (font metric) information at hand. RootInlineBox::ascentAndDescentForBox can be more seen as 'figure out the box metrics', and one of those metrics can be the underline position (and other such metrics in the future in addition to the current ascent and descent). InlineFlowBox::computeLogicalBoxHeights can be seen as 'combine a bunch of box metrics and adjust as needed' and can 'clean up' the computed values of the underline position along multiple adjacent boxes. The second is that CSS is itself moving toward eventually cleaning up information about text decorations, with recent proposals for setting (and of course querying the computed values of) text decoration locations. This makes quite a bit of sense actually, but does mean that the values need to be computed as part of laying out, and not just computed as needed later during paint. @schenney: Do InlineFlowBox::computeLogicalBoxHeights and RootInlineBox::ascentAndDescentForBox seem like the right place for this sort of thing? If you know of a better place I'd like to know as I may take a stab at cleaning this up. Or maybe I can conscript fmalita to help out a bit.
On 2014/08/03 02:25:50, bungeman2 wrote: > After taking a look into this, I believe that it is better to compute a single > underline position for multiple adjacent TextBoxes but to allow the thickness to > vary. In addition, this sort of logic should probably not happen during > painting, but during layout. For example it seems the best place to calculate > the underline position would be in InlineFlowBox::computeLogicalBoxHeights and > RootInlineBox::ascentAndDescentForBox. > > This is for two reasons. First, these already have the (font metric) information > at hand. RootInlineBox::ascentAndDescentForBox can be more seen as 'figure out > the box metrics', and one of those metrics can be the underline position (and > other such metrics in the future in addition to the current ascent and descent). > InlineFlowBox::computeLogicalBoxHeights can be seen as 'combine a bunch of box > metrics and adjust as needed' and can 'clean up' the computed values of the > underline position along multiple adjacent boxes. The second is that CSS is > itself moving toward eventually cleaning up information about text decorations, > with recent proposals for setting (and of course querying the computed values > of) text decoration locations. This makes quite a bit of sense actually, but > does mean that the values need to be computed as part of laying out, and not > just computed as needed later during paint. > > @schenney: Do InlineFlowBox::computeLogicalBoxHeights and > RootInlineBox::ascentAndDescentForBox seem like the right place for this sort of > thing? If you know of a better place I'd like to know as I may take a stab at > cleaning this up. Or maybe I can conscript fmalita to help out a bit. fmalita would be the man to talk to now, or jbroman. I'm not super familiar with line box layout and fmalita has been in the space more recently. Having said that, I don't think anyone is super familiar with the code.
> fmalita would be the man to talk to now, or jbroman. I'm not super familiar with line box layout and fmalita has been in the space more recently. Having said that, I don't think anyone is super familiar with the code. @fmalita, can you comment on this patch? I'd love to help move it forward.
On 2014/09/16 00:25:55, paulirish wrote: > > fmalita would be the man to talk to now, or jbroman. I'm not super familiar > with line box layout and fmalita has been in the space more recently. Having > said that, I don't think anyone is super familiar with the code. > > > @fmalita, can you comment on this patch? I'd love to help move it forward. FYI, the underline change that triggered this has been reverted. |
