Gradients should restrict possible content

Gradients should restrict possible content

Gradients have specific rules about the content that is
allowed, change the code to allow only <stop> and paint
servers as children.
Also add a dump render tree test for it to verify content
other than <stop> or paint servers do not get rendered.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183477

[rwlbuis, 2014-10-01 23:23:03]

rob.buis@samsung.com changed reviewers:
+ pdr@chromium.org

[rwlbuis, 2014-10-01 23:23:03]

PTAL

[pdr., 2014-10-01 23:35:05]

This is crazy, but this seems to be spec'd to allow for children that require
renderers:
https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement

[rwlbuis, 2014-10-01 23:47:02]

On 2014/10/01 23:35:05, pdr wrote:
> This is crazy, but this seems to be spec'd to allow for children that require
> renderers:
> https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement

How do you mean? The content model section still restricts it a lot.
OTOH paint servers are allowed as children, not sure how it should work and what
we do right now, that may be another bug...

[pdr., 2014-10-01 23:49:29]

On 2014/10/01 at 23:47:02, rob.buis wrote:
> On 2014/10/01 23:35:05, pdr wrote:
> > This is crazy, but this seems to be spec'd to allow for children that
require
> > renderers:
> > https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement
> 
> How do you mean? The content model section still restricts it a lot.
> OTOH paint servers are allowed as children, not sure how it should work and
what we do right now, that may be another bug...

Here's an example of what I mean: http://jsfiddle.net/jjk5e2jc

[Erik Dahlström (inactive), 2014-10-02 13:42:42]

On 2014/10/01 23:49:29, pdr wrote:
> On 2014/10/01 at 23:47:02, rob.buis wrote:
> > On 2014/10/01 23:35:05, pdr wrote:
> > > This is crazy, but this seems to be spec'd to allow for children that
> require
> > > renderers:
> > >
https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement
> > 
> > How do you mean? The content model section still restricts it a lot.
> > OTOH paint servers are allowed as children, not sure how it should work and
> what we do right now, that may be another bug...
> 
> Here's an example of what I mean: http://jsfiddle.net/jjk5e2jc

Specwise: that was added in https://svgwg.org/hg/svg2/rev/441, and I just asked
Brian about it (no reply yet). It may be a mistake on his part.

The SVG 1.1 content model doesn't allow it for sure. I think it's reasonable to
filter out all elements that aren't stop elements.

[rwlbuis, 2014-10-02 14:08:10]

On 2014/10/02 13:42:42, Erik Dahlström wrote:
> On 2014/10/01 23:49:29, pdr wrote:
> > On 2014/10/01 at 23:47:02, rob.buis wrote:
> > > On 2014/10/01 23:35:05, pdr wrote:
> > > > This is crazy, but this seems to be spec'd to allow for children that
> > require
> > > > renderers:
> > > >
> https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement
> > > 
> > > How do you mean? The content model section still restricts it a lot.
> > > OTOH paint servers are allowed as children, not sure how it should work
and
> > what we do right now, that may be another bug...
> > 
> > Here's an example of what I mean: http://jsfiddle.net/jjk5e2jc
> 
> Specwise: that was added in https://svgwg.org/hg/svg2/rev/441, and I just
asked
> Brian about it (no reply yet). It may be a mistake on his part.
> 
> The SVG 1.1 content model doesn't allow it for sure. I think it's reasonable
to
> filter out all elements that aren't stop elements.

I agree my patch is correct when keeping to the letter of SVG 1.1 spec. Pdr's
jsfiddle
example btw looks the same in both Firefox and trunk, i.e. the "gradient in the
gradient" is found.
The question is if this was intended behavior, and if we should enable web
developers to write
incorrect SVG (at least according to 1.1) just because you can put a gradient in
a gradient... Personally
I'd say go for the patch and if something breaks we can re-add the ability to
embed paint server in
other paint servers.

[fs, 2014-10-02 14:16:29]

