Closed Bug 1380590 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1380590 - Update expectation
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
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
Priority: -- → P1
Priority: P1 → --
Priority: -- → P3
svg.context-properties.content.enabled is disabled by default, so this probably should be P2 as it can affect content.
Priority: P3 → P2
Summary: stylo: the fill color should be transparent when `svg.context-properties.content.enabled` and no fallback color given → stylo: the fill color should be transparent when `svg.context-properties.content.enabled=false` and no fallback color given
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
Attachment #8903511 - Flags: review?(xidorn+moz)
Attachment #8903512 - Flags: review?(xidorn+moz)
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
Attachment #8903512 - Flags: review?(xidorn+moz)
Comment on attachment 8903511 [details]
Bug 1380590 - Update expectation

https://reviewboard.mozilla.org/r/175344/#review180448
Attachment #8903511 - Flags: review?(xidorn+moz) → review+
(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 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.
Attachment #8903512 - Flags: review?(xidorn+moz)
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.
(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.
Attachment #8904195 - Attachment is obsolete: true
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.
Attachment #8903512 - Flags: review?(xidorn+moz)
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.
Attachment #8903512 - Flags: review?(xidorn+moz) → review+
(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.
Attachment #8903512 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9b75676259d8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: