Comment on attachment 8989657 [details] Bug 1467622 - P2: Change SMIL paced tests to represent new distance calc. https://reviewboard.mozilla.org/r/254664/#review261518

Comment on attachment 8989656 [details] Bug 1467622 - P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor. https://reviewboard.mozilla.org/r/254662/#review261522 ::: layout/style/nsStyleStruct.h:2984 (Diff revision 1) > - void SetPaintServer(mozilla::css::URLValue* aPaintServer) { > + void SetPaintServer(mozilla::css::URLValue* aPaintServer); > - SetPaintServer(aPaintServer, eStyleSVGFallbackType_NotSet, > - NS_RGB(0, 0, 0)); > - } Is there any reason you need to de-inlinify this method and the second `SetContextValue` method below? It's not clear to me any. If there is nothing specific, I'd probably prefer keeping it inline here.

Comment on attachment 8989658 [details] Bug 1467622 - P3: Cleanup transition tests with currentcolor. https://reviewboard.mozilla.org/r/254666/#review261530

Comment on attachment 8989659 [details] Bug 1467622 - P4: Correct background/foreground ratio mixing. https://reviewboard.mozilla.org/r/254668/#review261542 I don't really like this "optimization". It feels error-prone. Could you elaborate on how that case was buggy and why this fixes that issue? I see no transparent in that testcase.

Comment on attachment 8989660 [details] Bug 1467622 - P5: Reftests for SVG pserver currentcolor override. https://reviewboard.mozilla.org/r/254670/#review261588 Looks good. (These probably would be even better as WPT tests, so other browsers can be tested against them.) ::: layout/reftests/svg/currentColor-override-pserver-stroke.svg:4 (Diff revision 1) > +<svg xmlns="http://www.w3.org/2000/svg"> > + <!-- pattern inherits fill color via currentcolor --> > + <defs> > + <pattern id ="MyPattern" patternUnits="userSpaceOnUse" Nit: no space after the `id` (and in the other files).

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive until July 7) from comment #14) > Comment on attachment 8989659 [details] > Bug 1467622 - P4: Optimise animated::Color for addition of currentcolor and > transparent. > > https://reviewboard.mozilla.org/r/254668/#review261542 > > I don't really like this "optimization". It feels error-prone. Could you > elaborate on how that case was buggy and why this fixes that issue? I see no > transparent in that testcase. This helped to fix failing test case from layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-curcol.svg. The testcase is animating from indigo `by` currentcolor, ie. to indigo + currentcolor, by different amounts. In a majority of the cases the blue component was +1 what was expected. I'll choose just one failing example to illustrate; <animate> that results in interpolating by 0.375. Compose animation 0 t1 := RGBA { transparent } + Foreground => Complex(RGBA { transparent }, bg: 1.0, fg: 1.0) t2 := RGBA { transparent } lerp(0.375) t1 => Complex(RGBA { transparent }, bg: 1.0, fg: 0.375) col := RGBA { indigo }) + t2 => Complex(RGBA { indigo, alpha: 0.5 }, bg: 2.0, fg: 0.375) When CalcColor is called with currentcolor, #AAF573, the result is #8B5CAE instead of expected. #8B5CAD temporary color, t1, results from the <animate by="currentcolor"> property. Because there's no explicit `from` attr, SMIL uses transparent. t2, is calculating the animated lerp of 37.5% from `indigo` to `indigo + currentcolor`. The "optimisation" was to recognize that adding transparent to a color should result in the same color, so to just skip the creation of a complex color in that situation. Compose animation 3 t1 := RGBA { transparent } + Foreground => Foreground t2 := RGBA { transparent }) lerp(0.375) t1 => Complex(RGBA { transparent }, bg: 0.625, fg: 0.375) col := RGBA { indigo } + t2 => Complex(RGBA { indigo, alpha: 0.61538464 }, bg: 1.625, fg: 0.375) When CalcColor is called this result in #8B5CAD, the expected value. Infact all the failing tests pass. The change fixes the issue by simplifying the calculations and we get "lucky" enough that all the failure cases work again. I chose this approach for lack of a good specification for the handling of alpha when adding colors. Relevant spec appear to be [1] and [2], "The values in the from/to/by and values attributes may specify negative and out of gamut values for colors. [...] However, as described in The animation sandwich model, the implementation should only correct the final combined result of all animations for a given attribute, and should not correct the effect of individual animations. Values are corrected by "clamping" the values to the correct range." To correctly, each step needs to be checked to determine why the desired result of: col := Complex(RGBA { indigo }, bg: 1, fg: 0.375) doesn't result. (Note: 2 * { indigo, a: 0.5 } = indigo and 1.625 * { indigo, a: .61538464 } ~= indigo) [1] https://drafts.csswg.org/css-transitions/#animtype-color [2] https://www.w3.org/TR/SMIL3/smil-animation.html#q46

(In reply to Cameron McCormack (:heycam) (busy until July 12) from comment #15) > Comment on attachment 8989660 [details] > Bug 1467622 - P5: Reftests for SVG pserver currentcolor override. > > https://reviewboard.mozilla.org/r/254670/#review261588 > > Looks good. (These probably would be even better as WPT tests, so other > browsers can be tested against them.) What's the best folder for these to go into for WPT? layout/reftests/w3c-css/submitted/...?

testing/web-platform/tests/svg/...

And probably specifically testing/web-platform/tests/svg/painting/ -- I guess that's the best matching chapter from SVG.

On inspections of the failing calculation, this is what is happening: The complex color is mColor=0x8082004B, mBgRatio=2, mFgRatio=0.375. p1 = 2, a1 = 128/255 = 0.5019607... b1 = a1 * b1 = 128/255 * 130 = 64.7539411... p2 = 0.375 a2 = 255/255 = 1. b1 = 115 a = clamp(p1 * a1 + p2 * a2) = clamp(2*128/255 + 0.375*255/255) = 1 b = clamp((p1 * b1 + p2 * b2) / a) = (2 * 130 * 128/255 + 0.375 * 115) / 1 = 173.635 Oh no, that rounds up to 174! (0xAE) Correct answer is: p1 = 1 a1 = 255/255 = 1 b1 = a1 * b1 = 255/255 * 129 = 129 p2 = 0.375 a2 = 255/255 = 1 b2 = a2 * b2 = 255/255 * 115 = 115 a = clamp(p1 * a1 + p2 * a2) = clamp(1 + 0.375) = 1 b = clamp((p1 * b1 + p2 * b2) / a) = (130 + 0.375 * 115) / 1 = 173.125 Yes, that rounds to 173 (0xAD) The issue is the alpha 0x80 is just a smidge too big when scaled back. It should be 0x7F. (Or aBgRatio should 1 not 2)

The round error happens because of the alpha losing its precision during the conversion to integer. So I think we should not do the optimization because the correctness needs further reasoning and it can be error-prone, and the underlying issue isn't really fixed here. (It cannot be fixed until we add more bits to alpha.) I would prefer we change the test to make it work properly in this case (or just change the reference with a comment saying the exact correct result).

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive until July 7) from comment #21) > The round error happens because of the alpha losing its precision during the > conversion to integer. > > So I think we should not do the optimization because the correctness needs > further reasoning and it can be error-prone, and the underlying issue isn't > really fixed here. (It cannot be fixed until we add more bits to alpha.) > > I would prefer we change the test to make it work properly in this case (or > just change the reference with a comment saying the exact correct result). I've worked through the equations for addition and lerping in this case and can see a better strategy, that I'm going to implement. On paper, these changes would result in aBgRatio not becoming 2 and alpha not becoming 0x80. I can walk through the details in Sydney, if you wish?

I can imagine how such strategy may work given this, and that should be better than the current special-casing I think. I'm fine if you do that I guess.

