svg file with flowRoot not rendering correctly

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: chris hofmann, Assigned: heycam)

Tracking

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
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.
(Assignee)

Comment 2

8 years ago
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?
(Assignee)

Comment 3

8 years ago
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.
(Reporter)

Comment 4

8 years ago
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
(Assignee)

Comment 5

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

Comment 6

8 years ago
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?
(Assignee)

Comment 7

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
(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?

Updated

8 years ago
Duplicate of this bug: 476396

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All

Updated

8 years ago
Summary: svg file created in inkscape not rendering correctly → svg file with flowRoot not rendering correctly

Updated

8 years ago
Duplicate of this bug: 517423
(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.
>... has an awkward-looking black rect
s/... has/... which has/

Updated

7 years ago
Duplicate of this bug: 603666

Comment 15

7 years ago
(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.
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.
blocking2.0: --- → final+
Tentatively assigning to Cameron :-)
Assignee: nobody → cam
(Assignee)

Comment 18

7 years ago
Created attachment 489313 [details] [diff] [review]
Make unknown elements in the SVG namespace not render any children
(Assignee)

Comment 19

7 years ago
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.).
(Assignee)

Updated

7 years ago
Attachment #489313 - Attachment is obsolete: true
(Assignee)

Comment 20

7 years ago
Created attachment 489371 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Attachment #489371 - Flags: review?(roc)
+    }
+    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.
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.
(Assignee)

Updated

7 years ago
Attachment #489371 - Attachment is obsolete: true
Attachment #489371 - Flags: review?(roc)
(Assignee)

Comment 23

7 years ago
Created attachment 489403 [details] [diff] [review]
Make unknown elements in the SVG namespace not render any children (v3)

Updated patch addressing comments
(Assignee)

Updated

7 years ago
Attachment #489403 - Flags: review?(roc)
Attachment #489403 - Flags: review?(roc) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Attachment #489403 - Attachment is obsolete: true
(Assignee)

Comment 24

7 years ago
Created attachment 489404 [details] [diff] [review]
Make unknown elements in the SVG namespace not render any children (v3.1)

Add r= and a= to check in message.
Landed:
http://hg.mozilla.org/mozilla-central/rev/4d725ae3efb9
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Flags: in-testsuite+
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.

Updated

7 years ago
Blocks: 449215
(Assignee)

Comment 27

7 years ago
Created attachment 490732 [details] [diff] [review]
hg changeset for checkin
(Assignee)

Comment 28

7 years ago
wrong bug, sorry -_-
Depends on: 613999
Attachment #490732 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.