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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
6 months ago
16 days ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 affected)

Details

Attachments

(2 attachments)

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.
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.
Flags: needinfo?(jwatt)
(Assignee)

Comment 2

6 months ago
Created attachment 8872839 [details] [diff] [review]
part 1 - Support 'context-fill-opacity'/'context-stroke-opacity' as SVG-as-an-image context properties
Flags: needinfo?(jwatt)
Attachment #8872839 - Flags: review?(cam)
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?
Attachment #8872839 - Flags: review?(cam) → review+
(Assignee)

Comment 4

6 months ago
Created attachment 8873279 [details] [diff] [review]
part 2 - Always allow the 'context-fill-opacity'/'context-stroke-opacity' keywords in SVG 'fill-opacity'/'stroke-opacity'
Attachment #8873279 - Flags: review?(cam)
(Assignee)

Comment 5

6 months ago
(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.
Attachment #8873279 - Flags: review?(cam) → review+

Comment 6

6 months ago
Pushed by jwatt@jwatt.org:
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
(Assignee)

Comment 7

6 months ago
Leaving open for finishing the tests.
Keywords: leave-open

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71669172faf1
https://hg.mozilla.org/mozilla-central/rev/5f6d6bbd7890
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.
Status: NEW → RESOLVED
Last Resolved: 16 days ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.