Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1455153003:
Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' (Closed)
|
Created:
5 years, 1 month ago by hyunjunekim2 Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, krit, eae+blinkwatch, f(malita), gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke'
When animating 'color', 'fill' or 'stroke' properties with the value
'currentColor' doesn't repaint properly, or with the right color.
For SVG elements which has 'currentColor' as part of their 'fill' or
'stroke' property, make sure they get repainted when 'color' changes.
BUG=537807
Committed: https://crrev.com/ba6fc5130a3ca06c653ece942a19d06e64be187d
Cr-Commit-Position: refs/heads/master@{#367286}
Patch Set 1 #Patch Set 2 : Draft #Patch Set 3 : Draft2 #Patch Set 4 : Draft3 #
Total comments: 3
Patch Set 5 : #Patch Set 6 : Draft4 #
Total comments: 8
Patch Set 7 : Draft5 #Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 9
Patch Set 12 : modify a test. #
Total comments: 2
Patch Set 13 : #Patch Set 14 : indentation #Patch Set 15 : Just Draft #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : Remove color-prop-05-t. #Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #
Total comments: 3
Patch Set 25 : #Patch Set 26 : rebase it #Patch Set 27 : split #Patch Set 28 : split 2 #
Total comments: 2
Patch Set 29 : #
Messages
Total messages: 80 (22 generated)
Description was changed from ========== WIP BUG=537807 ========== to ========== WIP BUG=537807 ==========
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org
pdr, fs, Hi, i'm hyunjune kim. Could you check this patch or give me a advice? Thanks.
For some reason I'm no longer allowed to look at this bug (=/), but I think I recall what it was about... Also, can we get a test for this? https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:380: if (state.element()->isSVGElement()) { I think it might be a better idea to just resolve currentColor when we setup the paint (LayoutSVGResourcePaintServer.cpp:requestPaint). Also, IIRC we normalize away currentColor in CSSAnimatableValueFactory, so that may need adjusting too. https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right): https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/SVGComputedStyle.h:360: bool isFillColorCurrentColor() const { return (fillPaintType() == SVG_PAINTTYPE_CURRENTCOLOR) || (visitedLinkFillPaintType() == SVG_PAINTTYPE_CURRENTCOLOR); } You need to also check for a currentColor fallback (SVG_PAINTTYPE_URI_CURRENTCOLOR IIRC).
fs, Okay, I will check it.
https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:380: if (state.element()->isSVGElement()) { On 2015/11/20 16:27:14, fs wrote: > I think it might be a better idea to just resolve currentColor when we setup the > paint (LayoutSVGResourcePaintServer.cpp:requestPaint). Also, IIRC we normalize > away currentColor in CSSAnimatableValueFactory, so that may need adjusting too. fs, Hi, I have a question and want a advice. I tried to implement on LayoutSVGResourcePaintServer.cpp::requestPaint for resolving fill or stroke color. But have a problem. If there is currentColor on SVG Element's attribute not css style, How to distinguish between CSS and SVG Element's attribute? For example, == CSS case == circle { fill: currentColor; transition: 2s; color: blue; } <body> <svg xmlns="http://www.w3.org/2000/svg" width=90 height=90> <circle id="circle" r="40" cx="40" cy="40" /> </svg> </body> == SVG Element Attribute == <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif" font-size="18"> <g fill="currentColor" color="lime"> <rect x="120" y="60" width="150" height="150" color="red"/> </g> </g> Thank you.
On 2015/11/24 at 08:07:36, hyunjune.kim wrote: > https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): > > https://codereview.chromium.org/1455153003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:380: if (state.element()->isSVGElement()) { > On 2015/11/20 16:27:14, fs wrote: > > I think it might be a better idea to just resolve currentColor when we setup the > > paint (LayoutSVGResourcePaintServer.cpp:requestPaint). Also, IIRC we normalize > > away currentColor in CSSAnimatableValueFactory, so that may need adjusting too. > > fs, Hi, I have a question and want a advice. I tried to implement on LayoutSVGResourcePaintServer.cpp::requestPaint for resolving fill or stroke color. > But have a problem. If there is currentColor on SVG Element's attribute not css style, How to distinguish between CSS and SVG Element's attribute? > For example, > == CSS case == > circle { > fill: currentColor; > transition: 2s; > color: blue; > } > <body> > <svg xmlns="http://www.w3.org/2000/svg" width=90 height=90> > <circle id="circle" r="40" cx="40" cy="40" /> > </svg> > </body> > > == SVG Element Attribute == > <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif" font-size="18"> > <g fill="currentColor" color="lime"> > <rect x="120" y="60" width="150" height="150" color="red"/> > </g> > </g> Since 'fill' (and 'stroke') are presentation attributes, these two cases can be handled in the same way, since they are both applied through the style system (and hence affect the value in ComputedStyle.) Similarly for 'color' itself.
fs, I tried to resolve currentColor. But for resolving on requestPaint, need to
handle whether SVG+CSS and originally SVG.
I analyzed this status.
Then SVG+CSS,
> circle {
> fill: currentColor;
> transition: 2s;
> color: blue;
> }
> <body>
> <svg xmlns="http://www.w3.org/2000/svg" width=90 height=90>
> <circle id="circle" r="40" cx="40" cy="40" />
> </svg>
> </body>
The value(color) was sat on style.color only about CSSPropertyColor.
but wasn't sat currentColor on svgStyle.fillcolor.
And on SVG,
> <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif"
font-size="18">
> <g fill="currentColor" color="lime">
> <rect x="120" y="60" width="150" height="150" color="red"/>
> </g>
> </g>
The value(color) was sat on svgStyle.fillcolor only as "lime"
But wasn't sat currentColor on style.color just "red".
So if just return style.color on requestPaint, the second
example(svg/W3C-SVG-1.1-SE/color-prop-05-t.svg) is fail.
and early i tried to resolve value on requestPaint as PS1 and PS2.
Now I want to find how to distinguish whether SVG+CSS(first example) and
originally SVG(second example).
Could you give me a advice? Thank you for your help. :)
On 2015/11/25 at 10:15:12, hyunjune.kim wrote:
> fs, I tried to resolve currentColor. But for resolving on requestPaint, need
to handle whether SVG+CSS and originally SVG.
>
> I analyzed this status.
>
> Then SVG+CSS,
> > circle {
> > fill: currentColor;
> > transition: 2s;
> > color: blue;
> > }
> > <body>
> > <svg xmlns="http://www.w3.org/2000/svg" width=90 height=90>
> > <circle id="circle" r="40" cx="40" cy="40" />
> > </svg>
> > </body>
> The value(color) was sat on style.color only about CSSPropertyColor.
> but wasn't sat currentColor on svgStyle.fillcolor.
>
> And on SVG,
> > <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif"
> font-size="18">
> > <g fill="currentColor" color="lime">
> > <rect x="120" y="60" width="150" height="150" color="red"/>
> > </g>
> > </g>
> The value(color) was sat on svgStyle.fillcolor only as "lime"
> But wasn't sat currentColor on style.color just "red".
>
> So if just return style.color on requestPaint, the second
example(svg/W3C-SVG-1.1-SE/color-prop-05-t.svg) is fail.
> and early i tried to resolve value on requestPaint as PS1 and PS2.
> Now I want to find how to distinguish whether SVG+CSS(first example) and
originally SVG(second example).
This is not really an "SVG+CSS vs. SVG" comparison - more of an illustration of
why 'currentColor' ought to resolve at a later stage (than it currently does).
In this last case the 'color="red"' never gets applied properly (I'm guessing)
because we don't notice that change in context wrt 'color'.
I think PS1 so far is the closest, but it doesn't actually resolve currentColor
- it should read style.color() though (not fillPaintColor).
ComputedStyle::visitedDependentColor / ComputedStyle::colorIncludingFallback
should be good "guides".
fs, Could you check PS6? Thank you :)
Please add a test. https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:174: if (isFillCurrentColor || isStrokeCurrentColor) Why do you do this? https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:97: if (!color.alpha()) I don't see why you need this. https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/SVGComputedStyle.h:362: return (fillPaintType() == SVG_PAINTTYPE_CURRENTCOLOR) Drop the redundant (). https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/SVGComputedStyle.h:370: return (strokePaintType() == SVG_PAINTTYPE_CURRENTCOLOR) Drop () (like above)
Ok, i am going to add test cases. https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:174: if (isFillCurrentColor || isStrokeCurrentColor) On 2015/11/25 17:23:00, fs wrote: > Why do you do this? > > <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif" > font-size="18"> > > <g fill="currentColor" color="lime"> > > <rect x="120" y="60" width="150" height="150" color="red"/> > > </g> > > </g> About the second case, the 'color="red"' was applied on style.color(). So before set color, i checked whether has currentColor or not. If have currentColor about 'fill' (and 'stroke'), don't was apply. https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:97: if (!color.alpha()) On 2015/11/25 17:23:00, fs wrote: > I don't see why you need this. And If don't has color(the color wasn't applied), just return svgStyle.{fill or stroke}. but has this one, return style.color().
https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:174: if (isFillCurrentColor || isStrokeCurrentColor) On 2015/11/26 at 01:07:26, hyunjunekim2 wrote: > On 2015/11/25 17:23:00, fs wrote: > > Why do you do this? > > > > <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif" > > font-size="18"> > > > <g fill="currentColor" color="lime"> > > > <rect x="120" y="60" width="150" height="150" color="red"/> > > > </g> > > > </g> > > About the second case, the 'color="red"' was applied on style.color(). > So before set color, i checked whether has currentColor or not. If have > currentColor about 'fill' (and 'stroke'), don't was apply. I think I understand what you mean, but I think the (current) behavior here is wrong per my reading of https://drafts.csswg.org/css-color/#currentcolor-color . It's possibly a different bug than the one about animation, so it could make sense to make that change in semantics first. https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp (right): https://codereview.chromium.org/1455153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:97: if (!color.alpha()) On 2015/11/26 at 01:07:26, hyunjunekim2 wrote: > On 2015/11/25 17:23:00, fs wrote: > > I don't see why you need this. > > And If don't has color(the color wasn't applied), just return svgStyle.{fill or stroke}. > but has this one, return style.color(). I suspect this is just a side-effect of the change in applyValueCSSPropertyColor. Regardless though this only checks if the alpha-channel of the color is zero - not whether it was applied or not (that's just a lucky coincidence at best.)
fs, I uploaded new patch. and tomorrow i will make tests.
https://codereview.chromium.org/1455153003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/1455153003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:179: state.style()->setColor(state.style()->svgStyle().fillPaintColor()); You shouldn't be overriding the value of 'color' like this. (This will break for instance inheritance of the 'color' property.)
I am working in progress to make a test.
fs, I made a test, but i guess it's not good. Could you give me a advice to make test? How do you think to make it as repaint test?
On 2015/12/06 at 13:13:15, hyunjune.kim wrote: > fs, I made a test, but i guess it's not good. Could you give me a advice to make test? > How do you think to make it as repaint test? According to https://code.google.com/p/chromium/issues/detail?id=537807#c2 the issue does not manifest itself through computed style, so another mechanism has to be used to "observe" the issue. It's possible that a repaint test could be utilized, but it will be difficult to ensure that the animation proceeds as expected then. I think it would be preferable to try to use an animation and pause it halfway (or somewhere between the end-points), and then compare that to a reference. It'd also be good if you actually added a description of what you think you are fixing and how.
Description was changed from ========== WIP BUG=537807 ========== to ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. BUG=537807 ==========
fs, Could you check this test(color-fill-currentColor-and-css.html)? It's possible to check repaint through color-fill-currentColor-and-css-expected.txt.
https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html (right): https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:6: var rootSVGElement; You don't need this. https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:8: function svgLoaded() { ...and hence not this. https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:18: cirlce = rootSVGElement.ownerDocument.getElementsByTagName("circle")[0]; 'rootSVGElement.ownerDocument' could just be written as 'document'. I also don't see any reason to put cirlce and bound on the global object (use var to make them local). https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:22: eventSender.mouseMoveTo(bound.left + 40, bound.top + 40); Do we really need 'hover' to trigger the bug, or could we just do: circle.style.color = 'cyan'; or circle.classList.add('someclass'); or similar instead to trigger the transition (or animation)? https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:25: setTimeout(runTimer, 2000); s/runTimer/finishRepaintTest/ - but I suspect this will be flaky, so you should really seek a method that does not require these kind of mechanism. Maybe you could use Element.animate(...) instead and use currentTime (and maybe pause())? https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:39: <body onload="runRepaintAndPixelTest()"> You can do: onload = runRepaintAndPixelTest; within the <script> instead. Then you can remove the <body>. https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:40: <svg id="svg" xmlns="http://www.w3.org/2000/svg" width=90 height=90 onload="svgLoaded()"> Per above you no longer need 'id' or 'onload' - 'xmlns' is not needed and the default dimension ought to be fine. so this could just be: <svg> https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:41: <circle id="circle" r="40" cx="40" cy="40" /> The 'id' isn't used so that can be dropped. I also don't think this needs to be a circle, so I'd suggest a simple 100x100 <rect> instead. https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/1455153003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:176: if (value->isPrimitiveValue() && (isFillCurrentColorByParent || isStrokeCurrentColorByParent)) { I still don't think that this is something that you should be doing.
fs, Could you check test? and i am going to check StyleBuilderCustom.cpp. Thank you.
https://codereview.chromium.org/1455153003/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html (right): https://codereview.chromium.org/1455153003/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:16: setInterval(interval, 1); Use requestAnimationFrame instead - or maybe runAfterLayoutAndPaint. Or maybe setTimeout, update the animation there and then wait for a layout+paint. Something like: var ani = circle.animate(...); setTimeout(function() { ani.currentTime = ...; ani.pause(); runAfterLayoutAndPaint(finishRepaintTest); }, 0); https://codereview.chromium.org/1455153003/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/repaint/color-fill-currentColor-and-css.html:25: <body onload="runRepaintAndPixelTest()"> I would prefer a ref-test if we really need to look at what's painted (doesn't seem entirely unreasonable). For a repaint test hopefully runRepaintTest here is enough.
fs, I worked test on ps13. Thank you. And about StyleBuilderCustom.cpp, why not inherit color?
fs, I checked it. this code has a bug.
for example,
<svg version="1.1" baseProfile="tiny" id="svg-root"
width="100%" height="100%" viewBox="0 0 480 360"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif"
font-size="18">
<g fill="currentColor" color="blue">
<rect id="rect" x="120" y="60" width="150" height="150"
fill="currentColor" color="red"/>
</g>
</g>
</svg>
// expected red rect.
Currently, if this code ran, paint blue rect because of this patch.
On 2015/12/16 at 15:03:41, hyunjune.kim wrote: > fs, I worked test on ps13. Thank you. Test looks ok. Could you try making a ref-test version too? I'm still a bit puzzled over whether this is a repaint issue only or a style-inheritance issue - or both. > And about StyleBuilderCustom.cpp, why not inherit color? I think the question you should ask is: "Why magically inherit 'color' if some other random properties have some random value?" My guess would be that you're trying to fix something - maybe counteracting that 'currentcolor' now has the proper used-value semantics (per the change in requestPaint; resolving the value of 'color' "later")? If that's the case, then it really would make sense to split out that into its own CL an only have the repaint fix here. It'd be good to have information like that in the description: "what", "why" and "how" are good guidelines for what to put in the description.
On 2015/12/16 at 15:24:03, hyunjune.kim wrote: > fs, I checked it. this code has a bug. > for example, > <svg version="1.1" baseProfile="tiny" id="svg-root" > width="100%" height="100%" viewBox="0 0 480 360" > xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> > <g id="test-body-content" font-family="SVGFreeSansASCII,sans-serif" font-size="18"> > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> > </g> > </g> > </svg> > > // expected red rect. > > Currently, if this code ran, paint blue rect because of this patch. Yes, that's the kind of effect the change in StyleBuilderCustom.cpp can have.
fs, Hi, I uploaded just version which is ps15.
> I'm still a bit puzzled over whether this is a repaint issue only or a
style-inheritance issue -
or both.
After go home and make document about this issue, i will send this to your
email.
>Summary
I guess this issue is repaint + inherit.
For fixing up repaint,
i changed a file called 'LayoutObject.cpp'.
For inherit.
we need to check flow inherited color.
for example,
<g fill="currentColor" color="blue">
<rect id="rect" x="120" y="60" width="150" height="150" color="red"/>
</g>
Currently on the blink,
g = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color {blue} }
rect = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color
{blue} }
After inherit,
On StyleBuilderCustom.cpp, changing color.
rect = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color {red}
}
but, i think it's a bug, because not need fill_style about
SVG_PAINTTYPE_CURRENTCOLOR.
Rect's color is not current color. So on the ps 15, i changed fill_style,
if parent is currentColor on {fill, strok},
made it like this
rect = { fill_style {SVG_PAINTTYPE_RGBCOLOR}, fill_color {blue}, color {red} }
For detail understanding, i will send e-mail.
Thank you.
On 2015/12/17 at 08:03:14, hyunjune.kim wrote:
> fs, Hi, I uploaded just version which is ps15.
> > I'm still a bit puzzled over whether this is a repaint issue only or a
style-inheritance issue -
> or both.
>
> After go home and make document about this issue, i will send this to your
email.
I'm really more interested in this information being available in the CL
description than in a document. So please don't waste any time writing a
document (hopefully you haven't already), but rather try to include the
information in the CL description.
>
> >Summary
> I guess this issue is repaint + inherit.
Ok, then we should probably make sure that we have tests that verify both of
those. I do think that we could land the inheritance change first too if we
wanted to.
> For fixing up repaint,
> i changed a file called 'LayoutObject.cpp'.
Ok, that change seems reasonable.
> For inherit.
> we need to check flow inherited color.
> for example,
> <g fill="currentColor" color="blue">
> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/>
> </g>
>
> Currently on the blink,
> g = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color {blue}
}
> rect = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color
{blue} }
>
> After inherit,
> On StyleBuilderCustom.cpp, changing color.
> rect = { fill_style {SVG_PAINTTYPE_CURRENTCOLOR}, fill_color {blue}, color
{red} }
I'm not sure what you mean with "After inherit" here - the specified value of
'color' on 'rect' is 'red', so it would be 'red' even before inheriting (and
'fill' would be the default) - I'm not sure it's useful to reason about it this
way though.
> but, i think it's a bug, because not need fill_style about
SVG_PAINTTYPE_CURRENTCOLOR.
This is actually what
https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor says:
"The keyword currentcolor takes its value from the value of the color property
on the same element. This happens at used-value time, which means that if the
value is inherited, it’s inherited as currentcolor, not as the value of the
color property, so descendants will use their own color property to resolve it."
> Rect's color is not current color. So on the ps 15, i changed fill_style,
> if parent is currentColor on {fill, strok},
> made it like this
> rect = { fill_style {SVG_PAINTTYPE_RGBCOLOR}, fill_color {blue}, color {red} }
This would be the "used value" (per above) - except fill_color would be 'red' -
this is the bit implemented by the style.visitedDependentColor(CSSPropertyColor)
change in requestPaint. So, the "computed value" would still be 'currentcolor'
(and then the fill_color can really be seen as "not defined".) Compare to for
instance https://jsfiddle.net/u1rcj8gf/.
> For detail understanding, i will send e-mail.
I'd prefer if we keep the discussion here.
On 2015/12/17 11:18:15, fs wrote: > On 2015/12/17 at 08:03:14, hyunjune.kim wrote: > > fs, Hi, I uploaded just version which is ps15. > > > I'm still a bit puzzled over whether this is a repaint issue only or a > style-inheritance issue - > > or both. > > > > After go home and make document about this issue, i will send this to your > email. > > I'm really more interested in this information being available in the CL > description than in a document. So please don't waste any time writing a > document (hopefully you haven't already), but rather try to include the > information in the CL description. Ok, i'm going to write information in the CL description.
fs, <g fill="currentColor" color="blue"> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> </g> Currently g element's computedStyle is copied to rect. also Fill type of rect's computedStyle become 'SVG_PAINTTYPE_CURRENTCOLOR'. <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> Looks at this. it's not CURRENTCOLOR type, just we need to inherit g's fills color. but currently on the blink, color as well as type(SVG_PAINTTYPE_CURRENTCOLOR) also inherits. So on StyleBuilderCustom.cpp, i tried replacing SVG_PAINTTYPE_CURRENTCOLOR with SVG_PAINTTYPE_RGBCOLOR.
Because of color type(SVG_PAINTTYPE_CURRENTCOLOR), So this confusion comes from LayoutSVGResourcePaintServer.cpp.
On 2015/12/17 at 12:26:54, hyunjune.kim wrote: > fs, > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > </g> > > Currently g element's computedStyle is copied to rect. Only the 'fill property should be "copied" (inherited). I don't think that 'color' for instance is "copied". (You can compare to the case where you specify fill=currentcolor on the rect instead.) > also Fill type of rect's computedStyle become 'SVG_PAINTTYPE_CURRENTCOLOR'. Yes, that's what we want. > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > Looks at this. it's not CURRENTCOLOR type, just we need to inherit g's fills color. Yes, it is, since 'fill' is an inherited property (https://svgwg.org/svg2-draft/painting.html#SpecifyingFillPaint, ditto 'stroke'), and because we want to resolve it when it's used we need to keep it around when inheriting. > but currently on the blink, color as well as type(SVG_PAINTTYPE_CURRENTCOLOR) also inherits. Yes. The inheritance of the color is what we want to avoid (or rather the use of it, because of how the compute style is stored at the moment, we will need to store something in the fill color field - even we will not actually use it.) > So on StyleBuilderCustom.cpp, i tried replacing SVG_PAINTTYPE_CURRENTCOLOR with SVG_PAINTTYPE_RGBCOLOR. I don't think you should be doing that. (With our current handling this is essentially what we're doing already because of the fill color being stored - and used.)
Description was changed from ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. BUG=537807 ========== to ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. Also, on the inherit has a problem. When fix up repaint and return color, occur to other issues, For example, <g fill="currentColor" color="blue"> <rect id="rect" x="0" y="0" width="2" height="2" color="red"/> </g> Rect elment is inherited color as well as fill type, So rect is filled by 'red', It should prevent the type inheritance. BUG=537807 ==========
fs, If inherit fillPaintType, <g fill="currentColor" color="blue"> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> </g> and <g fill="currentColor" color="blue"> <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> </g> It will be difficult to separate the two on LayoutSVGResourcePaintServer.cpp. If on the first case, if don't inherit fillPaintType, paint blue. but inherit type, on the this patch paint red. So I think that, just only inherit fill color except fillPaintType on first case, flow this way, case SVG_PAINTTYPE_RGBCOLOR: case SVG_PAINTTYPE_URI_RGBCOLOR: color = applyToFill ? svgStyle.fillPaintColor() : svgStyle.strokePaintColor(); hasColor = true; break; but on second case, because of setting new value on fillPaintType, flow this way, case SVG_PAINTTYPE_CURRENTCOLOR: case SVG_PAINTTYPE_URI_CURRENTCOLOR: color = style.visitedDependentColor(CSSPropertyColor); hasColor = true; break; If it's wrong, Could you give me a few advice for solving this issue? Thank you.
On 2015/12/17 at 13:09:03, hyunjune.kim wrote: > fs, If inherit fillPaintType, > > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > </g> > > and > > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> > </g> > > It will be difficult to separate the two on LayoutSVGResourcePaintServer.cpp. There should be no need to because they are (should be) the same. > If on the first case, if don't inherit fillPaintType, paint blue. > but inherit type, on the this patch paint red. Not if visitedDependentColor(CSSPropertyColor) is used. > So I think that, just only inherit fill color except fillPaintType on first case, > flow this way, > case SVG_PAINTTYPE_RGBCOLOR: > case SVG_PAINTTYPE_URI_RGBCOLOR: > color = applyToFill ? svgStyle.fillPaintColor() : svgStyle.strokePaintColor(); > hasColor = true; > break; > > but on second case, because of setting new value on fillPaintType, > > flow this way, > case SVG_PAINTTYPE_CURRENTCOLOR: > case SVG_PAINTTYPE_URI_CURRENTCOLOR: > color = style.visitedDependentColor(CSSPropertyColor); > hasColor = true; > break; > > If it's wrong, Could you give me a few advice for solving this issue? > Thank you. No, that's pretty much exactly what I'd expect. Funny business in application of 'color' is not what I'd expect though.
fs,
<g fill="currentColor" color="blue">
<rect id="rect" x="120" y="60" width="150" height="150" color="red"/>
</g>
On the this case, then Is expected color red?
<g fill="currentColor" color="blue">
<rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor"
color="red"/>
</g>
And also Is it red?
>> <g fill="currentColor" color="blue"> >> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> >> </g> >> >> and >> >> <g fill="currentColor" color="blue"> >> <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> >> </g> >> >> It will be difficult to separate the two on LayoutSVGResourcePaintServer.cpp. >There should be no need to because they are (should be) the same. I guess it's not the same.(https://jsfiddle.net/u1rcj8gf/2/)
On 2015/12/17 13:39:57, hyunjunekim2 wrote: > >> <g fill="currentColor" color="blue"> > >> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > >> </g> > >> > >> and > >> > >> <g fill="currentColor" color="blue"> > >> <rect id="rect" x="120" y="60" width="150" height="150" > fill="currentColor" > color="red"/> > >> </g> > >> > >> It will be difficult to separate the two on LayoutSVGResourcePaintServer.cpp. > > >There should be no need to because they are (should be) the same. > > I guess it's not the same.(https://jsfiddle.net/u1rcj8gf/2/) https://jsfiddle.net/u1rcj8gf/3/
On 2015/12/17 at 13:34:51, hyunjune.kim wrote: > fs, > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > </g> > > On the this case, then Is expected color red? Yes. > <g fill="currentColor" color="blue"> > <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" > color="red"/> > </g> > > And also Is it red? Yes.
On 2015/12/17 at 13:40:45, hyunjune.kim wrote: > On 2015/12/17 13:39:57, hyunjunekim2 wrote: > > >> <g fill="currentColor" color="blue"> > > >> <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > > >> </g> > > >> > > >> and > > >> > > >> <g fill="currentColor" color="blue"> > > >> <rect id="rect" x="120" y="60" width="150" height="150" > > fill="currentColor" > > color="red"/> > > >> </g> > > >> > > >> It will be difficult to separate the two on LayoutSVGResourcePaintServer.cpp. > > > > >There should be no need to because they are (should be) the same. > > > > I guess it's not the same.(https://jsfiddle.net/u1rcj8gf/2/) > > https://jsfiddle.net/u1rcj8gf/3/ In this case (the fiddle), background-color does not inherit, so when "background-color: inherit" is removed it will get the initial value instead.
On 2015/12/17 13:45:23, fs wrote: > On 2015/12/17 at 13:34:51, hyunjune.kim wrote: > > fs, > > <g fill="currentColor" color="blue"> > > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > > </g> > > > > On the this case, then Is expected color red? > > Yes. > > > <g fill="currentColor" color="blue"> > > <rect id="rect" x="120" y="60" width="150" height="150" > fill="currentColor" > > color="red"/> > > </g> > > > > And also Is it red? > > Yes. On the firefox 41, > > <g fill="currentColor" color="blue"> > > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > > </g> > > > > On the this case, then Is expected color red? Actual color is Blue. > > <g fill="currentColor" color="blue"> > > <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> > > </g> Actual color is Red. fs, Could you check test-result on LayoutTests/svg/W3C-SVG-1.1-SE/color-prop-05-t.svg?
On 2015/12/17 at 14:12:37, hyunjune.kim wrote: > On 2015/12/17 13:45:23, fs wrote: > > On 2015/12/17 at 13:34:51, hyunjune.kim wrote: > > > fs, > > > <g fill="currentColor" color="blue"> > > > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > > > </g> > > > > > > On the this case, then Is expected color red? > > > > Yes. > > > > > <g fill="currentColor" color="blue"> > > > <rect id="rect" x="120" y="60" width="150" height="150" > > fill="currentColor" > > > color="red"/> > > > </g> > > > > > > And also Is it red? > > > > Yes. > > On the firefox 41, > > > <g fill="currentColor" color="blue"> > > > <rect id="rect" x="120" y="60" width="150" height="150" color="red"/> > > > </g> > > > > > > On the this case, then Is expected color red? > > Actual color is Blue. > > > > <g fill="currentColor" color="blue"> > > > <rect id="rect" x="120" y="60" width="150" height="150" fill="currentColor" color="red"/> > > > </g> > > Actual color is Red. Yes, Gecko (Firefox) seems to still implement the old behavior. Blink however implements the new behavior almost universally AFAICS - SVG properties excluded. > fs, Could you check test-result on LayoutTests/svg/W3C-SVG-1.1-SE/color-prop-05-t.svg? Yes, that test tests the old behavior, so it'll now fail (and is likely a good reason to split the behavioral change to a CL of its own.)
Description was changed from ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. Also, on the inherit has a problem. When fix up repaint and return color, occur to other issues, For example, <g fill="currentColor" color="blue"> <rect id="rect" x="0" y="0" width="2" height="2" color="red"/> </g> Rect elment is inherited color as well as fill type, So rect is filled by 'red', It should prevent the type inheritance. BUG=537807 ========== to ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. Also, on the inherit has a problem. When fix up repaint and return color, occur to other issues, BUG=537807 ==========
Description was changed from ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. Also, on the inherit has a problem. When fix up repaint and return color, occur to other issues, BUG=537807 ========== to ========== Fixup currentColor for SVG color property(fill and stroke) doesn't animate to change color. Because when SVG Element has currentColor, don't work repaint(LayoutObject) and shouldn't return color on ComputedStyle. Also, on the inherit has a problem. BUG=537807 ==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Also, on the inherit has a problem.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property on the same element.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property on the same element.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification.
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property."
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification.
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property."
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property.".
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property.".
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property.".
#https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited
as currentcolor, not as the value of the color property.".
#https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
#https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
#https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except LayoutTest.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except this test.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
except this test.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
remove this test.
BUG=537807
==========
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",
remove this test.
BUG=537807
==========
to
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",remove
this test.
BUG=537807
==========
fs, Could you check PS 20? i removed color-prop-05-t files, and i will make new tests.
Description was changed from
==========
Fixup currentColor for SVG color property(fill and stroke)
doesn't animate to change color. Because when SVG Element
has currentColor, don't work repaint(LayoutObject) and
shouldn't return color on ComputedStyle.
Because if currentColor on {fill and stroke}, try to repaint.
And return value from the value of the color property
on the same element.
About inherit color, changed specification such as
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
-https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because there is Failure on "color-prop-05-t.svg",remove
this test.
BUG=537807
==========
to
==========
Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke'
When animating 'color', 'fill' or 'stroke' properties with the value
'currentColor' doesn't repaint properly, or with the right color.
For SVG elements which has 'currentColor' as part of their 'fill' or
'stroke' property, make sure they get repainted when 'color' changes.
This also updates the application of 'currentColor' to match the
later versions of the spec[1] which states that:
"If the value is inherited, it’s inherited as currentcolor,
not as the value of the color property.".
[1] https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor
Because of this, the test svg/W3C-SVG-1.1-SE/color-prop-05-t.svg
will appear as a fail (but is incorrect after the spec changed.)
BUG=537807
==========
On 2015/12/18 at 06:04:38, hyunjune.kim wrote: > fs, Could you check PS 20? i removed color-prop-05-t files, and i will make new tests. Code LG-TM now. I think we need at least one additional test for the new behavior of 'currentColor'. Also I don't think we should remove color-prop-05-t.svg, but rather just update the expected result to indicate that the behavior changed. As you may notice, I took the liberty of editing your description (and subject) a bit, so please read through that and see if I removed anything you'd like to keep or so (or undo it if you think I went too far - the old one should be available in a generated message.)
> I don't think we should remove color-prop-05-t.svg, but rather just update the expected result to indicate that > the behavior changed. fs, Now i am changing the behavior for color-prop-05-t.svg. But currently on the mac, occur to failure. Because not match image about this test. I want to change or get actual image based on mac. But i don't have mac computer. :( Could you give me a advice to change the behavior? Thank you.
On 2015/12/21 at 15:53:52, hyunjune.kim wrote: > > I don't think we should remove color-prop-05-t.svg, but rather just update the expected result to indicate that > > the behavior changed. > > fs, Now i am changing the behavior for color-prop-05-t.svg. But currently on the mac, occur to failure. > Because not match image about this test. I want to change or get actual image based on mac. But i don't have mac computer. :( > Could you give me a advice to change the behavior? Thank you. Add a line to TestExpectations like: crbug.com/537807 svg/W3C-SVG-1.1-SE/color-prop-05-t.svg [ NeedsRebaseline ] much preferable to split that change into a separate CL though so that we have the change in behavior (core/layout/svg/LayoutSVGResourcePaintServer.cpp change) separate from the bugfix. You should also add a new (passing) test for the new behavior.
>much preferable to split that change into a separate CL though so that we have the change in behavior (core/layout/svg/LayoutSVGResourcePaintServer.cpp change) separate from the bugfix. fs, How do i make new issues for fixing the hehavior which is 'color-prop-05-t.svg'? >You should also add a new (passing) test for the new behavior. I added a new test. Could you check PS24? Thank you.
On 2015/12/21 at 23:02:24, hyunjune.kim wrote: > >much preferable to split that change into a separate CL though so that we have > the change in behavior (core/layout/svg/LayoutSVGResourcePaintServer.cpp change) > separate from the bugfix. > > fs, How do i make new issues for fixing the hehavior which is 'color-prop-05-t.svg'? The easiest way would probably be to create new branch, cherry-pick this fix and remove the bits that are not related to the behavior change. Then upload that as a new CL. https://codereview.chromium.org/1455153003/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/css/currentColor-inheritance-about-fill-color.svg (right): https://codereview.chromium.org/1455153003/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/css/currentColor-inheritance-about-fill-color.svg:1: <svg width="800" height="400" viewBox="0 0 1200 600" This is an excellent basis for a ref-test. Just make another file with the same name, but -expected before the extension (...color-expected.svg), and then just make that paint two green <rect>s in the same positions as in the actual test file. Then you don't need the -expected.png and -expected.txt. Now for some test simplifications... I suggest you remove width/height/viewBox/xmlns:xlink here, and then adjust content per below. https://codereview.chromium.org/1455153003/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/css/currentColor-inheritance-about-fill-color.svg:5: <rect id="rect" x="0" y="0" width="150" height="150" color="green"/> Drop id/x/y and make width and height 100. https://codereview.chromium.org/1455153003/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/css/currentColor-inheritance-about-fill-color.svg:8: <rect id="rect" x="150" y="0" width="150" height="150" fill="currentColor" color="green"/> Make width/height/x 100 and drop id/y
fs, Could you check PS25? Thank you.
Re-based it.
On 2015/12/22 at 13:37:02, hyunjune.kim wrote: > fs, Could you check PS25? Thank you. Yes, this looks good to me - do you want to try splitting it into two pieces, or should be go ahead with this as is?
On 2015/12/22 13:53:34, fs wrote: > On 2015/12/22 at 13:37:02, hyunjune.kim wrote: > > fs, Could you check PS25? Thank you. > > Yes, this looks good to me - do you want to try splitting it into two pieces, or > should be go ahead with this as is? Ok, I will try splitting it which is a test named currentColor-inheritance-about-fill-color.svg. But first i guess this was landed on the blink.
fs, Do you say that try spliting currentColor-inheritance-about-fill-color.svg?
On 2015/12/22 at 14:10:05, hyunjune.kim wrote: > fs, Do you say that try spliting currentColor-inheritance-about-fill-color.svg? That test would be moved to the split out CL, yes (and the TestExpectations update.)
fs, Could you check PS27? and the split out CL is this one(https://codereview.chromium.org/1545673002/). Thank you.
Description was changed from ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. This also updates the application of 'currentColor' to match the later versions of the spec[1] which states that: "If the value is inherited, it’s inherited as currentcolor, not as the value of the color property.". [1] https://drafts.csswg.org/css-color-4/#valdef-color-currentcolor Because of this, the test svg/W3C-SVG-1.1-SE/color-prop-05-t.svg will appear as a fail (but is incorrect after the spec changed.) BUG=537807 ========== to ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. BUG=537807 ==========
fs, merged this one(https://codereview.chromium.org/1545673002/). Could you check PS28? Thank you.
LGTM w/ minor style nit https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right): https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/SVGComputedStyle.h:369: || visitedLinkFillPaintType() == SVG_PAINTTYPE_CURRENTCOLOR Indent the three ||-lines one more step (4 more spaces). https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/SVGComputedStyle.h:377: || visitedLinkStrokePaintType() == SVG_PAINTTYPE_CURRENTCOLOR Ditto.
On 2015/12/23 16:13:43, fs (OoO) wrote: > LGTM w/ minor style nit > > https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right): > > https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/style/SVGComputedStyle.h:369: || > visitedLinkFillPaintType() == SVG_PAINTTYPE_CURRENTCOLOR > Indent the three ||-lines one more step (4 more spaces). > > https://codereview.chromium.org/1455153003/diff/540001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/style/SVGComputedStyle.h:377: || > visitedLinkStrokePaintType() == SVG_PAINTTYPE_CURRENTCOLOR > Ditto. fs, I modified indent. And I am landing it.
The CQ bit was checked by hyunjune.kim@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1455153003/#ps560001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455153003/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455153003/560001
Message was sent while issue was closed.
Description was changed from ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. BUG=537807 ========== to ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. BUG=537807 ==========
Message was sent while issue was closed.
Committed patchset #29 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. BUG=537807 ========== to ========== Fix animation of 'color' w/ currentColor for SVG 'fill' and 'stroke' When animating 'color', 'fill' or 'stroke' properties with the value 'currentColor' doesn't repaint properly, or with the right color. For SVG elements which has 'currentColor' as part of their 'fill' or 'stroke' property, make sure they get repainted when 'color' changes. BUG=537807 Committed: https://crrev.com/ba6fc5130a3ca06c653ece942a19d06e64be187d Cr-Commit-Position: refs/heads/master@{#367286} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/ba6fc5130a3ca06c653ece942a19d06e64be187d Cr-Commit-Position: refs/heads/master@{#367286} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
