1388220 - stylo: currentColor breaks box-shadow interpolation

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Chris Morgan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170805100334

Steps to reproduce:

With layout.css.servo.enabled true, I loaded this demo document:

<style>
    div {
        box-shadow: 0 2em currentColor;
        transition: 0.5s all;
    }
    div:hover {
        box-shadow: 0 3em currentColor;
    }
</style>
<div>foo</div>

… and hovered over the DIV.


Actual results:

The box-shadow disappeared for half a second, and then appeared in the final position.


Expected results:

The box-shadow should have smoothly transitioned from 2em to 3em, as it does if you avoid using the currentColor keyword or if you disable stylo.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Transition of box-shadow still has problem, and there is an open bug for that.

We can see if this is fixed after that bug gets fixed.

Comment 2

[Darkspirit]

Attachment: demo.html

from comment 0

Comment 3

[Darkspirit]

Confirmed in Nightly 57 x64 20170807113452 @ Debian Testing.
Someone will certainly have something like this on his website.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

The failure reason is that we don't get back currentcolor state[1] when we convert gecko's shadow value into servo's one for animation.

[1] https://hg.mozilla.org/mozilla-central/file/e928c65095ed/servo/components/style/gecko_bindings/sugar/ns_css_shadow_item.rs#l26

https://treeherder.mozilla.org/#/jobs?repo=try&revision=543d03ed5c966fe1870ab3601e2ec35535c21009

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Do'h! I thought I did push patches for review.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388220 - Reuse to_simple_shadow() for box-shadow as well.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388220 - Convert gecko's currentcolor to servo one.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388220 - Simple interpolation test cases for text-shadow and box-shadow with currentcolor.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8897293 [details]
Bug 1388220 - Reuse to_simple_shadow() for box-shadow as well.

https://reviewboard.mozilla.org/r/168592/#review173816

::: servo/components/style/gecko_bindings/sugar/ns_css_shadow_item.rs:53
(Diff revision 1)
>      pub fn to_simple_shadow(&self) -> SimpleShadow {
> -        debug_assert_eq!(self.mSpread, 0);
> -        debug_assert_eq!(self.mInset, false);
>          SimpleShadow {
>              color: Color::rgba(convert_nscolor_to_rgba(self.mColor)),
>              horizontal: Au(self.mXOffset),
>              vertical: Au(self.mYOffset),
>              blur: Au(self.mRadius).into(),
>          }
>      }

Actually I'd prefer you leave the assertions inside `to_simple_shadow` function, and move the actual SimpleShadow creating code into a private function, otherwise the name `to_simple_shadow` could be misleading.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8897294 [details]
Bug 1388220 - Convert gecko's currentcolor to servo one.

https://reviewboard.mozilla.org/r/168594/#review173826

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8897294 [details]
Bug 1388220 - Convert gecko's currentcolor to servo one.

https://reviewboard.mozilla.org/r/168594/#review173828

::: servo/components/style/gecko_bindings/sugar/ns_css_shadow_item.rs:64
(Diff revision 1)
> +
>      /// Returns this item as a simple shadow.
>      #[inline]
>      pub fn to_simple_shadow(&self) -> SimpleShadow {
>          SimpleShadow {
> -            color: Color::rgba(convert_nscolor_to_rgba(self.mColor)),
> +            color: self.extract_color(),

It isn't really necessary to have a separate function, but either way is fine.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8897295 [details]
Bug 1388220 - Simple interpolation test cases for text-shadow and box-shadow with currentcolor.

https://reviewboard.mozilla.org/r/168596/#review173830

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> Comment on attachment 8897293 [details]
> Bug 1388220 - Reuse to_simple_shadow() for box-shadow as well.
> 
> https://reviewboard.mozilla.org/r/168592/#review173816
> 
> ::: servo/components/style/gecko_bindings/sugar/ns_css_shadow_item.rs:53
> (Diff revision 1)
> >      pub fn to_simple_shadow(&self) -> SimpleShadow {
> > -        debug_assert_eq!(self.mSpread, 0);
> > -        debug_assert_eq!(self.mInset, false);
> >          SimpleShadow {
> >              color: Color::rgba(convert_nscolor_to_rgba(self.mColor)),
> >              horizontal: Au(self.mXOffset),
> >              vertical: Au(self.mYOffset),
> >              blur: Au(self.mRadius).into(),
> >          }
> >      }
> 
> Actually I'd prefer you leave the assertions inside `to_simple_shadow`
> function, and move the actual SimpleShadow creating code into a private
> function, otherwise the name `to_simple_shadow` could be misleading.

Oh right. Indeed. I will add the private function named... I need to think the name somehow.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> > Comment on attachment 8897293 [details]
> > Bug 1388220 - Reuse to_simple_shadow() for box-shadow as well.
> > 
> > https://reviewboard.mozilla.org/r/168592/#review173816
> > 
> > ::: servo/components/style/gecko_bindings/sugar/ns_css_shadow_item.rs:53
> > (Diff revision 1)
> > >      pub fn to_simple_shadow(&self) -> SimpleShadow {
> > > -        debug_assert_eq!(self.mSpread, 0);
> > > -        debug_assert_eq!(self.mInset, false);
> > >          SimpleShadow {
> > >              color: Color::rgba(convert_nscolor_to_rgba(self.mColor)),
> > >              horizontal: Au(self.mXOffset),
> > >              vertical: Au(self.mYOffset),
> > >              blur: Au(self.mRadius).into(),
> > >          }
> > >      }
> > 
> > Actually I'd prefer you leave the assertions inside `to_simple_shadow`
> > function, and move the actual SimpleShadow creating code into a private
> > function, otherwise the name `to_simple_shadow` could be misleading.
> 
> Oh right. Indeed. I will add the private function named... I need to think
> the name somehow.

I name it extract_simple_shadow.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897295 [details]
Bug 1388220 - Simple interpolation test cases for text-shadow and box-shadow with currentcolor.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ad3d262ebbe
Simple interpolation test cases for text-shadow and box-shadow with currentcolor. r=xidorn

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ad3d262ebbe

Comment 19

[Darkspirit]

Such smooth, much wow.
Nightly 57 x64 20170816100153 @ Debian Testing

56 beta = wontfix, I think. (not a blocker for experiment participants and not a crash fix)

I should go through all stylo-site-issues and should set status-firefox57 to verified if they are fine on WOW64/Win64/Linux64/Mac.