On 2014/10/02 14:08:10, rwlbuis wrote:
> On 2014/10/02 13:42:42, Erik Dahlström wrote:
> > On 2014/10/01 23:49:29, pdr wrote:
> > > On 2014/10/01 at 23:47:02, rob.buis wrote:
> > > > On 2014/10/01 23:35:05, pdr wrote:
> > > > > This is crazy, but this seems to be spec'd to allow for children that
> > > require
> > > > > renderers:
> > > > >
> > https://svgwg.org/svg2-draft/single-page.html#pservers-LinearGradientElement
> > > > 
> > > > How do you mean? The content model section still restricts it a lot.
> > > > OTOH paint servers are allowed as children, not sure how it should work
> and
> > > what we do right now, that may be another bug...
> > > 
> > > Here's an example of what I mean: http://jsfiddle.net/jjk5e2jc
> > 
> > Specwise: that was added in https://svgwg.org/hg/svg2/rev/441, and I just
> asked
> > Brian about it (no reply yet). It may be a mistake on his part.
> > 
> > The SVG 1.1 content model doesn't allow it for sure. I think it's reasonable
> to
> > filter out all elements that aren't stop elements.
> 
> I agree my patch is correct when keeping to the letter of SVG 1.1 spec. Pdr's
> jsfiddle
> example btw looks the same in both Firefox and trunk, i.e. the "gradient in
the
> gradient" is found.
> The question is if this was intended behavior, and if we should enable web
> developers to write
> incorrect SVG (at least according to 1.1) just because you can put a gradient
in
> a gradient... Personally
> I'd say go for the patch and if something breaks we can re-add the ability to
> embed paint server in
> other paint servers.

For gradients nesting doesn't make a tonne of sense - but for <pattern>s it's
less obvious that it shouldn't be allowed (re: "the ability to embed paint
server in other paint servers.")

[rwlbuis, 2014-10-02 14:31:05]

On 2014/10/02 14:16:29, fs wrote:
> On 2014/10/02 14:08:10, rwlbuis wrote:
> > I agree my patch is correct when keeping to the letter of SVG 1.1 spec.
Pdr's
> > jsfiddle
> > example btw looks the same in both Firefox and trunk, i.e. the "gradient in
> the
> > gradient" is found.
> > The question is if this was intended behavior, and if we should enable web
> > developers to write
> > incorrect SVG (at least according to 1.1) just because you can put a
gradient
> in
> > a gradient... Personally
> > I'd say go for the patch and if something breaks we can re-add the ability
to
> > embed paint server in
> > other paint servers.
> 
> For gradients nesting doesn't make a tonne of sense - but for <pattern>s it's
> less obvious that it shouldn't be allowed (re: "the ability to embed paint
> server in other paint servers.")

Right, my statement was too generic. I just checked and <pattern> allows a lot
more 
in its content model than gradients do, including in fact other patterns (I
checked 1.1).

[pdr., 2014-10-02 15:45:06]

Because this breaks compatibility across browsers and moves away from the spec,
I'd prefer if we at updated the spec in parallel to landing this.

[krit, 2014-10-02 15:54:41]

On 2014/10/02 15:45:06, pdr wrote:
> Because this breaks compatibility across browsers and moves away from the
spec,
> I'd prefer if we at updated the spec in parallel to landing this.

If Brian added it, then probably on purpose. The SVG WG wants to have a selector
that allows referencing a paint server that is a child of the current element:

<rect fill="child">
   <linearGradient>....</linearGradient>
</rect>

If you do that, then it makes sense to allow nesting. After all, gradients can
reference other gradients:

<linearGradient href="child">
  <linearGradient></linearGradient>
</linearGradient>

[rwlbuis, 2014-10-02 17:59:34]

