Closed Bug 1350010 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwatt, Assigned: jwatt, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 4 obsolete files)

9.01 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.02 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.09 KB, patch
heycam
: review+
Details | Diff | Splinter Review
18.80 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
7.03 KB, patch
dao
: review+
Details | Diff | Splinter Review
997 bytes, patch
jryans
: review+
Details | Diff | Splinter Review
1.06 KB, patch
heycam
: review+
Details | Diff | Splinter Review
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.
The syntax would be "context-properties: fill, fill-opacity, stroke;" for instance?
Yes, that's what I had in mind.
Blocks: 1351236
Blocks: 1351243
Assignee: nobody → jwatt
Attachment #8862687 - Flags: review?(cam)
Attachment #8862687 - Attachment is obsolete: true
Attachment #8862687 - Flags: review?(cam)
Attachment #8862688 - Flags: review?(cam)
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.
Attachment #8862688 - Flags: review?(cam) → review-
Attachment #8862688 - Attachment is obsolete: true
Attachment #8862691 - Flags: review?(cam)
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.
Attachment #8862691 - Flags: review?(cam) → review+
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://?
Flags: needinfo?(dbaron)
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
Attachment #8862698 - Flags: review?(cam) → review+
Attachment #8862699 - Flags: review?(cam) → review+
Attachment #8862700 - Flags: review?(cam) → review+
Attachment #8862691 - Attachment is obsolete: true
Attachment #8862707 - Flags: review+
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.
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.
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.
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.
Pushed by jwatt@jwatt.org:
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
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.
Flags: needinfo?(cam)
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?
Flags: needinfo?(jwatt)
Yes. Grep'ed the wrong tree I think. :/ There's also the changes to page-action.svg that seems to have landed since.
Flags: needinfo?(jwatt)
Attachment #8862929 - Flags: review?(dao+bmo)
Attachment #8862929 - Flags: review?(dao+bmo) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/578b2b0e4324
part 5 - Specify '-moz-context-properties' for more Firefox UI icons. r=dao
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.
Flags: needinfo?(jwatt)
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?
Flags: needinfo?(jwatt)
Attachment #8862967 - Flags: review?(jryans)
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! :)
Attachment #8862967 - Flags: review?(jryans) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92079ba7f449
part 6 - Specify '-moz-context-properties' for DevTools icons. r=jryans
(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.
Blocks: 1360662
(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...)
Flags: needinfo?(cam) → needinfo?(jwatt)
Good catch, thanks.
Flags: needinfo?(jwatt)
Attachment #8865267 - Flags: review?(cam)
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;
Attachment #8865267 - Flags: review?(cam) → review+
Ah, right. That became possible after I switched from nsCOMArray to nsTArray<nsCOMPtr>.
Pushed by jwatt@jwatt.org:
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
Summary: Require explicit opt-in from elements embedding SVG images before exposing context properties → Require explicit opt-in from elements embedding SVG images before exposing context properties [-moz-context-properties]
You need to log in before you can comment on or make changes to this bug.