1350010 - Require explicit opt-in from elements embedding SVG images before exposing context properties [-moz-context-properties]

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

In bug 1058040 I added support for passing through fill and stroke from a element embedding an SVG image into the SVG image itself. Currently this is only exposed to chrome. We've no immediate plans to expose this functionality on the Web, but before we could do that we would need to add protections to require elements embedding SVG images to explicitly opt-in to exposing properties to the SVG image.

My current thoughts on this are to add a new 'context-properties' property where the names of properties to be exposed have to be listed.

Comment 1

[Dão Gottwald [:dao]]

The syntax would be "context-properties: fill, fill-opacity, stroke;" for instance?

Comment 2

[Jonathan Watt [:jwatt]]

Yes, that's what I had in mind.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement the '-moz-context-properties' property

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement the '-moz-context-properties' property

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8862688 [details] [diff] [review]
part 1 - Implement the '-moz-context-properties' property

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

This is generally OK, but seems to be missing nsCSSParser.cpp changes?

::: layout/style/nsCSSPropList.h
@@ +1620,5 @@
> +    -moz-context-properties,
> +    _moz_context_properties,
> +    ContextProperties,
> +    CSS_PROPERTY_PARSE_FUNCTION |
> +        CSS_PROPERTY_VALUE_LIST_USES_COMMAS,

Please use CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME so that the property is not exposed to content.

::: layout/style/nsRuleNode.cpp
@@ +10204,5 @@
>  
> +  // -moz-context-properties:
> +  const nsCSSValue* contextPropsValue = aRuleData->ValueForContextProperties();
> +  switch (contextPropsValue->GetUnit()) {
> +  case eCSSUnit_Null:

Nit: case statements should be indented a level.

::: layout/style/nsStyleConsts.h
@@ +1054,5 @@
> +// See nsStyleSVGReset
> +
> +// -moz-context-properties
> +#define NS_STYLE_CONTEXT_PROPERTY_FILL          1
> +#define NS_STYLE_CONTEXT_PROPERTY_STROKE        2

Since these are used as bits, I recommend writing them as 0x1 and 0x2 or (1 << 0) and (1 << 1) to make that clear.

::: layout/style/nsStyleStruct.cpp
@@ +1142,5 @@
>    , mFloodOpacity(1.0f)
>    , mDominantBaseline(NS_STYLE_DOMINANT_BASELINE_AUTO)
>    , mVectorEffect(NS_STYLE_VECTOR_EFFECT_NONE)
>    , mMaskType(NS_STYLE_MASK_TYPE_LUMINANCE)
> +  , mContextProps(0)

Should this be mContextPropBits instead?  (The default constructor for the mContextProps nsCOMArray should be fine.)

@@ +1163,5 @@
>    , mFloodOpacity(aSource.mFloodOpacity)
>    , mDominantBaseline(aSource.mDominantBaseline)
>    , mVectorEffect(aSource.mVectorEffect)
>    , mMaskType(aSource.mMaskType)
> +  , mContextProps(aSource.mContextProps)

And here we should be copying mContextPropBits too.

@@ +1216,5 @@
>               mLightingColor != aNewData.mLightingColor ||
>               mStopOpacity   != aNewData.mStopOpacity   ||
>               mFloodOpacity  != aNewData.mFloodOpacity  ||
> +             mMaskType      != aNewData.mMaskType      ||
> +             mContextPropsBits != aNewData.mContextPropsBits) {

You need to compare mContextProps and generate nsChangeHint_NeutralChange if it is different.  You can do that at the end of the function (and, if you want, you can do it only if hint doesn't have anything in it).

::: layout/style/test/property_database.js
@@ +1650,5 @@
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "fill", "stroke" ],
> +    invalid_values: [ "auto", "all", "default", "clipPath", "mask" ]

These aren't invalid values, right?  You should include some other valid values that are lists.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement the '-moz-context-properties' property

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8862691 [details] [diff] [review]
part 1 - Implement the '-moz-context-properties' property

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

r=me with these things addressed.

I'd love for this to be restricted further than just "it has no effect when the referenced image is not chrome://", I can't think of any easy way to do that given the various places (about:reader, video control bindings, ...) that you want to use this.  You could use CSS_PROPERTY_INTERNAL, which would hide the property from getComputedStyle, but it still would be able to be set in style sheets, and @supports / CSS.supports() would still recognise it.

::: layout/style/nsCSSParser.cpp
@@ +8064,5 @@
>    SkipUntil(')');
>    return false;
>  }
>  
> +bool CSSParserImpl::ParseContextProperties()

