614265 - Balloon demo is missing the background

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Oskar Ivanić (:icecold)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101123 Firefox/4.0b8pre
Build Identifier: 

How it seems background is only missing in Firefox and Google Chrome 9, but Opera 10.63 and IE Platform Preview 7 display background in demo.


Reproducible: Always

Steps to Reproduce:
1.Go to http://srufaculty.sru.edu/david.dailey/svg/balloon.svg and run the demo
2.Compare it to IE 9
3.
Actual Results:  
Background was missing.

Expected Results:  
Background should be there like in IE 9 or Opera.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The page has a typo: <def> instead of <defs> for the container for the background gradient.

In our case that probably means no frame rather than a defs frame, so no rendering of the kids when referenced.

It's not clear to me what the SVG spec says about the behavior here.

Comment 2

[Robert Longson [:longsonr]]

Children of an unknown element should not display.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, but children without a frame should still be usable as a source pattern, so this is a valid bug. I'm sure we already have a bug on this somewhere but I don't know the number offhand.

Comment 4

[Robert Longson [:longsonr]]

Children of a known element such as g with display="none" should be usable and we do have a bug for that but but children of an unknown element are not usable. 

http://www.w3.org/TR/SVGTiny12/implnote.html#UnsupportedProps

Within an SVG document fragment, any subtree that is rooted by an unknown element (including those in the SVG or XML Events namespaces) or a known element that occurs in unexpected location, is not rendered.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The spec is a bit ambiguous. It's not clear whether "not rendered" and "not processed" mean that paint servers are available or not. I lean towards agreeing that they are not, but this needs to be clarified. Can you raise it in the WG? Otherwise I can.

Comment 6

[Jonathan Watt [:jwatt]]

Talking to Erik Dahlström on IRC:

jwatt: so it's clear that [SVGTiny12] says not to render such elements directly
jwatt: but it also says they are processed as SVG DOM elements correctly
jwatt: so it seems like there is no reason that these elements shouldn't be used
jwatt: for example with <use> or as a paint server
jwatt: because in those cases the unrecognized ancestor doesn't really matter, no?
ed:    right

I'll also bring it up on the WG telcon tomorrow.

Comment 7

[Robert Longson [:longsonr]]

Rendering things we don't create frames for is not something we want to encourage though is it? Without frames we'd have to determine the styles on the fly from content which would be slow.

Comment 8

[Robert Longson [:longsonr]]

I'd even like us to move away from having all that display="none" rendering for SVG 2 and just say if you don't want it rendered put it as a child of a defs element.

Comment 9

[Jonathan Watt [:jwatt]]

I think I'd probably ideally like SVG to be consistent with HTML here. I.e. unknown elements don't prevent rendering, but rather just act similarly to a <g>. Still, I'd be happy to see the spec fixed up to say that display:none content can't be used as a paint server etc.