On 2014/10/02 15:54:41, krit wrote:
> On 2014/10/02 15:45:06, pdr wrote:
> > Because this breaks compatibility across browsers and moves away from the
> spec,
> > I'd prefer if we at updated the spec in parallel to landing this.
> 
> If Brian added it, then probably on purpose. The SVG WG wants to have a
selector
> that allows referencing a paint server that is a child of the current element:
> 
> <rect fill="child">
>    <linearGradient>....</linearGradient>
> </rect>
> 
> If you do that, then it makes sense to allow nesting. After all, gradients can
> reference other gradients:
> 
> <linearGradient href="child">
>   <linearGradient></linearGradient>
> </linearGradient>

Conscensus seems to be pservers should be allowed. So my
latest patch set allows it and also incorporates pdr's jsfiddle bits.

[pdr., 2014-10-02 22:07:37]

On 2014/10/02 at 17:59:34, rob.buis wrote:
> On 2014/10/02 15:54:41, krit wrote:
> > On 2014/10/02 15:45:06, pdr wrote:
> > > Because this breaks compatibility across browsers and moves away from the
> > spec,
> > > I'd prefer if we at updated the spec in parallel to landing this.
> > 
> > If Brian added it, then probably on purpose. The SVG WG wants to have a
selector
> > that allows referencing a paint server that is a child of the current
element:
> > 
> > <rect fill="child">
> >    <linearGradient>....</linearGradient>
> > </rect>
> > 
> > If you do that, then it makes sense to allow nesting. After all, gradients
can
> > reference other gradients:
> > 
> > <linearGradient href="child">
> >   <linearGradient></linearGradient>
> > </linearGradient>
> 
> Conscensus seems to be pservers should be allowed. So my
> latest patch set allows it and also incorporates pdr's jsfiddle bits.

Can we do all of the paint servers at once? For example, patterns are still much
more lax than the spec allows.

[rwlbuis, 2014-10-03 17:56:40]

On 2014/10/02 22:07:37, pdr wrote:
> On 2014/10/02 at 17:59:34, rob.buis wrote:
> > On 2014/10/02 15:54:41, krit wrote:
> > > On 2014/10/02 15:45:06, pdr wrote:
> > > > Because this breaks compatibility across browsers and moves away from
the
> > > spec,
> > > > I'd prefer if we at updated the spec in parallel to landing this.
> > > 
> > > If Brian added it, then probably on purpose. The SVG WG wants to have a
> selector
> > > that allows referencing a paint server that is a child of the current
> element:
> > > 
> > > <rect fill="child">
> > >    <linearGradient>....</linearGradient>
> > > </rect>
> > > 
> > > If you do that, then it makes sense to allow nesting. After all, gradients
> can
> > > reference other gradients:
> > > 
> > > <linearGradient href="child">
> > >   <linearGradient></linearGradient>
> > > </linearGradient>
> > 
> > Conscensus seems to be pservers should be allowed. So my
> > latest patch set allows it and also incorporates pdr's jsfiddle bits.
> 
> Can we do all of the paint servers at once? For example, patterns are still
much
> more lax than the spec allows.

I started on this but actually not sure what to do:

- pattern allows the world as children so I think we are already doing the right
thing
- we do not support solidColor/hatch etc. so nothing to do there now

<stop> seems to allow pserver children:

https://svgwg.org/svg2-draft/pservers.html#StopElement

This seems like a bigger change since RenderSVGGradient would need to derive
from
RenderSVGResourceContainer to support the children? Firefox does not support
pservers
as children for <stop> either atm.

[pdr., 2014-10-03 18:00:26]

