Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
950623002:
Store resolved color in AppliedTextDecoration (Closed)
DescriptionStore resolved color in AppliedTextDecoration
Now AppliedTextDecoration store resolved color instead of currentColor.
This patch also adjust the simple underline optimization
so that it is still valid. Now optimization works as long
as node underline decoration style is not Invalidated.
BUG=460698
TEST=LayoutTests/fast/css/text-decoration-color-on-hover.html
Patch Set 1 #
Total comments: 2
Patch Set 2 : Patch in progress(simple underline optimization removed) #Patch Set 3 : Updated patch with underline optimization handling #
Total comments: 8
Patch Set 4 : Applied review comments. #
Total comments: 5
Patch Set 5 : removed inherited_flags.m_textunderline #
Total comments: 6
Messages
Total messages: 45 (3 generated)
samahto@cisco.com changed reviewers: + alancutter@chromium.org, andersr@opera.com, philipj@opera.com, timloh@chromium.org
plz review this.
I'm not sure what the actual bug here is since the two function calls are very similar (visitedDependentColor calls into decorationColor.. eventually anyway). Could you explain what's actually going on here? :) Also if possible it would be better to use a layout test which doesn't depend on mouse movement. https://codereview.chromium.org/950623002/diff/1/Source/core/layout/style/Lay... File Source/core/layout/style/LayoutStyle.cpp (right): https://codereview.chromium.org/950623002/diff/1/Source/core/layout/style/Lay... Source/core/layout/style/LayoutStyle.cpp:1293: StyleColor styleColor = visitedDependentColor(CSSPropertyTextDecorationColor); visitedDependentColor returns a Color and not a StyleColor, so this looks a bit weird.
On 2015/02/22 at 22:49:37, timloh wrote: > Also if possible it would be better to use a layout test which doesn't depend on mouse movement. Is there a way to change hover state without mouse movement?
On 2015/02/24 03:58:34, pdr wrote: > On 2015/02/22 at 22:49:37, timloh wrote: > > Also if possible it would be better to use a layout test which doesn't depend > on mouse movement. > > Is there a way to change hover state without mouse movement? I assume @timloh's point was that the test doesn't need to depend on :hover/mousemove and could use e.g. a class change instead, to avoid the need for interaction also when not run as a layout test.
https://codereview.chromium.org/950623002/diff/1/Source/core/layout/style/Lay... File Source/core/layout/style/LayoutStyle.cpp (right): https://codereview.chromium.org/950623002/diff/1/Source/core/layout/style/Lay... Source/core/layout/style/LayoutStyle.cpp:1293: StyleColor styleColor = visitedDependentColor(CSSPropertyTextDecorationColor); On 2015/02/22 22:49:37, Timothy Loh wrote: > visitedDependentColor returns a Color and not a StyleColor, so this looks a bit > weird. Acknowledged.
while debugging issue i found decorationColorIncludingFallback return wrong color but visitedDependentColor return correct color. this is happening becasue decorationColorIncludingFallback doesn't resolve currentcolor to default color.I guess it is missed due to patch https://codereview.chromium.org/746173002/ Before this, https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... line: 1407 we use to resolve the currentcolor.But Now that is missed in decorationColorIncludingFallback VisitedDependentColor Handles the case due to last line in colorIncludingFallback(). see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... While debugging I found no style calc problem on hovering.
@Timothy I have put comment explaining things. I need your further view in this before proceeding further
andruud@gmail.com changed reviewers: + andruud@gmail.com
On 2015/02/25 04:06:04, samahto wrote: > Before this, > https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... > line: 1407 we use to resolve the currentcolor. But Now that > is missed in decorationColorIncludingFallback Look at line 1274. It used to call visitedDependentDecoration*Style*Color(), which did not resolve the color. Perhaps our style diffing fails to catch this case, causing missing paint invalidation? Although I don't remember why we store StyleColors for applied text decorations in the first place. We should be able to store resolved Colors, I think.
On 2015/02/25 17:40:51, andruud wrote: > On 2015/02/25 04:06:04, samahto wrote: > > Before this, > > > https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... > > line: 1407 we use to resolve the currentcolor. But Now that > > is missed in decorationColorIncludingFallback > > Look at line 1274. It used to call visitedDependentDecoration*Style*Color(), > which did not resolve the color. Perhaps our style diffing fails to catch this > case, causing missing paint invalidation? Right, you are correct in line 1274. since it retrieve styleColor so it need not be resolved.But as you said later it must be resolved later. just guess that, since text decoration is propagated to child inline text. we resolve the color in context with inline text on which decoration is actually applied. if we resolve early, then current color will be resolved with parent element on which textdecoration css property is specified. this way it propogates the textdecoration but avoid propagation of color to inline text on which actual decoration is applied. May be for same reason we don't store resolved color. What you think? then should we propagate the decoration color also to child ? if my assumption is correct. > Although I don't remember why we store StyleColors for applied text decorations > in the first place. We should be able to store resolved Colors, I think. @timloh & andruud. I saw we are using StyleColor and color interchangebly since styleColor constructor is not explicit. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... May be this arrogancy in conversion leads us to miss styleColor resolution.Should we tight this commonly used basic class?
On 2015/02/25 17:40:51, andruud wrote: > On 2015/02/25 04:06:04, samahto wrote: > > Before this, > > > https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... > > line: 1407 we use to resolve the currentcolor. But Now that > > is missed in decorationColorIncludingFallback > > Look at line 1274. It used to call visitedDependentDecoration*Style*Color(), > which did not resolve the color. Perhaps our style diffing fails to catch this > case, causing missing paint invalidation? > > Although I don't remember why we store StyleColors for applied text decorations > in the first place. We should be able to store resolved Colors, I think. Agreed that we should just be able to store Colors, but I'm not clear on why this doesn't work right now. Shouldn't our diff notice that the color has changed (inherited->color != other.inherited->color)?
On 2015/02/26 05:28:33, Timothy Loh wrote: > On 2015/02/25 17:40:51, andruud wrote: > > On 2015/02/25 04:06:04, samahto wrote: > > > Before this, > > > > > > https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... > > > line: 1407 we use to resolve the currentcolor. But Now that > > > is missed in decorationColorIncludingFallback > > > > Look at line 1274. It used to call visitedDependentDecoration*Style*Color(), > > which did not resolve the color. Perhaps our style diffing fails to catch this > > case, causing missing paint invalidation? > > > > Although I don't remember why we store StyleColors for applied text > decorations > > in the first place. We should be able to store resolved Colors, I think. > > Agreed that we should just be able to store Colors, but I'm not clear on why > this doesn't work right now. Shouldn't our diff notice that the color has > changed (inherited->color != other.inherited->color)? Nevermind, I realise now that the parent color changes but not the child's color, so only the parent would get marked as needing text/color invalidation. I guess the m_textUnderline optimisation won't work any more?
On 2015/02/26 08:14:57, Timothy Loh wrote: > On 2015/02/26 05:28:33, Timothy Loh wrote: > > On 2015/02/25 17:40:51, andruud wrote: > > > On 2015/02/25 04:06:04, samahto wrote: > > > > Before this, > > > > > > > > > > https://codereview.chromium.org/746173002/diff/1/Source/core/rendering/style/... > > > > line: 1407 we use to resolve the currentcolor. But Now that > > > > is missed in decorationColorIncludingFallback > > > > > > Look at line 1274. It used to call visitedDependentDecoration*Style*Color(), > > > which did not resolve the color. Perhaps our style diffing fails to catch > this > > > case, causing missing paint invalidation? > > > > > > Although I don't remember why we store StyleColors for applied text > > decorations > > > in the first place. We should be able to store resolved Colors, I think. > > > > Agreed that we should just be able to store Colors, but I'm not clear on why > > this doesn't work right now. Shouldn't our diff notice that the color has > > changed (inherited->color != other.inherited->color)? > > Nevermind, I realise now that the parent color changes but not the child's > color, so only the parent would get marked as needing text/color invalidation. I > guess the m_textUnderline optimisation won't work any more? I can see that paint is not having on hover even if color property is changed. Actually in remote inspector I enabled "show paint rectangle" but no repaint happening. if I enable "Enable Continuous page repainting" everything works fine.I mean underline become red perfectly.
There's a few outstanding issues here: - We should be able to test this by just changing for example an element's class. Probably a better direction is to change the color from red to green, since we tend to avoid red in passing tests. The text should just say something like "This test passes if the underline is green". - Change AppliedTextDecoration to store Color instead of StyleColor since we'll only be using resolved colors now. - Work out what to do with m_textUnderline. Either we should drop this optimisation completely (it relies on currentcolor not being resolved so it won't work with the current patch as is), or we should fix it. - Update patch title and description. The current title and description aren't very useful. We should mention the behaviour change, and saying something about storing resolved colors is probably a good idea too.
andruud == andersr, BTW. > - Change AppliedTextDecoration to store Color instead of StyleColor since we'll > only be using resolved colors now. Now I remember. We're storing StyleColors instead of Colors to be able to move the optimized simple case to the appliedTextDecorations list if the decorations become non-simple. (See LayoutStyle::addAppliedTextDecoration). I initially wanted to store Colors in AppliedTextDecoration, and resolve dynamically only for the m_textUnderline-case, but that led to a very inconsistent LayoutStyle API, and hence ugly painting code, so in the end everything was resolved dynamically. But yeah, we should store Colors. > - Work out what to do with m_textUnderline. Either we should drop this > optimisation completely (it relies on currentcolor not being resolved so it > won't work with the current patch as is), or we should fix it. It would suck to drop the optimization completely. We would have to store an extra Color and VisitedLinkColor quite often. How about this: During style adjustment for some element, if the parent has m_textUnderline set, convert that to an AppliedTextDecoration (resolving the color against the parent) before storing it on the style. This should make the optimization valid for the common case of underlined elements with only text children. This also has the benefit that the currentColor we're implicitly referring to in m_textUnderline actually refers to the currentColor of *that style*. This means that we don't have to traverse upwards while painting, and that certain assumptions about how currentColor is used w.r.t. style propagation actually hold.
On 2015/02/27 10:53:52, andersr wrote: > andruud == andersr, BTW. > > > - Change AppliedTextDecoration to store Color instead of StyleColor since > we'll > > only be using resolved colors now. > > Now I remember. We're storing StyleColors instead of Colors to be able to move > the optimized simple case to the appliedTextDecorations list if the decorations > become non-simple. (See LayoutStyle::addAppliedTextDecoration). > > I initially wanted to store Colors in AppliedTextDecoration, and resolve > dynamically only for the m_textUnderline-case, but that led to a very > inconsistent LayoutStyle API, and hence ugly painting code, so in the end > everything was resolved dynamically. > > But yeah, we should store Colors. > > > - Work out what to do with m_textUnderline. Either we should drop this > > optimisation completely (it relies on currentcolor not being resolved so it > > won't work with the current patch as is), or we should fix it. > > It would suck to drop the optimization completely. We would have to store an > extra Color and VisitedLinkColor quite often. > > How about this: > > During style adjustment for some element, if the parent has m_textUnderline set, > convert that to an AppliedTextDecoration (resolving the color against the > parent) before storing it on the style. This should make the optimization valid > for the common case of underlined elements with only text children. > > This also has the benefit that the currentColor we're implicitly referring to in > m_textUnderline actually refers to the currentColor of *that style*. This means > that we don't have to traverse upwards while painting, and that certain > assumptions about how currentColor is used w.r.t. style propagation actually > hold. I have updated the patch with simple underline optimization removed.although it may not be final based on what we finalize on handling of underline optimization. From my point of view regarding optimization I didn't found any measurable(debatable) optimization done by previous code. it looks to me from code that optimization was to save memory even though it is not much. Also putting textdecoration:underline in special category and handling in completely different code path appears confusing to me.it create diversion in impl of css3 textdeco impl and old css2 textdeco impl. comment if I am missing some conceptual concept. With current patch(optimization removed) it looks to me saving more memory since it removes inherited_flags bit(m_textunderline).so we don't have overhead where there is no textdecoration. and also code get more comfortable for future enhancements. I feel we should clearly define advantage level of underline optimization.
On 2015/03/01 20:02:28, samahto wrote: > I have updated the patch with simple underline optimization removed.although it > may not be final > based on what we finalize on handling of underline optimization. > > From my point of view regarding optimization I didn't found any > measurable(debatable) optimization done by > previous code. it looks to me from code that optimization was to save memory > even though it is not much. > Also putting textdecoration:underline in special category and handling in > completely different code path appears confusing to me.it create diversion in > impl of css3 textdeco impl and old css2 textdeco impl. > comment if I am missing some conceptual concept. > > With current patch(optimization removed) it looks to me saving more memory > since it removes inherited_flags bit(m_textunderline).so we don't have overhead > where there is no textdecoration. > and also code get more comfortable for future enhancements. > > I feel we should clearly define advantage level of underline optimization. Removing the flag doesn't actually make things smaller, since the compiler will pad the bitfield section to a multiple of 4 or 8 bytes. If we remove this optimisation, we might have to pay the cost of a AppliedTextDecorationList (40 bytes) + vector backing (32 bytes probably) + sometimes StyleRareNonInheritedData (176 bytes). On textual content, the fraction of elements with underlines can be very high. On the page http://www.w3.org/html/wg/drafts/html/master/single-page.html, over 30% of elements have underlines, so dropping the optimisation may cause us to take up to 10MB more memory on this page!
On 2015/03/09 07:16:39, Timothy Loh wrote: > On 2015/03/01 20:02:28, samahto wrote: > > I have updated the patch with simple underline optimization removed.although > it > > may not be final > > based on what we finalize on handling of underline optimization. > > > > From my point of view regarding optimization I didn't found any > > measurable(debatable) optimization done by > > previous code. it looks to me from code that optimization was to save memory > > even though it is not much. > > Also putting textdecoration:underline in special category and handling in > > completely different code path appears confusing to me.it create diversion in > > impl of css3 textdeco impl and old css2 textdeco impl. > > comment if I am missing some conceptual concept. > > > > With current patch(optimization removed) it looks to me saving more memory > > since it removes inherited_flags bit(m_textunderline).so we don't have > overhead > > where there is no textdecoration. > > and also code get more comfortable for future enhancements. > > > > I feel we should clearly define advantage level of underline optimization. > > Removing the flag doesn't actually make things smaller, since the compiler will > pad the bitfield section to a multiple of 4 or 8 bytes. If we remove this > optimisation, we might have to pay the cost of a AppliedTextDecorationList (40 > bytes) + vector backing (32 bytes probably) + sometimes > StyleRareNonInheritedData (176 bytes). On textual content, the fraction of > elements with underlines can be very high. > > On the page http://www.w3.org/html/wg/drafts/html/master/single-page.html, over > 30% of elements have underlines, so dropping the optimisation may cause us to > take up to 10MB more memory on this page! I was trying to find the solution for simple underline optimization. The best optimized way I could come out is to add another flags(1 bit) in inherited_flags (inherited_flags.m_textunderlineIsDirty). which is set whenever m_textUnderline becomes dirty(due to color or decorationstyle change). Child node use this flags to know decoration property change in case of simple underline and set paint Invalidation of current object. Right now inherited_flags is 42 bit. so we have space to fill the compiler optimized space. what you say about this approach? I investigated also to resolve final decoration styles at stylecalc. but could not find better simplified code for that and still it end up using AppliedtextDecoration which adds up memory. Other option is what andurud suggested. But still will lower the memory optimization.code become clumsy while resolving current color wrt parent( we need to hook up badly to access parents style) -----andurud---------------------------------------- How about this: During style adjustment for some element, if the parent has m_textUnderline set, convert that to an AppliedTextDecoration (resolving the color against the parent) before storing it on the style. This should make the optimization valid for the common case of underlined elements with only text children. -------------------------------------------------------------
Hi timothy, andruud Could you review the latest patch uploaded?
This still seems a bit complicated to me. It's probably easier if do what Anders said in his last comment. Instead of propagating m_textUnderline, only have that set on elements where the parent has no text decorations. This should also let us clean up the painting code for text-decorations and make it easier to fix bug 462142. https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:24: Hover me and then hover away There's no more hovering in this test. Just write something like "This test passes if the underline is green" (it's preferred to use green for passing and red for failing). Also the test file name needs to be updated. https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: testRunner.waitUntilDone(); We shouldn't use a setTimeout. Just call outspan.offsetTop before setting the className to force a style recalc, and remove all the testRunner calls. https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutObject.cpp:1603: style()->adjustTextDecorationDifference(); This seems a bit weird. I don't think we should be updating the style object in here. The call to this function isn't wrapped in styleWillChange/styleDidChange. https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/style... File Source/core/layout/style/LayoutStyle.cpp (right): https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/style... Source/core/layout/style/LayoutStyle.cpp:1325: int decorations = textDecoration(); Don't think we should have to check this. If you like we can assert that we have the TextDecorationUnderline bit set when m_textUnderline is set. https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/style... Source/core/layout/style/LayoutStyle.cpp:1332: TextDecorationStyle style = textDecorationStyle(); Is this always TextDecorationUnderline when we get here? https://codereview.chromium.org/950623002/diff/40001/Source/core/layout/style... Source/core/layout/style/LayoutStyle.cpp:1354: if (!rareInheritedData->appliedTextDecorations && underline.isSimpleUnderline(styleColor.isCurrentColor())) I think this is just if (!rareInheritedData->appliedTextDecorations && styleColor.isCurrentColor()))
Are you still working on this?
Hi Sorry I was out of touch for few days. I will update the bug soon. Regards santosh -----Original Message----- From: timloh@chromium.org [mailto:timloh@chromium.org] Sent: Wednesday, April 29, 2015 5:44 AM To: Santosh Mahto (samahto); andersr@opera.com; alancutter@chromium.org; philipj+reviewer@opera.com; andruud@gmail.com Cc: blink-reviews@chromium.org; blink-reviews-rendering@chromium.org; blink-reviews-style@chromium.org; zoltan@webkit.org; pdr+renderingwatchlist@chromium.org; eae+blinkwatch@chromium.org; leviw+renderwatch@chromium.org; dominik.rottsches@intel.com; jchaffraix+rendering@chromium.org; santoshbit2007@gmail.com Subject: Re: Use visitedDependentColor() to get text decoration color. (issue 950623002 by samahto@cisco.com) Are you still working on this? https://codereview.chromium.org/950623002/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: testRunner.waitUntilDone(); On 2015/03/25 00:26:23, Timothy Loh wrote: > We shouldn't use a setTimeout. Just call outspan.offsetTop before setting the > className to force a style recalc, and remove all the testRunner calls. Tried the same(outspan.offsetTop trick) but its seems to not working. Although style recalc is happening but we need repaint. it looks only one tree paint is happening at end which paint the inner span also with latest style. We had issue that inner span paint is not triggered after recalc so old textdeco color. any suggestions?
https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: testRunner.waitUntilDone(); On 2015/04/30 20:06:44, samahto wrote: > On 2015/03/25 00:26:23, Timothy Loh wrote: > > We shouldn't use a setTimeout. Just call outspan.offsetTop before setting the > > className to force a style recalc, and remove all the testRunner calls. > > Tried the same(outspan.offsetTop trick) but its seems to not working. Although > style recalc is happening but we need repaint. it looks only one tree paint is > happening at end which paint the inner span also with latest style. We had issue > that inner span paint is not triggered after recalc so old textdeco color. any > suggestions? Can we try using a requestAnimationFrame instead of a setTimeout?
On 2015/05/04 00:35:16, Timothy Loh wrote: > https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... > File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): > > https://codereview.chromium.org/950623002/diff/40001/LayoutTests/fast/css/tex... > LayoutTests/fast/css/text-decoration-color-on-hover.html:30: > testRunner.waitUntilDone(); > On 2015/04/30 20:06:44, samahto wrote: > > On 2015/03/25 00:26:23, Timothy Loh wrote: > > > We shouldn't use a setTimeout. Just call outspan.offsetTop before setting > the > > > className to force a style recalc, and remove all the testRunner calls. > > > > Tried the same(outspan.offsetTop trick) but its seems to not working. Although > > style recalc is happening but we need repaint. it looks only one tree paint is > > happening at end which paint the inner span also with latest style. We had > issue > > that inner span paint is not triggered after recalc so old textdeco color. any > > suggestions? > > Can we try using a requestAnimationFrame instead of a setTimeout? Updated the patch.
Please don't remove the automatically added CC-ed people and lists. I think we should move m_textUnderline to be non-inherited, since the value never propagates down the tree. Otherwise there might be issues with styles being adjusted multiple times (according to alancutter, we need style adjustment to be idempotent). https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: function busyWait(millis) { This doesn't seem right. Why do we do this? https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:35: var iterationCount = 0; Are you sure this doesn't work with a single rAF?
On 2015/05/05 06:51:18, Timothy Loh wrote: > Please don't remove the automatically added CC-ed people and lists. > > I think we should move m_textUnderline to be non-inherited, since the value > never propagates down the tree. Otherwise there might be issues with styles > being adjusted multiple times (according to alancutter, we need style adjustment > to be idempotent). Fine. But Will bit alignment be issue in that case ?
https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: function busyWait(millis) { On 2015/05/05 06:51:18, Timothy Loh wrote: > This doesn't seem right. Why do we do this? To simulate testrunner.WaitUntilDone(). I had found while coding this that test result comparison doesn't wait for second repaint finish. so manually making it work. I saw animation and repaint available tests each of them uses waitUnitlDone(). I found this trick in animation tests https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:35: var iterationCount = 0; On 2015/05/05 06:51:18, Timothy Loh wrote: > Are you sure this doesn't work with a single rAF? Yes, with single rAF behaviour is as single paint.
https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:30: function busyWait(millis) { On 2015/05/05 15:45:51, samahto wrote: > On 2015/05/05 06:51:18, Timothy Loh wrote: > > This doesn't seem right. Why do we do this? > > To simulate testrunner.WaitUntilDone(). I had found while coding this > that test result comparison doesn't wait for second repaint finish. > so manually making it work. I saw animation and repaint available tests > each of them uses waitUnitlDone(). I found this trick in animation tests Sorry if I wasn't clear earlier, it'd be fine to use testRunner if we need to rAF. Perhaps this would be better to instead use one of the helpers in fast/repaint/resources though?
On 2015/05/05 06:51:18, Timothy Loh wrote: > Please don't remove the automatically added CC-ed people and lists. > > I think we should move m_textUnderline to be non-inherited, since the value > never propagates down the tree. Otherwise there might be issues with styles > being adjusted multiple times (according to alancutter, we need style adjustment > to be idempotent). On moving,now m_textUnderline has not much significance. we can use visual.textdecoration(which also solves somewhat redundant information in style about textdecoration). I have updated the patch for review > https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... > File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): > > https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... > LayoutTests/fast/css/text-decoration-color-on-hover.html:30: function > busyWait(millis) { > This doesn't seem right. Why do we do this? > > https://codereview.chromium.org/950623002/diff/60001/LayoutTests/fast/css/tex... > LayoutTests/fast/css/text-decoration-color-on-hover.html:35: var iterationCount > = 0; > Are you sure this doesn't work with a single rAF?
https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... File Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... Source/core/style/ComputedStyle.cpp:1334: if (hasOutOfFlowPosition() || isFloating()) { [spec says]: "text decorations are not propagated to floating and absolutely positioned descendants, nor to the contents of atomic inline-level descendants such as inline blocks and inline tables." The above line handles this case.Hope to get some suggestion if better things can be done here
On 2015/05/17 17:12:40, samahto wrote: > https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... > File Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... > Source/core/style/ComputedStyle.cpp:1334: if (hasOutOfFlowPosition() || > isFloating()) { > [spec says]: "text decorations are not propagated to floating and absolutely > positioned descendants, nor to the contents of atomic inline-level descendants > such as inline blocks and inline tables." > > The above line handles this case.Hope to get some suggestion if better things > can be done here Hi timothy Please review this patch.
On 2015/05/17 17:12:40, samahto wrote: > https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... > File Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... > Source/core/style/ComputedStyle.cpp:1334: if (hasOutOfFlowPosition() || > isFloating()) { > [spec says]: "text decorations are not propagated to floating and absolutely > positioned descendants, nor to the contents of atomic inline-level descendants > such as inline blocks and inline tables." > > The above line handles this case.Hope to get some suggestion if better things > can be done here Hi timothy Please review this patch.
+wkorman to cc, sounds like you might be looking at text decorations now?
On 2015/06/22 at 01:33:54, timloh wrote: > +wkorman to cc, sounds like you might be looking at text decorations now? I picked up a few dangling bugs from a previous owner who's moved on, at minimum to help route but yes I'm happy to work on them as well as time permits. The area is new to me so I'm best as observer currently but I'll send a few brief comments from reading.
wkorman@chromium.org changed reviewers: + wkorman@chromium.org
https://codereview.chromium.org/950623002/diff/80001/LayoutTests/fast/css/tex... File LayoutTests/fast/css/text-decoration-color-on-hover.html (right): https://codereview.chromium.org/950623002/diff/80001/LayoutTests/fast/css/tex... LayoutTests/fast/css/text-decoration-color-on-hover.html:14: .nextouter{ space before { https://codereview.chromium.org/950623002/diff/80001/Source/core/paint/Inline... File Source/core/paint/InlineTextBoxPainter.cpp (right): https://codereview.chromium.org/950623002/diff/80001/Source/core/paint/Inline... Source/core/paint/InlineTextBoxPainter.cpp:682: switch (decoration.style()) { Was there a reason we weren't using accessors previously, or just legacy code? https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Applie... File Source/core/style/AppliedTextDecoration.cpp (right): https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Applie... Source/core/style/AppliedTextDecoration.cpp:18: : m_line(decoration) Should member data be renamed to suit? https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... File Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... Source/core/style/ComputedStyle.cpp:1334: if (hasOutOfFlowPosition() || isFloating()) { On 2015/05/17 at 17:12:40, samahto wrote: > [spec says]: "text decorations are not propagated to floating and absolutely positioned descendants, nor to the contents of atomic inline-level descendants such as inline blocks and inline tables." > > The above line handles this case.Hope to get some suggestion if better things can be done here Add layout tests to validate this works as expected? https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... File Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/950623002/diff/80001/Source/core/style/Comput... Source/core/style/ComputedStyle.h:574: bool isSimpleUnderlineDecoration() const { return visual->textDecoration & TextDecorationUnderline; } Suggest adding comment explaining more on what simple vs non-simple underlines are, so future eng reading have an easier time understanding.
Ping? This review looked promising.
On 2015/10/26 22:47:08, pdr wrote: > Ping? This review looked promising. sashab@ has been looking at this in https://codereview.chromium.org/1328283005/
Message was sent while issue was closed.
Closing this issue as it has been superseded by https://codereview.chromium.org/1328283005/ (although that one is stalled at the moment too..)
Message was sent while issue was closed.
Closing this issue as it has been superseded by https://codereview.chromium.org/1328283005/ (although that one is stalled at the moment too..) |