Assuming for the moment that we go with SVGTiny12 and don't let content under an unknown element be directly rendered, rather than not creating frames we could create a frame that acts like nsSVGGFrame, but for which PaintSVG just returns (doesn't paint its descendants). I think that's more useful for authors, and unlike the display:none case, there aren't good implementation reasons not to do that.

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #9)
> 
> Assuming for the moment that we go with SVGTiny12 and don't let content under
> an unknown element be directly rendered, rather than not creating frames we
> could create a frame that acts like nsSVGGFrame, but for which PaintSVG just
> returns (doesn't paint its descendants). 

That's basically a nsSVGDefsFrame.

Comment 11

[Robert Longson [:longsonr]]

And perhaps that's what we should do i.e. create a nsSVGDefsFrame each time we encounter an unknown element rather than creating no frame at all.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> And perhaps that's what we should do i.e. create a nsSVGDefsFrame each time we
> encounter an unknown element rather than creating no frame at all.

That sounds reasonable and would be enough to resolve this bug.

If we're going to get the spec changed so that display:none content can't be used as a paint server, I think we need a stronger reason than "it's broken in our implementation". We could fix our implementation.

I think where the current spec gets tricky is when a display:none element contains a pattern or mask that contains a foreignobject. CSS 2.1 says that display:none means no boxes are created for any descendants of the element, so it seems to me that per spec those foreignobjects render nothing, which may be unexpected. But personally I don't feel that's quite enough to want to ban paint servers in display:none subtrees altogether.

We can probably fix our implementation in the frame constructor, by detecting when a display:none element has paint server descendants, and creating special frames --- like nsSVGDefsFrame --- for the elements between the display:none element and the paint servers.

Comment 13

[Cameron McCormack (:heycam)]

I agree that elements within a display:none element should be referenceable by use, as paint servers, etc.

(In reply to comment #12)
> We can probably fix our implementation in the frame constructor, by detecting
> when a display:none element has paint server descendants, and creating special
> frames --- like nsSVGDefsFrame --- for the elements between the display:none
> element and the paint servers.

Is it too much to use something like nsSVGDefsFrame for *all* SVG elements that currently otherwise get no frame (those with display:none, or unknown SVG namespace elements)?

Comment 14

[Robert Longson [:longsonr]]

Almost, animation type elements need to be frameless.

Comment 15

[Cameron McCormack (:heycam)]

OK.  (What is the consequence of them getting some generic frame?)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> Is it too much to use something like nsSVGDefsFrame for *all* SVG elements that
> currently otherwise get no frame (those with display:none, or unknown SVG
> namespace elements)?

That isn't enough to fix the case where you have a display:none HTML element containing an SVG fragment containing a paint server.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(or a display:none SVG element containing a <foreignobject> containing an SVG fragment containing a paint server)

Comment 18

[Cameron McCormack (:heycam)]

Hmm, right.  Presumably giving some generic, non-displaying frame to display:none HTML elements will be too costly (as they will be much more common in HTML documents rather than SVG documents)?  Can a single generic, non-displaying frame be created for display:none HTML elements that can be shared between all elements (per document, say)?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Frame is not the issue per se.  The issue is that we optimize display:none subtrees by not resolving style on anything in the subtree.  We don't really want to remove that optimization, in general.

Comment 20

[Cameron McCormack (:heycam)]

(This might be a larger issue than this bug, but: ) Is this optimization actually valid?  Per spec, should I be able to look up the computed style of the 'color' property on an element deeply nested within a display:none subtree?  And if so, could we possibly keep the optimization for the common case of content not caring about the subtree, but if it does (by getting computed style, or referencing an element as a paint server, etc.) lazily construct the frames then?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Is this optimization actually valid? 

Yes.

> should I be able to look up the computed style of the 'color' property on an
> element deeply nested within a display:none subtree?

Sure.  And if you do that, we will resolve its style.  We just won't cache the result anywhere.  See also comment 7.

> lazily construct the frames then

That still adds overhead, unless we take further steps to keep the frames from being reflowed and a few other things.  It might be doable; it'd just take work, especially to make sure the new setup is safe.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think constructing dummy frames for the ancestors of paint servers in display:none subtrees would be a good tradeoff between performance and correctness. Perfectly correct, and most unlikely to hurt performance in practice, assuming we use node flags or something to track the presence of paint server descendants.

Comment 23

[Robert Longson [:longsonr]]

We've bug 376027 for solving display="none" so we could limit this bug to a simple nsSVGDefsFrame for unknown elements.

animation type elements need to be frameless or animated clip paths don't display properly. desc and title are also frameless and it's probably OK to leave them frameless too as they can't themselves have useful children.

Non-active switch children are frameless and that *would* need fixing. 

There are some comments in nsCSSFrameConstructor.cpp about all this.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Make paint server references to elements within an unknown SVG element subtree work again

Comment 25

[Cameron McCormack (:heycam)]

The patch also makes the otherwise currently unused nsSVGGenericContainerFrame inherit from nsSVGContainerFrame rather than nsSVGDisplayContainerFrame.  Actually, I am not sure whether nsSVGGenericContainerFrame serves much of a purpose over simply nsSVGContainerFrame.

Comment 26

[Robert Longson [:longsonr]]

Comment on attachment 493196 [details] [diff] [review]
Make paint server references to elements within an unknown SVG element subtree work again

nsSVGGenericContainer frame is used by xbl for extends="svg:generic". See bug 378518 for some discussion. I don't know where it's really documented though but I suspect asking Boris would be revealing. There's another example here: http://www.croczilla.com/~alex/fosdem2003/behaviour-with-xbl.xml.

Changing nsSVGGenericContainerFrame to inherit from nsSVGContainerFrame is incorrect as that will break that xbl usage I think by causing the frame not to display. We really ought to have a reftest to prevent this from being changed if the current reftests all pass with your change in. Perhaps something adapted from the croczilla example?

switch also needs fixing to not suppress its children (see higher in that same method) that you changed in nsCSSFrameConstructor.

Comment 27

[Cameron McCormack (:heycam)]

Attachment: Make paint server references to elements within an unknown SVG element subtree work again (v2)

Updated patch

Thanks for the comments Robert.

> nsSVGGenericContainer frame is used by xbl for extends="svg:generic".
> See bug 378518 for some discussion. I don't know where it's really
> documented though but I suspect asking Boris would be revealing.
> There's another example here:
> http://www.croczilla.com/~alex/fosdem2003/behaviour-with-xbl.xml.

I see.  I've now added an explicit entry to the frame constructor info
array for "svg:generic", making it get an nsSVGGenericContainer again.
Other unknown names in the SVG namespace still get nsSVGContainerFrame.

> Changing nsSVGGenericContainerFrame to inherit from
> nsSVGContainerFrame is incorrect as that will break that xbl usage I
> think by causing the frame not to display. We really ought to have a
> reftest to prevent this from being changed if the current reftests all
> pass with your change in. Perhaps something adapted from the croczilla
> example?

In the original patch I had planned to use the repurposed-
nsSVGGenericContainerFrame for unknown SVG elements, but that changed
was left out of the patch for some reason.  Anyway.  I've added a test
for this (xbl-basic-03.svg).

> switch also needs fixing to not suppress its children (see higher in
> that same method) that you changed in nsCSSFrameConstructor.

OK.  I've made that give nsSVGContainerFrames to elements with failing
conditional attributes.  I also removed the comments there because as
far as I can tell, <use> targetting elements within failing-conditional-
attribute subtrees works.  nsSVGUseElement uses nsReferencedElement
rather than GetReferencedFrame.  (<use> targetting an element which
itself has failing conditional attribtues should result in no rendering,
as evidenced by other implementations and a test in the W3C test suite.)
I added a test for this too, even though it wasn't failing before this
patch.

Comment 28

[Cameron McCormack (:heycam)]

Attachment: XBL binding extending svg:generic on an unknown element

A related question: should this example work?  It shows red in current builds (and after my patch).

Comment 29

[Robert Longson [:longsonr]]

(In reply to comment #28)
> Created attachment 493330 [details]
> XBL binding extending svg:generic on an unknown element
> 
> A related question: should this example work?  It shows red in current builds
> (and after my patch).

I guess not as it shows red on Firefox 3.6. There are more xbl examples that do work on Firefox 3.6 here... http://croczilla.com/bits_and_pieces/svg/samples/

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/bb43b6a9b621