477171 - svg file with flowRoot not rendering correctly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[chris hofmann]

I created a block diagram image 
http://people.mozilla.org/~chofmann/open-and-not-so-open-web-v2.svg
in inkscape.

when I view it in inkscape it looks fine.  something like the exported png
http://people.mozilla.org/~chofmann/open-and-not-so-open-web-v2.png

also if I view the .svg file in safari it looks like the png.

when I view the .svg version in firefox 2, 3, or latest trunk the text isn't positioned correctly and big black areas are present in the image.

Comment 1

[chris hofmann]

actually safari gets some of the text positioning wrong too, but not so bad as firefox.  safari also doesn't show the blacked out areas.

Comment 2

[Cameron McCormack (:heycam)]

Inkscape has generated a <flowRoot> element for the text "w3c" and "Javascript Libraries", which is a feature from an old draft of SVG Full 1.2.  It uses a <rect> to describe the area in which to flow the text.  Firefox sees the <rect> and renders it instead, not knowing what <flowRoot> is.  (SVG Tiny 1.2 requires unknown elements not to be rendered; I think that would be better behaviour.)

Can you make Inkscape generate plain (non-wrapped) <text> elements for those bits of text instead?

Comment 3

[Cameron McCormack (:heycam)]

Looks like firefox also doesn't understand the writing-mode:tb-l.  That seems to be a bit of an abuse of that property, though.  Better would be to just rotate the text with a transform.

Comment 4

[chris hofmann]

here is another (shorter) example I've run across

http://people.mozilla.com/~chofmann/slides/sa-trip-200808/en-US/circles-n-slices.svg

http://people.mozilla.com/~chofmann/slides/sa-trip-200808/en-US/circles-n-slices.png 

I'll try playing with rotated text via transform

Comment 5

[Cameron McCormack (:heycam)]

That one also has a <flowRoot> in it (although that one has no text in it).  You could safely remove it.

Comment 6

[chris hofmann]

I think I see a way to reproduce some of the postioning problems

in inkscape
 
   select text entry
   postition the cursor
   type in some text
   -everything looks fine

   then select "vertical text" on the toolbar
   and the text becomes poorly postitioned.

The black splotches seem to be introduced after editing the text inside a box.

From this it looks like cruft is introduced into the .svg as a result of the editing and manipulating the text in inkscape.  

I guess the question is should firefox handle these extra <flowRoot>'s and writing-mode's  better or is this just something that should be fixed in inkscape?

Comment 7

[Cameron McCormack (:heycam)]

(In reply to comment #6)
> I guess the question is should firefox handle these extra <flowRoot>'s and
> writing-mode's  better or is this just something that should be fixed in
> inkscape?

There is bug 319163 for vertical text support in SVG.  I can't immediately see a bug for following the don't-render-subtrees-rooted-at-elements-you-don't-know behaviour required by SVG Tiny 1.2.

I guess it's unfortunate that Inkscape generates these <flowRoot>s, since they were never fully standardised.  Ideally, these should be SVG Tiny 1.2 <textArea>s if they're rectangular areas, or just <text> with manual line breaks if they are more complex shapes.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #2)
> Inkscape has generated a <flowRoot> element for the text "w3c" and "Javascript
> Libraries", which is a feature from an old draft of SVG Full 1.2.  It uses a
> <rect> to describe the area in which to flow the text.  Firefox sees the <rect>
> and renders it instead, not knowing what <flowRoot> is.  (SVG Tiny 1.2 requires
> unknown elements not to be rendered; I think that would be better behaviour.)
> 

I don't :-) and I'm not sure we're likely to ever implement that. See

http://www.croczilla.com/svg/samples/xulsvg1/xulsvg1.xul

The polygon is not a direct child of the svg element. If you don't want the rect to render then ensure it is within a <defs> container.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to comment #8)
> I don't :-) and I'm not sure we're likely to ever implement that. See
> 
> http://www.croczilla.com/svg/samples/xulsvg1/xulsvg1.xul
> 
> The polygon is not a direct child of the svg element. If you don't want the
> rect to render then ensure it is within a <defs> container.

SVG doesn't say anything about how it interacts with XBL, but I think it would be reasonable to say that the rule for not rendering subtrees rooted at unknown elements should apply to the flattened tree, not the actual DOM.  In the XBL 2 spec (I'm aware Mozilla implements XBL and not XBL 2) it says:

  The final flattened tree is the view of the document and shadow trees after
  XBL has been fully applied. It is only used for two things:

  Rendering
    Rendering must be performed using the final flattened tree. Nodes that do
    not appear in the final flattened tree must not be rendered. [...]

  All other processing continues to use the DOM Core tree.
   -- http://www.w3.org/TR/xbl/#the-final

So I suppose that would override SVG.  Although it feels a little strange for one spec to override another, and for an implementation to claim conformance to both.

Was this the only case you were thinking of where the rule would be ignored?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> Firefox sees the <rect>
> and renders it instead, not knowing what <flowRoot> is.  (SVG Tiny 1.2 requires
> unknown elements not to be rendered; I think that would be better behaviour.)

FWIW, this also affects Firefox's rendering of this IE testdrive site:
http://ie.microsoft.com/testdrive/Graphics/SpaceInvader/Default.html

The above demo uses this embedded graphic...
http://ie.microsoft.com/testdrive/Graphics/SpaceInvader/backgrounds/zap.svg
... has an awkward-looking black rect embedded in a <flowRoot>.  Opera (and presumably IE9) don't display the rect. Firefox does.

Comment 13

[Daniel Holbert [:dholbert]]

>... has an awkward-looking black rect
s/... has/... which has/

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #9)
> 
> Was this the only case you were thinking of where the rule would be ignored?

Just realised I never answered this... belatedly, yes, that's the only case.

Comment 16

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

It seems like we should implement this rule of not rendering a subtree of an unknown element (in the flattened tree). Even if we don't particularly like it, it's in a spec (1.2 Tiny, but still), other browsers do it, and it seems content is going to spread that depends on it.

Comment 17

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

Tentatively assigning to Cameron :-)

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Make unknown elements in the SVG namespace not render any children

Comment 19

[Cameron McCormack (:heycam)]

The patch changes the default to generate no frame for an SVG element, rather than defaulting to making a generic container frame.  feDiffuseLighting and feSpecularLighting broke with no frame, so I gave them leaf frames instead.  I included a test for having graphical content as children of all non-container SVG elements (and an unknown element in the SVG namespace) to ensure that they do not render.  nsCSSFrameConstructor::FindSVGData now needs to search for the first non-<a> ancestor when determining the frame type for most SVG elements, since it needs to ensure that frames don't get created for graphical elements that are children of a text content element (text, tspan, etc.).

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Make unknown elements in the SVG namespace not render any children

Updated to add a simple test for symbol, which might have broken with this change.

Comment 21

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

+    }
+    else if (aTag == nsGkAtoms::textPath) {

We go for "} else if ("

+    nsIAtom* ancestorFrameType = ancestorFrame->GetType();

Avoid calling GetType() here unnecessarily; just call it where you need it. It's a virtual call so worth avoiding.

Comment 22

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

Also, in your test where you have basically the same script occurring twice, you should probably move the script to an external file that both test pages pull in.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Make unknown elements in the SVG namespace not render any children (v3)

Updated patch addressing comments

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Make unknown elements in the SVG namespace not render any children (v3.1)

Add r= and a= to check in message.

Comment 25

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/4d725ae3efb9

Comment 26

[Daniel Holbert [:dholbert]]

One of the new reftests included in this push had a 2px failure -- disabled the test for now, and heycam filed bug 610945 for investigation.

Comment 27

[Cameron McCormack (:heycam)]

Attachment: hg changeset for checkin

Comment 28

[Cameron McCormack (:heycam)]

wrong bug, sorry -_-