Closed Bug 1467621 Opened Last year Closed Last year

nsCSSShadowItem: Replace nsColor with StyleComplexColor

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

()

Details

Attachments

(4 files)

No description provided.
Assignee: nobody → dglastonbury
Blocks: 760345
Status: NEW → ASSIGNED
Priority: -- → P3
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.
Attachment #8985297 - Flags: review?(xidorn+moz) → review+
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:dglastonbury@mozilla.com" />
> +    <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:dglastonbury@mozilla.com" />
> +    <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.
Attachment #8985298 - Flags: review?(xidorn+moz) → review+
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.
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.
Flags: needinfo?(xidorn+moz)
Filed spec issue https://github.com/w3c/csswg-drafts/issues/2766
Flags: needinfo?(xidorn+moz)
Attachment #8985297 - Flags: review?(xidorn+moz)
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
Attachment #8986985 - Flags: review?(hikezoe) → review+
Comment on attachment 8985297 [details]
Bug 1467621 - P1: nsCSSShadowItem - Change nscolor to StyleComplexColor.

https://reviewboard.mozilla.org/r/250918/#review258674
Attachment #8985297 - Flags: review?(xidorn+moz) → review+
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
Attachment #8986984 - Flags: review?(xidorn+moz) → review+
Putting the spec issue in URL.
(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.
Pushed by dglastonbury@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.