1467621 - nsCSSShadowItem: Replace nsColor with StyleComplexColor

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[u480271]

Attachment: Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

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

Comment 2

[u480271]

Attachment: Bug 1467621 - P2: Reftests for {box,text}-shadow with currentcolor.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8985297 [details]
Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

https://reviewboard.mozilla.org/r/250918/#review257216

::: servo/components/style/values/specified/effects.rs:148
(Diff revision 1)
>          }
>  
>          let lengths = lengths.ok_or(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))?;
>          Ok(BoxShadow {
>              base: SimpleShadow {
> -                color: color,
> +                color: color.unwrap_or_else(|| Color::currentcolor()),

`unwrap_or_else(Color::currentcolor)` should be enough.

::: servo/components/style/values/specified/effects.rs:256
(Diff revision 1)
> -        let color = input.try(|i| RGBAColor::parse(context, i)).ok();
> +        let color = input.try(|i| Color::parse(context, i)).ok();
>          let horizontal = Length::parse(context, input)?;
>          let vertical = Length::parse(context, input)?;
>          let blur = input.try(|i| Length::parse_non_negative(context, i)).ok();
> -        let color = color.or_else(|| input.try(|i| RGBAColor::parse(context, i)).ok());
> +        let color = color.or_else(|| input.try(|i| Color::parse(context, i)).ok());
> +        let color = color.unwrap_or_else(|| Color::currentcolor());

Ditto.

::: servo/components/style/values/specified/effects.rs:259
(Diff revision 1)
>              color: color,
>              horizontal: horizontal,
>              vertical: vertical,

You can do
```rust
let blur = blur.map(...);
```
then
```rust
color,
horizontal,
vertical,
blur,
```
if you like.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8985298 [details]
Bug 1467621 - P2: Reftests for {box,text}-shadow with currentcolor.

https://reviewboard.mozilla.org/r/250920/#review257222

::: layout/reftests/w3c-css/submitted/background/box-shadow-currentcolor-ref.html:13
(Diff revision 1)
> +    <meta charset="utf-8">
> +    <title>CSS Reftest Reference</title>
> +    <link rel="author" title="Dan Glastonbury" href="mailto:[e-mail]" />
> +    <style type="text/css">
> +     div {
> +         color: red;

So you are showing a red box shadow on the outer box? This feels unfortunate, because any red is generally considered failure.

Probably choose a different color like blue or yellow, or just transparent to let it disappear.

::: layout/reftests/w3c-css/submitted/background/box-shadow-currentcolor.html:13
(Diff revision 1)
> +    <meta charset="utf-8">
> +    <title>CSS Test: 'box-shadow' respects 'currentcolor'</title>
> +    <link rel="author" title="Dan Glastonbury" href="mailto:[e-mail]" />
> +    <link rel="help" href="https://www.w3.org/TR/css-bckgrounds-3/#the-box-shadow" />
> +    <link rel="help" href="https://www.w3.org/TR/css-color-3/#currentcolor" />
> +    <link rel="match" href="color-stop-currentcolor-ref.html" />

The reference is wrong.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8985298 [details]
Bug 1467621 - P2: Reftests for {box,text}-shadow with currentcolor.

https://reviewboard.mozilla.org/r/250920/#review257224

Also, consider updating test_shadow_transition in test_transitions_per_property.html to check the currentcolor interpolating behavior of shadows.

Comment 6

[u480271]

When discussing how to update test_transitions_per_property.html to capture the animation of shadows with current color a question was raised about the meaning of the following in the spec: (https://www.w3.org/TR/css-backgrounds-3/#the-box-shadow)

`Specifies the color of the shadow. If the color is absent, the used color is taken from the color property.`

:xidorn thinks it should mean to set the color to `currentcolor` and will raise an issue with the spec.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Filed spec issue https://github.com/w3c/csswg-drafts/issues/2766

Comment 8

[u480271]

Attachment: Bug 1467621 - P3: Add test for shadow-like color transitions on top of color transitions.

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

Comment 9

[u480271]

Attachment: Bug 1467621 - P4: Fix distance for drop-shadow with currentcolor.

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

Comment 10

[u480271]

Comment on attachment 8985297 [details]
Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

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

Comment 11

[u480271]

Comment on attachment 8985298 [details]
Bug 1467621 - P2: Reftests for {box,text}-shadow with currentcolor.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8986985 [details]
Bug 1467621 - P4: Fix distance for drop-shadow with currentcolor.

https://reviewboard.mozilla.org/r/252246/#review258672

Can you please also fix a typo in the distance calculation?  s/symmetic/symmetric/

https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/servo/components/style/values/animated/color.rs#189

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8985297 [details]
Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

https://reviewboard.mozilla.org/r/250918/#review258674

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8986984 [details]
Bug 1467621 - P3: Add test for shadow-like color transitions on top of color transitions.

https://reviewboard.mozilla.org/r/252244/#review258676

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Putting the spec issue in URL.

Comment 16

[u480271]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8986985 [details]
>
> Can you please also fix a typo in the distance calculation? 
> s/symmetic/symmetric/

Done.

Comment 17

[u480271]

Comment on attachment 8985297 [details]
Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

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

Comment 18

[u480271]

Comment on attachment 8985298 [details]
Bug 1467621 - P2: Reftests for {box,text}-shadow with currentcolor.

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

Comment 19

[u480271]

Comment on attachment 8986984 [details]
Bug 1467621 - P3: Add test for shadow-like color transitions on top of color transitions.

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

Comment 20

[u480271]

Comment on attachment 8986985 [details]
Bug 1467621 - P4: Fix distance for drop-shadow with currentcolor.

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

Comment 21

[u480271]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41dab05d9d47637f1d0cd97aa4b6f84a2ef413dd

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e91166471ea
P1: nsCSSShadowItem - Change nscolor to StyleComplexColor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2a33aac6c793
P2: Reftests for {box,text}-shadow with currentcolor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/118775b69afe
P3: Add test for shadow-like color transitions on top of color transitions. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/05fd2ee8852b
P4: Fix distance for drop-shadow with currentcolor. r=hiro

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2e91166471ea
https://hg.mozilla.org/mozilla-central/rev/2a33aac6c793
https://hg.mozilla.org/mozilla-central/rev/118775b69afe
https://hg.mozilla.org/mozilla-central/rev/05fd2ee8852b

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Tests merged to WPT in https://github.com/web-platform-tests/wpt/pull/11670 .