On 2014/10/03 at 17:56:40, rob.buis wrote:
> On 2014/10/02 22:07:37, pdr wrote:
> > On 2014/10/02 at 17:59:34, rob.buis wrote:
> > > On 2014/10/02 15:54:41, krit wrote:
> > > > On 2014/10/02 15:45:06, pdr wrote:
> > > > > Because this breaks compatibility across browsers and moves away from
the
> > > > spec,
> > > > > I'd prefer if we at updated the spec in parallel to landing this.
> > > > 
> > > > If Brian added it, then probably on purpose. The SVG WG wants to have a
> > selector
> > > > that allows referencing a paint server that is a child of the current
> > element:
> > > > 
> > > > <rect fill="child">
> > > >    <linearGradient>....</linearGradient>
> > > > </rect>
> > > > 
> > > > If you do that, then it makes sense to allow nesting. After all,
gradients
> > can
> > > > reference other gradients:
> > > > 
> > > > <linearGradient href="child">
> > > >   <linearGradient></linearGradient>
> > > > </linearGradient>
> > > 
> > > Conscensus seems to be pservers should be allowed. So my
> > > latest patch set allows it and also incorporates pdr's jsfiddle bits.
> > 
> > Can we do all of the paint servers at once? For example, patterns are still
much
> > more lax than the spec allows.
> 
> I started on this but actually not sure what to do:
> 
> - pattern allows the world as children so I think we are already doing the
right thing
> - we do not support solidColor/hatch etc. so nothing to do there now
> 
> <stop> seems to allow pserver children:
> 
> https://svgwg.org/svg2-draft/pservers.html#StopElement
> 
> This seems like a bigger change since RenderSVGGradient would need to derive
from
> RenderSVGResourceContainer to support the children? Firefox does not support
pservers
> as children for <stop> either atm.

Thank you for looking into this. We should probably ask for the spec to be
updated to disallow all children for <stop>

LGTM for this patch as-is. Could you please wait for a second LGTM from fs@opera
or dschulze before landing?

[rwlbuis, 2014-10-03 18:03:30]

On 2014/10/03 18:00:26, pdr wrote:
> On 2014/10/03 at 17:56:40, rob.buis wrote:
> > On 2014/10/02 22:07:37, pdr wrote:
> > > On 2014/10/02 at 17:59:34, rob.buis wrote:
> > > > On 2014/10/02 15:54:41, krit wrote:
> > > > > On 2014/10/02 15:45:06, pdr wrote:
> > > > > > Because this breaks compatibility across browsers and moves away
from
> the
> > > > > spec,
> > > > > > I'd prefer if we at updated the spec in parallel to landing this.
> > > > > 
> > > > > If Brian added it, then probably on purpose. The SVG WG wants to have
a
> > > selector
> > > > > that allows referencing a paint server that is a child of the current
> > > element:
> > > > > 
> > > > > <rect fill="child">
> > > > >    <linearGradient>....</linearGradient>
> > > > > </rect>
> > > > > 
> > > > > If you do that, then it makes sense to allow nesting. After all,
> gradients
> > > can
> > > > > reference other gradients:
> > > > > 
> > > > > <linearGradient href="child">
> > > > >   <linearGradient></linearGradient>
> > > > > </linearGradient>
> > > > 
> > > > Conscensus seems to be pservers should be allowed. So my
> > > > latest patch set allows it and also incorporates pdr's jsfiddle bits.
> > > 
> > > Can we do all of the paint servers at once? For example, patterns are
still
> much
> > > more lax than the spec allows.
> > 
> > I started on this but actually not sure what to do:
> > 
> > - pattern allows the world as children so I think we are already doing the
> right thing
> > - we do not support solidColor/hatch etc. so nothing to do there now
> > 
> > <stop> seems to allow pserver children:
> > 
> > https://svgwg.org/svg2-draft/pservers.html#StopElement
> > 
> > This seems like a bigger change since RenderSVGGradient would need to derive
> from
> > RenderSVGResourceContainer to support the children? Firefox does not support
> pservers
> > as children for <stop> either atm.
> 
> Thank you for looking into this. We should probably ask for the spec to be
> updated to disallow all children for <stop>
> 
> LGTM for this patch as-is. Could you please wait for a second LGTM from
fs@opera
> or dschulze before landing?

Sure. In fact I think I want to get rid of the "resource->resourceType() ==
SolidColorResourceType" check
since I wrongly assumed solidColor was supported, so I'll upload one more patch
set...

[rwlbuis, 2014-10-07 14:40:30]

