stylo: currentColor breaks box-shadow interpolation

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: me, Assigned: hiro)

Tracking

(Blocks 1 bug, {nightly-community})

Trunk
mozilla57
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 disabled, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
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.
Depends on: 1387973
Confirmed in Nightly 57 x64 20170807113452 @ Debian Testing.
Someone will certainly have something like this on his website.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Version: 57 Branch → Trunk
(Assignee)

Updated

2 years ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
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
(Assignee)

Updated

2 years ago
No longer depends on: 1387973
(Assignee)

Comment 5

2 years ago
Do'h! I thought I did push patches for review.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1387973

Comment 9

2 years ago
mozreview-review
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.
Attachment #8897293 - Flags: review?(xidorn+moz) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8897294 [details]
Bug 1388220 - Convert gecko's currentcolor to servo one.

https://reviewboard.mozilla.org/r/168594/#review173826
Attachment #8897294 - Flags: review?(xidorn+moz) → review+

Comment 11

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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
Attachment #8897295 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 13

2 years ago
(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.
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
Posted file Servo PR
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8897293 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8897294 - Attachment is obsolete: true

Comment 17

2 years ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ad3d262ebbe
Simple interpolation test cases for text-shadow and box-shadow with currentcolor. r=xidorn

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ad3d262ebbe
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Status: RESOLVED → VERIFIED
OS: Unspecified → All
You need to log in before you can comment on or make changes to this bug.