Nit: newline after "bool".

@@ +8080,5 @@
> +    }
> +
> +    if (value.GetUnit() == eCSSUnit_None)
> +    {
> +      return false;

Hmm, do we really want to return false if we find "none"?  That should be a valid value.  I think this block should be dropped.  (For will-change, "none" is explicitly an invalid value.)

@@ +8085,5 @@
> +    }
> +
> +    if (value.GetUnit() != eCSSUnit_Ident) {
> +      if (first) {
> +        AppendValue(eCSSProperty__moz_context_properties, value);

(Here is where if we parse "none" we'll return true, and only if it was the first value we got.)

::: layout/style/test/property_database.js
@@ +1649,5 @@
> +    domProp: "MozContextProperties",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "fill", "stroke", "fill stroke" ],

I think it needs to be "fill, stroke".

Other valid values I think you should include:

  "fill, stroke, fill"
  "fill, foo"
  "foo"

@@ +1650,5 @@
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "fill", "stroke", "fill stroke" ],
> +    invalid_values: [ "auto", "all", "default", "clipPath", "mask" ]

"default" is an invalid value of properties taking a <custom-ident>.  Some other invalid values for this property would be:

  "none, fill"
  "fill, none"
  "fill, default"
  "fill stroke"
  "2px"

but I think "auto", "all", "clipPath" and "mask" should be removed.

Comment 8

[Cameron McCormack (:heycam)]

David, do you have any bright ideas on how we could restrict the visibility of this property, given it needs to be set from various (internal) WebExtensions, about:reader, video controls XBL bindings, and various other places in the browser that aren't simply chrome://?

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Fix the existing context paint tests to specify '-moz-context-properties'

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Specify '-moz-context-properties' as necessary for Firefox UI icons

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Require '-moz-context-properties' on elements linking to SVG images for the images to use context paint

Comment 12

[Cameron McCormack (:heycam)]

For stylo support, we can follow how will-change is handled.  Here is where the property is defined:

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/servo/components/style/properties/longhand/box.mako.rs#2406

(You'd add your property to svg.mako.rs.)

And here is where it's handled in the glue that sets the value in the Gecko style struct:

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/servo/components/style/properties/gecko.mako.rs#1782-1792

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/servo/components/style/properties/gecko.mako.rs#2358-2442

You can see that it uses FFI functions Gecko_ClearWillChange and Gecko_AppendWillChange and Gecko_CopyWillChangeFrom to deal with the atom array.  You'll need similar functions.  (Although if you keep using nsCOMArray, you could generalize those functions to be general "append atom to an array" function.)  Those functions are defined here:

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/layout/style/ServoBindings.h#360-362

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/layout/style/ServoBindings.cpp#1398-1416

For the gecko.mako.rs changes, you'll need to add them to the SVG struct:

http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/servo/components/style/properties/gecko.mako.rs#3712-3720

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement the '-moz-context-properties' property [r=heycam]

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement the '-moz-context-properties' property [r=heycam]

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8862708 [details] [diff] [review]
part 1 - Implement the '-moz-context-properties' property [r=heycam]

>+  "-moz-context-properties": {
>+    domProp: "MozContextProperties",
>+    inherited: false,
>+    type: CSS_TYPE_LONGHAND,
>+    initial_values: [ "none" ],
>+    other_values: [ "fill", "stroke", "fill, stroke", "fill, stroke, fill", "fill, foo", "foo" ],
>+    invalid_values: [ "default", "fill, auto", "all, stroke", "none, fill", "fill, none", "fill, default", "fill, stroke", "2px" ]
>+  },

I think this should be inherited by default like fill and stroke are.

Comment 16

[Cameron McCormack (:heycam)]

I was going to argue that if this is just going to be used in conjunction with background-image, then it makes sense for it to be a non-inherited property.  But I see from one of the patches on this bug that we're going to use it with list-style-image, which is an inherited property.  So I'd be OK with it being inherited.

Comment 17

[Jonathan Watt [:jwatt]]

The problem with making it inherited is that we use the value of this property to avoid going to the expense of gathering up context paint and creating an object to pass it through when we don't need it. In the vast majority of cases (including all web content) we'll never want that expense. Even in the UI I don't want someone setting this property on the root element and having us go to the expense of creating a context paint object for every single image, including raster images and SVG images that don't use context paint.

Comment 18

[Jonathan Watt [:jwatt]]

See bug 1359762 comment 11 for more background. Plus the other bugs where we've been trying to get the logic for SVGImageContext::MaybeStoreContextPaint right and getting tripped up by something every direction we turn. Unfortunately there's not an obvious perfect solution here.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9018e785e3b7
part 1 - Implement the '-moz-context-properties' property. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f7b692e3c3
part 2 - Fix the existing context paint tests to specify '-moz-context-properties'. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/06abdc6baab2
part 3 - Specify '-moz-context-properties' as necessary for Firefox UI icons. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f67abc9165
part 4 - Require '-moz-context-properties' on elements linking to SVG images using context paint. r=heycam

Comment 20

[Jonathan Watt [:jwatt]]

Dão persuaded me to make '-moz-context-properties' inherit on IRC, so I made that change before landing. You might want to double check the part 1 commit just to make sure you're still happy, Cameron.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8862699 [details] [diff] [review]
part 3 - Specify '-moz-context-properties' as necessary for Firefox UI icons

Isn't this missing your changes from bug 1359631?

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: part 5 - Specify '-moz-context-properties' for more Firefox UI icons

Yes. Grep'ed the wrong tree I think. :/ There's also the changes to page-action.svg that seems to have landed since.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/578b2b0e4324
part 5 - Specify '-moz-context-properties' for more Firefox UI icons. r=dao

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Was http://searchfox.org/mozilla-central/source/devtools/client/responsive.html/index.css#86 covered?  I don't see it in part 3 or 5.

Comment 25

[Daniel Holbert [:dholbert]]

One more needinfo -- SVGImageContext::MaybeStoreContextPaint still contains this code-comment a little ways down, which I thought might've been associated with this bug:
>    // XXX don't set values for properties not listed in 'context-properties'.

It's shown in the context of the last patch here:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/40f67abc9165#l1.55

I think that part's still un-addressed (and still wants to eventually happen).  If that's not already covered by an existing bug, could you file one?

Comment 26

[Jonathan Watt [:jwatt]]

Attachment: part 6 - Specify '-moz-context-properties' for DevTools icons

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8862967 [details] [diff] [review]
part 6 - Specify '-moz-context-properties' for DevTools icons

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

Looks good to me, thanks! :)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92079ba7f449
part 6 - Specify '-moz-context-properties' for DevTools icons. r=jryans

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> One more needinfo -- SVGImageContext::MaybeStoreContextPaint still contains
> this code-comment a little ways down, which I thought might've been
> associated with this bug:
> >    // XXX don't set values for properties not listed in 'context-properties'.

I noticed that too - it's on my todo which I'll get to shortly. Thanks.

Comment 30

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #20)
> Dão persuaded me to make '-moz-context-properties' inherit on IRC, so I made
> that change before landing. You might want to double check the part 1 commit
> just to make sure you're still happy, Cameron.

You need to adjust nsRuleNode::ComputeSVGData to treat unset like inherit.  (Also, I didn't realize that CSS_PROPERTY_INTERNAL makes the property untestable from our usual tests...)

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/9018e785e3b7
https://hg.mozilla.org/mozilla-central/rev/22f7b692e3c3
https://hg.mozilla.org/mozilla-central/rev/06abdc6baab2
https://hg.mozilla.org/mozilla-central/rev/40f67abc9165
https://hg.mozilla.org/mozilla-central/rev/578b2b0e4324
https://hg.mozilla.org/mozilla-central/rev/92079ba7f449

Comment 32

[Jonathan Watt [:jwatt]]

Attachment: part 7 - Treat unset -moz-context-properties as 'inherit'

Good catch, thanks.

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8865267 [details] [diff] [review]
part 7 - Treat unset -moz-context-properties as 'inherit'

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

::: layout/style/nsRuleNode.cpp
@@ +9632,2 @@
>      svg->mContextProps.Clear();
>      svg->mContextProps.AppendElements(parentSVG->mContextProps);

Unrelated to this patch, but FWIW these two lines can just be:

  svg->mContextProps = parentSVG->mContextProps;

Comment 34

[Jonathan Watt [:jwatt]]

Ah, right. That became possible after I switched from nsCOMArray to nsTArray<nsCOMPtr>.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7993e94902
part 7 - Treat unset -moz-context-properties as 'inherit'. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2567a5a33eab
part 8 - Simplify copying of mContextProps in nsRuleNode::ComputeSVGData. r=heycam

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac7993e94902
https://hg.mozilla.org/mozilla-central/rev/2567a5a33eab