1365926 - Support 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

Bug 1058040 didn't add support for 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties. There's now been a request for that in bug 1362408 comment 3.

Comment 1

[Andrew Overholt [:overholt]]

Is this likely to make it in time for 55 (soft code freeze June 5)? Just wondering if we should ask bug 1362408 to go a different route.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Support 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8872839 [details] [diff] [review]
part 1 - Support 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties

Review of attachment 8872839 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGContextPaint.cpp
@@ +395,5 @@
>      hash = HashGeneric(hash, mStroke->ToABGR());
>    }
>  
> +  if (mFillOpacity != 1.0f) {
> +    hash = HashGeneric(hash, mFillOpacity);

Why not just include the hash when it is 1.0f?

::: layout/svg/SVGContextPaint.h
@@ +308,5 @@
>  private:
>    Maybe<Color> mFill;
>    Maybe<Color> mStroke;
> +  float mFillOpacity;
> +  float mStrokeOpacity;

Do we not use Maybe<float> here because it's not possible to provide a fallback fill/stroke-opacity?

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Always allow the 'context-fill-opacity'/'context-stroke-opacity' keywords in SVG 'fill-opacity'/'stroke-opacity'

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Why not just include the hash when it is 1.0f?

Just because in the very common case that fill/stroke opacity are not part of the context paint it allows us to avoid the overhead of a couple of HashGeneric calls.

> ::: layout/svg/SVGContextPaint.h
> @@ +308,5 @@
> >  private:
> >    Maybe<Color> mFill;
> >    Maybe<Color> mStroke;
> > +  float mFillOpacity;
> > +  float mStrokeOpacity;
> 
> Do we not use Maybe<float> here because it's not possible to provide a
> fallback fill/stroke-opacity?

Exactly. And because on SVGContextPaint the GetFillOpacity and GetStrokeOpacity methods return a float (rather than a Maybe<float>). The behavior of these keywords and whether they should have a fallback is not really specified anywhere, but I think it would be sensible to have a fallback so I'd like to add that, but that's something for another bug.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71669172faf1
part 1 - Support 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6d6bbd7890
part 2 - Always allow the 'context-fill-opacity'/'context-stroke-opacity' keywords in SVG 'fill-opacity'/'stroke-opacity'. r=heycam

Comment 7

[Jonathan Watt [:jwatt]]

Leaving open for finishing the tests.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71669172faf1
https://hg.mozilla.org/mozilla-central/rev/5f6d6bbd7890

Comment 9

[Jonathan Watt [:jwatt]]

Actually I'm moving in the direction of replacing the context-* mechanism with a CSS variables based replacement, so I don't think it's worth finishing these tests just to remove them.