1387982 - stylo: The interpolation between drop-shadow functions without specified color is wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Chiou [:boris]]

There are two failures related to filter serialization:

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | Filter property is blur(37.5px) drop-shadow(rgba(0, 0, 0, 0) 6px 6px 0px) expected values of blur,37.5,drop-shadow,rgb(0, 0, 0) 6px 6px 0px

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | Filter property is blur(62.5px) drop-shadow(rgba(0, 0, 0, 0) 2px 2px 0px) expected values of blur,62.5,drop-shadow,rgb(0, 0, 0) 2px 2px 0px

Looks like the expected value is unitless?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I don't think bug 1369614 is related to this. I think gecko should serialize those unitless value with px for computed values. I thought mantaroh has been already filed a bug for it.

Apart from this bug, I guess that precision issue which is similar to bug 1367327 also happens on blur in drop-shadow.

Comment 2

[Jeremy Chen ([e-mail])]

We've already rejected unitless non-zero values for blur() during parsing time in Gecko. So, this is just the serialization issue for animation value, and I think making the serialization of an animation value to match that of an computed value is the right thing to do.

Hi Mantaroh, as comment 1 mentioned, have you already filed a bug for this? If you haven't started working on this, I can take a look.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> We've already rejected unitless non-zero values for blur() during parsing
> time in Gecko. So, this is just the serialization issue for animation value,
> and I think making the serialization of an animation value to match that of
> an computed value is the right thing to do.
> 
> Hi Mantaroh, as comment 1 mentioned, have you already filed a bug for this?
> If you haven't started working on this, I can take a look.

I filed the similar bug which shadow item will lost unit information, it is bug 1379908.
But I think that this bug is filter blur function, so it may be not same bug.

Comment 4

[Jeremy Chen ([e-mail])]

I think I know what's going wrong here. I'll upload a patch for review later today.

Comment 5

[Jeremy Chen ([e-mail])]

(In reply to Boris Chiou [:boris] from comment #0)
> There are two failures related to filter serialization:
> 
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | Filter property is blur(37.5px) drop-shadow(rgba(0, 0, 0, 0) 6px 6px 0px)
> expected values of blur,37.5,drop-shadow,rgb(0, 0, 0) 6px 6px 0px
> 
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | Filter property is blur(62.5px) drop-shadow(rgba(0, 0, 0, 0) 2px 2px 0px)
> expected values of blur,62.5,drop-shadow,rgb(0, 0, 0) 2px 2px 0px
> 
> Looks like the expected value is unitless?

I think the unitless is expected. The root cause of this test failure is that the color component of drop-shadow is not expected. Supposely, we should use currentColor instead if color component is not specified in drop-shadow, however, it seems that we use "transparent" in stylo....

Comment 6

[Brian Birtles (:birtles)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> I think the unitless is expected. The root cause of this test failure is
> that the color component of drop-shadow is not expected. Supposely, we
> should use currentColor instead if color component is not specified in
> drop-shadow, however, it seems that we use "transparent" in stylo....

Does bug 1388220 (currently on autoland) make any difference here?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> > I think the unitless is expected. The root cause of this test failure is
> > that the color component of drop-shadow is not expected. Supposely, we
> > should use currentColor instead if color component is not specified in
> > drop-shadow, however, it seems that we use "transparent" in stylo....
> 
> Does bug 1388220 (currently on autoland) make any difference here?

Yes, right. Now all of filter test cases work fine.

Comment 8

[Brian Birtles (:birtles)]

So I guess this bug just needs to update the following lines in test_transitions_per_property.html:

  http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/layout/style/test/test_transitions_per_property.html#970,974,1646-1649

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Bug 1387982 - re-enable filter tests in test_transitions_per_property.html.

The reason of the previous failure is we wrongly use default color for drop-shadow
functions that without specified color component. Since this has been resolved in
bug 1388220, we can enable these two tests now.

Review commit: https://reviewboard.mozilla.org/r/168960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168960/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897684 [details]
Bug 1387982 - re-enable filter tests in test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/168960/#review174308

Comment 11

[Brian Birtles (:birtles)]

Can we also drop the check here:

function test_filter_transition(prop) {
  if (!SpecialPowers.getBoolPref("layout.css.filters.enabled")) {
    return;
  }
  for (var i in filterTests) {
    var test = filterTests[i];
>    // Bug 1387982: remove this after we pass all filter tests.
>    if (!!test.skipped) {
>      continue;
>    }
    div.style.setProperty("transition-property", "none", "");
    div.style.setProperty(prop, test.start, "");
    cs.getPropertyValue(prop);
    div.style.setProperty("transition-property", prop, "");
    div.style.setProperty(prop, test.end, "");
    var actual = cs.getPropertyValue(prop);
    ok(filter_function_list_equals(actual, test.expected),
                                   "Filter property is " + actual + " expected values of " +
                                   test.expected);
  }
}

?

Comment 12

[Jeremy Chen ([e-mail])]

(In reply to Brian Birtles (:birtles) from comment #11)
> Can we also drop the check here:
> 
> function test_filter_transition(prop) {
>   if (!SpecialPowers.getBoolPref("layout.css.filters.enabled")) {
>     return;
>   }
>   for (var i in filterTests) {
>     var test = filterTests[i];
> >    // Bug 1387982: remove this after we pass all filter tests.
> >    if (!!test.skipped) {
> >      continue;
> >    }
>     div.style.setProperty("transition-property", "none", "");
>     div.style.setProperty(prop, test.start, "");
>     cs.getPropertyValue(prop);
>     div.style.setProperty("transition-property", prop, "");
>     div.style.setProperty(prop, test.end, "");
>     var actual = cs.getPropertyValue(prop);
>     ok(filter_function_list_equals(actual, test.expected),
>                                    "Filter property is " + actual + "
> expected values of " +
>                                    test.expected);
>   }
> }
> 
> ?

Yes, I should've dropped this as well. Thank you Brian. :)

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8897684 [details]
Bug 1387982 - re-enable filter tests in test_transitions_per_property.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168960/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4b427e9e8b5
re-enable filter tests in test_transitions_per_property.html. r=hiro

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4b427e9e8b5