On 2014/10/03 18:00:26, pdr wrote:
> Thank you for looking into this. We should probably ask for the spec to be
> updated to disallow all children for <stop>

I mailed to www-svg, hope it will show up soon.

[rwlbuis, 2014-10-09 14:01:16]

On 2014/10/03 18:00:26, pdr wrote:
> On 2014/10/03 at 17:56:40, rob.buis wrote:
> > On 2014/10/02 22:07:37, pdr wrote:
> > > On 2014/10/02 at 17:59:34, rob.buis wrote:
> > > > On 2014/10/02 15:54:41, krit wrote:
> > > > > On 2014/10/02 15:45:06, pdr wrote:
> > > > > > Because this breaks compatibility across browsers and moves away
from
> the
> > > > > spec,
> > > > > > I'd prefer if we at updated the spec in parallel to landing this.
> > > > > 
> > > > > If Brian added it, then probably on purpose. The SVG WG wants to have
a
> > > selector
> > > > > that allows referencing a paint server that is a child of the current
> > > element:
> > > > > 
> > > > > <rect fill="child">
> > > > >    <linearGradient>....</linearGradient>
> > > > > </rect>
> > > > > 
> > > > > If you do that, then it makes sense to allow nesting. After all,
> gradients
> > > can
> > > > > reference other gradients:
> > > > > 
> > > > > <linearGradient href="child">
> > > > >   <linearGradient></linearGradient>
> > > > > </linearGradient>
> > > > 
> > > > Conscensus seems to be pservers should be allowed. So my
> > > > latest patch set allows it and also incorporates pdr's jsfiddle bits.
> > > 
> > > Can we do all of the paint servers at once? For example, patterns are
still
> much
> > > more lax than the spec allows.
> > 
> > I started on this but actually not sure what to do:
> > 
> > - pattern allows the world as children so I think we are already doing the
> right thing
> > - we do not support solidColor/hatch etc. so nothing to do there now
> > 
> > <stop> seems to allow pserver children:
> > 
> > https://svgwg.org/svg2-draft/pservers.html#StopElement
> > 
> > This seems like a bigger change since RenderSVGGradient would need to derive
> from
> > RenderSVGResourceContainer to support the children? Firefox does not support
> pservers
> > as children for <stop> either atm.
> 
> Thank you for looking into this. We should probably ask for the spec to be
> updated to disallow all children for <stop>
> 
> LGTM for this patch as-is. Could you please wait for a second LGTM from
fs@opera
> or dschulze before landing?

@fs and @dschulze, can you have a look?

[rwlbuis, 2014-10-09 14:02:22]

rob.buis@samsung.com changed reviewers:
+ dschulze@chromium.org, fs@opera.com

[rwlbuis, 2014-10-09 14:02:23]

PTAL

[Erik Dahlström (inactive), 2014-10-09 14:42:58]

Spec should get updated soon,
http://www.w3.org/2014/10/09-svg-minutes.html#item02 (brian has an action to
drop the "paint server elements" from the content model of <stop> and <meshRow>)

[rwlbuis, 2014-10-09 14:44:31]

On 2014/10/09 14:42:58, Erik Dahlström wrote:
> Spec should get updated soon,
> http://www.w3.org/2014/10/09-svg-minutes.html#item02 (brian has an action to
> drop the "paint server elements" from the content model of <stop> and
<meshRow>)

I saw it in the minutes, thanks!

[fs, 2014-10-09 14:59:07]

LGTM w/ trivial nit.

https://codereview.chromium.org/622653003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.h (right):

https://codereview.chromium.org/622653003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.h:54: virtual bool
isChildAllowed(RenderObject* child, RenderStyle*) const OVERRIDE FINAL;
Nit: Use lowercase override/final (will probably conflict anyway though.)

[rwlbuis, 2014-10-09 15:36:23]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-10-09 15:37:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/622653003/80001

[commit-bot: I haz the power, 2014-10-09 16:40:54]

Committed patchset #5 (id:80001) as 183477