Comment on attachment 8989659 [details] Bug 1467622 - P4: Correct background/foreground ratio mixing. https://reviewboard.mozilla.org/r/254668/#review262750 ::: servo/components/style/values/animated/color.rs:104 (Diff revision 3) > + red: color.red * ratios.bg, > + green: color.green * ratios.bg, > + blue: color.blue * ratios.bg, This function is also used in compute distance, and I suspect that one probably shouldn't be changed this way... ::: servo/components/style/values/animated/color.rs:173 (Diff revision 3) > + // c1 op c2 > + // = (bg1 * mColor1 + fg1 * foreground) op (bg2 * mColor2 + fg2 * foreground) > + // = (bg1 * mColor1 op bg2 * mColor2) + (fg1 op fg2) * foreground Maybe worth commenting that it works because `c1 op c2` means `c1 * p + c2 * q`, and thus this equation. ::: servo/components/style/values/animated/color.rs:193 (Diff revision 3) > - // Then we compute the final background ratio, and derive > - // the final alpha value from the effective alpha value. > + > + // To calculate the final foreground color rations, perform > + // animation on effective ratios. > let self_ratios = self.effective_ratios(); > let other_ratios = other.effective_ratios(); > let ratios = self_ratios.animate(&other_ratios, procedure)?; If you are only animating `fg` maybe you can `self_ratios.fg.animate(&other_ratios.fg, procedure)?` instead, and that way `ComplexColorRatios` probably don't need `Animate` anymore.

Comment on attachment 8989659 [details] Bug 1467622 - P4: Correct background/foreground ratio mixing. https://reviewboard.mozilla.org/r/254668/#review262750 > This function is also used in compute distance, and I suspect that one probably shouldn't be changed this way... Good call. I've exacted the scaling of background color into a helper function that is local to animate(). > Maybe worth commenting that it works because `c1 op c2` means `c1 * p + c2 * q`, and thus this equation. I expanded the comment explaining the refactoring, including changing variable names to match the explanation. HTH.

Comment on attachment 8989659 [details] Bug 1467622 - P4: Correct background/foreground ratio mixing. https://reviewboard.mozilla.org/r/254668/#review263010

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90cede21bad1 P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor. r=xidorn https://hg.mozilla.org/integration/autoland/rev/56142038cc7f P2: Change SMIL paced tests to represent new distance calc. r=birtles https://hg.mozilla.org/integration/autoland/rev/e1dbee410e98 P3: Cleanup transition tests with currentcolor. r=xidorn https://hg.mozilla.org/integration/autoland/rev/74533cad9479 P4: Correct background/foreground ratio mixing. r=xidorn https://hg.mozilla.org/integration/autoland/rev/8694fe928b04 P5: Reftests for SVG pserver currentcolor override. r=heycam

Backed out 5 changesets (bug 1467622) or reftest failures on layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg. Backout: https://hg.mozilla.org/integration/autoland/rev/5a5c74e9ccc025350edeb6009e9071ee1f3e2891 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8694fe928b04200f132a4c45c77b9d3e6353bf93&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=187539823 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187539823&repo=autoland&lineNumber=7211 06:21:10 INFO - REFTEST TEST-START | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg == file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-ref.svg 06:21:10 INFO - REFTEST TEST-LOAD | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg | 19 / 102 (18%) 06:21:10 INFO - ++DOMWINDOW == 18 (000001BAA767C400) [pid = 7140] [serial = 48] [outer = 000001BAB6ACE200] 06:21:10 INFO - --DOMWINDOW == 17 (000001BAB6FDE800) [pid = 7140] [serial = 35] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E] 06:21:10 INFO - --DOMWINDOW == 16 (000001BAB6AB8400) [pid = 7140] [serial = 33] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E] 06:21:10 INFO - --DOMWINDOW == 15 (000001BAB6AB7400) [pid = 7140] [serial = 34] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-hex-hex.svg] 06:21:10 INFO - --DOMWINDOW == 14 (000001BAACE2F400) [pid = 7140] [serial = 32] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg] 06:21:10 INFO - --DOMWINDOW == 13 (000001BAACE2F800) [pid = 7140] [serial = 36] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-ident-hex.svg] 06:21:10 INFO - --DOMWINDOW == 12 (000001BAA7696800) [pid = 7140] [serial = 38] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-to-hex-hex.svg] 06:21:10 INFO - --DOMWINDOW == 11 (000001BAB6FE4000) [pid = 7140] [serial = 37] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E] 06:21:10 INFO - --DOMWINDOW == 10 (000001BAB6FDD800) [pid = 7140] [serial = 39] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E] 06:21:10 ERROR - REFTEST TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg == file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-ref.svg | image comparison, max difference: 1, number of differing pixels: 442

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11917 for changes under testing/web-platform/tests

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Upstream PR was closed without merging

