1380590 - stylo: the fill color should be transparent when `svg.context-properties.content.enabled=false` and no fallback color given

Status: RESOLVED FIXED

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

Description

[Chih-Hsuan Kuo [:tommykuo]]

I'm working on Bug 1377158 to enable stylo for SVG-as-an-image. I found the test case "context-fill-02.html"[1] would be failed if stylo applied. The color expected is transparent, but we got black.

[1]: http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/layout/reftests/svg/as-image/context-fill-02.html

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

svg.context-properties.content.enabled is disabled by default, so this probably should be P2 as it can affect content.

Comment 2

[Chih-Hsuan Kuo [:tommykuo]]

I found `style->mFill.GetFallbackType()` should be `eStyleSVGFallbackType_None` at [1], but we got `eStyleSVGFallbackType_NotSet` on stylo.


[1]: https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/svg/nsSVGUtils.cpp#1526

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1380590 - Update expectation

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

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

https://reviewboard.mozilla.org/r/175346/#review180444

So it seems to me that Gecko actually distinguish between cases like `context-value none` and `context-value`. With the former, `mFallbackType` would be `eStyleSVGFallbackType_None`, and with the latter, it would be `eStyleSVGFallbackType_NotSet`.

Have you done a try push and confirm that this wouldn't regress anything else?

The spec seems to distinguish between these two cases, too, although the result seems to be effectively identical (both `none` and nothing leads to a fallback which renders nothing). [1] It does affect computed value, though.

Ideally, we should probably change `SVGPaint` to take `Option<Option<ColorType>>` as fallback... Could you do that so that we match the spec's description as well as Gecko's impl?


[1] https://www.w3.org/TR/SVG2/painting.html#SpecifyingPaint

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8903511 [details]
Bug 1380590 - Update expectation

https://reviewboard.mozilla.org/r/175344/#review180448

Comment 7

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> Have you done a try push and confirm that this wouldn't regress anything
> else?

After the try finished, you're right. There are lots of regression with this patch. It seems to make lots of colors be transparent.

> The spec seems to distinguish between these two cases, too, although the
> result seems to be effectively identical (both `none` and nothing leads to a
> fallback which renders nothing). [1] It does affect computed value, though.
> 
> Ideally, we should probably change `SVGPaint` to take
> `Option<Option<ColorType>>` as fallback... Could you do that so that we
> match the spec's description as well as Gecko's impl?
> 
> 
> [1] https://www.w3.org/TR/SVG2/painting.html#SpecifyingPaint

I'll try your suggestion next week! Thx.

Comment 8

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Pushed via 'mach try fuzzy' with query: linux64stylodevtool

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

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

https://reviewboard.mozilla.org/r/175346/#review180980

::: servo/components/style/properties/gecko.mako.rs:746
(Diff revision 2)
>          if let Some(fallback) = fallback {
> +            if let Some(color) = fallback {
> -            paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_Color;
> +                paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_Color;
> -            paint.mFallbackColor = convert_rgba_to_nscolor(&fallback);
> +                paint.mFallbackColor = convert_rgba_to_nscolor(&color);
> +            } else {
> +                paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_None;
> +            }
> +        } else {
> +            paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet;
>          }

It may be shorter if you use match like
```rust
paint.mFallbackType = match fallback {
    Some(Some(color)) => {
        paint.mFallbackColor = convert_rgba_to_nscolor(&color);
        nsStyleSVGFallbackType::eStyleSVGFallbackType_Color
    }
    Some(None) => nsStyleSVGFallbackType::eStyleSVGFallbackType_None
    None => nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet
};
```

::: servo/components/style/values/generics/svg.rs:24
(Diff revision 2)
>      /// The fallback color
> -    pub fallback: Option<ColorType>,
> +    pub fallback: Option<Option<ColorType>>,

Please add comment here for the meaning of possible values.

::: servo/components/style/values/generics/svg.rs:68
(Diff revision 2)
>  /// https://svgwg.org/svg2-draft/painting.html#SpecifyingPaint
>  fn parse_fallback<'i, 't, ColorType: Parse>(context: &ParserContext,
>                                              input: &mut Parser<'i, 't>)
> -                                            -> Option<ColorType> {
> +                                            -> Option<Option<ColorType>> {
>      if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> -        None
> +        // eStyleSVGFallbackType_None

Please avoid mentioning Gecko constants in code which is not Gecko specific. Describing the meaning of each value in the comment of field should be enough.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8904195 [details]
Pushed via 'mach try fuzzy' with query: linux64stylodevtool

https://reviewboard.mozilla.org/r/175972/#review180982

Please do not include try fuzzy commit in mozreview push. I suppose 'mach try fuzzy' should remove this kind of commits automatically, so this could be a bug of that. You may want to report a bug for this.

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> Comment on attachment 8904195 [details]
> Pushed via 'mach try fuzzy' with query: linux64stylodevtool
> 
> https://reviewboard.mozilla.org/r/175972/#review180982
> 
> Please do not include try fuzzy commit in mozreview push. I suppose 'mach
> try fuzzy' should remove this kind of commits automatically, so this could
> be a bug of that. You may want to report a bug for this.

I think maybe that's caused by me... It is possibe that I used `git mozview push` when `mach try fuzzy` haven't finished.

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

https://reviewboard.mozilla.org/r/175346/#review181024

::: servo/components/style/values/generics/svg.rs:29
(Diff revision 3)
>      /// The fallback color
> -    pub fallback: Option<ColorType>,
> +    /// The possible value would be None, Some(None) or Some(Some(ColorType)).
> +    /// - None: empty
> +    /// - Some(None): the keyword `none`
> +    /// - Some(Some(ColorType)): color type
> +    pub fallback: Option<Option<ColorType>>,

Ok, so I just realized that it doesn't work, because the derived `ToCss` would generate nothing for `Some(None)` just like `None`.

Talked with :nox on IRC, the suggestion is to `Option<Either<ColorType, None_>>` here rather than a nested `Option`. This would allow the derived `ToCss` to serialize as expected.

Comment 15

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

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

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

https://reviewboard.mozilla.org/r/175346/#review181130

::: servo/components/style/properties/gecko.mako.rs:746
(Diff revision 4)
> -        if let Some(fallback) = fallback {
> -            paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_Color;
> -            paint.mFallbackColor = convert_rgba_to_nscolor(&fallback);
> +        paint.mFallbackType = match fallback {
> +            Some(fallback) => {
> +                match fallback {
> +                    Either::First(color) => {
> +                        paint.mFallbackColor = convert_rgba_to_nscolor(&color);
> +                        nsStyleSVGFallbackType::eStyleSVGFallbackType_Color
> +                    },
> +                    Either::Second(_) => {
> +                        nsStyleSVGFallbackType::eStyleSVGFallbackType_None
> +                    }
> -        }
> +                }

You can still use just one level of `match`, actually. It may not matter a lot, though.

Comment 17

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8903512 [details]
Bug 1380590 - Support None and NotSet for the fallback of SVGPaint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175346/diff/4-5/

Comment 18

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #16)
> You can still use just one level of `match`, actually. It may not matter a
> lot, though.

Updated, I also prefer the one level version. Thx.

Comment 19

[Chih-Hsuan Kuo [:tommykuo]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f036aadb4d116c3ac6b01070befd795fa1f29fb

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b75676259d8
Update expectation r=xidorn

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9b75676259d8