Bug 6899 – SVG <rect> does not respect display: none

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVG does not respect display: none

See attached test case.  This should be easy to fix.

[Eric Seidel (no email)]

Created attachment 6061 [details]
simple test case

[Eric Seidel (no email)]

This is a pretty fundemental rendering bug.  It might even be worthy of the SVG HitList, we wouldn't ship SVG w/o this.

[Rob Buis]

Created attachment 6924 [details]
Patch to prevent rendering of svg elements for display=none

This is a fix to the problem. Please let me know if
it is too simplistic and/or needs more work.
Cheers,

Rob.

[Maciej Stachowiak]

I think this fix would be ok, but an even better fix would be to make the renderIsNeeded method do the right thing. The default implementation in NodeImpl::renderIsNeeded should handle the display:none case. However, this breaks because of the way SVG supresses rendering for unknown elements and for certain SVG elements. SVGElementImpl has an override of renderIsNeeded which returns false, and then the elements that do render have another override that returns true. This drops the display:none handling.

Here are two possible fixes:

1) Slightly easier, but architecturally not as good IMO:

For all the SVGElementImpl subclasses that currently override renderIsNeeded to just return true, instead override them to call StyledElementImpl::rendererIsNeeded so they skip the forced false and fall through to NodeImpl's logic.

2) A bit more work, but architecturally better:

Make an SVGUnknownElementImpl or SVGGenericElementImpl or something like that, derived from SVGElementImpl, to be used for unknown elements in the SVG namespace. This would require some changes to the factory. Remove the renderIsNeeded override from SVGElementImpl. Remove all the true overrides. Add a false override to SVGUnknownElementImpl and any other classes that should never create a renderer. Alternately, for the latter, you could put a display: none rule in the svg.css stylesheet.

[Rob Buis]

Created attachment 6936 [details]
New patch which implements solution 1 as described by maciej

See description :)

[Eric Seidel (no email)]

Comment on attachment 6936 [details]
New patch which implements solution 1 as described by maciej

This looks fine.  This will need a change log (and the attached test case) when landing.   We also might consider someday reversing this rule such that all SVGStyleElementImpls which do not want renders implement rendererIsNeeded and return false, where all other renders just get the StyledElementImpl::renderIsNeeded behavior by default.

[Rob Buis]

Created attachment 6938 [details]
More complete patch

[Eric Seidel (no email)]

Comment on attachment 6938 [details]
More complete patch

Oops! tabs in your change log.  Otherwise very nice.  I'll fix the tabs as I land.
r=me.

[Eric Seidel (no email)]

I landed this a while back.