Spoke to :xidorn and :heycam and the agreement was to mark the test as fuzzy. (The test is already marked fuzzy for skiaContent)

Comment on attachment 8991518 [details] Bug 1467622 - P6: Mark anim-css-fill reftest fuzzy on Windows. https://reviewboard.mozilla.org/r/256424/#review263322 ::: layout/reftests/svg/smil/style/reftest.list:34 (Diff revision 1) > fuzzy-if(skiaContent,1,550) == anim-css-fill-1-to-ident-hex.svg anim-css-fill-1-ref.svg > fuzzy-if(skiaContent,1,550) == anim-css-fill-1-to-ident-ident.svg anim-css-fill-1-ref.svg > > # 'fill' property, from/to/by, with 'currentColor' keyword > fuzzy-if(skiaContent,1,550) == anim-css-fill-1-by-ident-curcol.svg anim-css-fill-1-ref.svg > -fuzzy-if(skiaContent,1,550) == anim-css-fill-1-from-by-curcol-hex.svg anim-css-fill-1-ref.svg > +fuzzy-if(/^Windows\x20NT/.test(http.oscpu),1,442) fuzzy-if(skiaContent,1,550) == anim-css-fill-1-from-by-curcol-hex.svg anim-css-fill-1-ref.svg # Bug 1467622 Just use `winWidget` should be enough for this case. Adding comment pointing to this bug doesn't make much sense. Probably just remove it. I consider bug number in comment to mean "the failure type can be removed with bug XXX fixed", which it's not the case here. It is usually easy to find bug which adds the fuzzy via blame, so adding this comment doesn't add much value, and the bug adding it is also not very likely to be a useful information for fixing it in general.

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a85379bad09f P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor. r=xidorn https://hg.mozilla.org/integration/autoland/rev/04de06202c82 P2: Change SMIL paced tests to represent new distance calc. r=birtles https://hg.mozilla.org/integration/autoland/rev/9ce0bf688f66 P3: Cleanup transition tests with currentcolor. r=xidorn https://hg.mozilla.org/integration/autoland/rev/0b95ed91dcea P4: Correct background/foreground ratio mixing. r=xidorn https://hg.mozilla.org/integration/autoland/rev/787756851d80 P5: Reftests for SVG pserver currentcolor override. r=heycam https://hg.mozilla.org/integration/autoland/rev/7d7fe1b01693 P6: Mark anim-css-fill reftest fuzzy on Windows. r=xidorn

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

https://hg.mozilla.org/mozilla-central/rev/a85379bad09f https://hg.mozilla.org/mozilla-central/rev/04de06202c82 https://hg.mozilla.org/mozilla-central/rev/9ce0bf688f66 https://hg.mozilla.org/mozilla-central/rev/0b95ed91dcea https://hg.mozilla.org/mozilla-central/rev/787756851d80 https://hg.mozilla.org/mozilla-central/rev/7d7fe1b01693

Upstream PR merged