319786 - xml:space="preserve" in SVG is ignored

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jeff Schiller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

The logo at http://www.codedread.com/images/svg/getsvg.logo.svg uses xml:space="preserve" which renders fine in Adobe SVG Viewer and Opera 9 TP1, yet Firefox ignores the xml:space="preserve" attribute value.

Reproducible: Always

Steps to Reproduce:
1. Browse to logo, all test is shoved to the left (whitespace is ignored)
2.
3.

Actual Results:  
Whitespace is not preserved

Expected Results:  
Whitespace should be preserved

Comment 1

[:Gijs (he/him)]

*** Bug 334661 has been marked as a duplicate of this bug. ***

Comment 2

[Robert Longson [:longsonr]]

*** Bug 346694 has been marked as a duplicate of this bug. ***

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[tor]

Comment on attachment 233238 [details] [diff] [review]
patch

>@@ -419,21 +435,86 @@ void nsSVGTextElement::ParentChainChange
> 
>+  nsIContent *content;
>+  CallQueryInterface(dom_elem, &content);
>+  content->AddMutationObserver(this);
>+

Ouch - that's fairly expensive to add an observer for the containing svg subtree, and even then isn't correct because xml:space could be defined further up.  You need a document-wide observer, I think.

For the Content{Appended,Inserted,Removed} notifications, wouldn't you want to check if the content in question is a child of this <svg:text> element?

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #4)
> 
> Ouch - that's fairly expensive to add an observer for the containing svg
> subtree, and even then isn't correct because xml:space could be defined further
> up.  You need a document-wide observer, I think.

Done. What I really need is to know whether an ancestor element mutated, not a child element, that's why I have to put the observer on the root. If I could get ancestor notification I could put an ancestor and child observer on the text element.


> 
> For the Content{Appended,Inserted,Removed} notifications, wouldn't you want to
> check if the content in question is a child of this <svg:text> element?
> 

I need to update the layout if either a child or a parent of the <svg:text> element changes. I've put in checks to ensure I don't update the layout if some other element e.g. a "cousin" element changes.

Comment 6

[tor]

Comment on attachment 233767 [details] [diff] [review]
address review comments

>+void
>+nsSVGTextElement::CharacterDataChanged(nsIDocument *aDocument,
>+                                       nsIContent *aContent,
>+                                       PRBool aAppend)
>+{
>+}

You can avoid this and the NodeWillBeDestroyed since you're inheriting from nsStubDocumentObserver by just declaring the methods of nsIDocumentObserver you're overloading.

>+void
>+nsSVGTextElement::AttributeChanged(nsIDocument *aDocument,
>+                                   nsIContent *aContent,
>+                                   PRInt32 aNameSpaceID,
>+                                   nsIAtom *aAttribute,
>+                                   PRInt32 aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_XML && aAttribute == nsGkAtoms::space) {
>+    nsIFrame* frame = GetPrimaryFrame();
>+    nsIFrame* contentFrame = GetPrimaryFrameFor(aContent, aDocument);
>+    if (frame == contentFrame ||
>+        nsLayoutUtils::IsProperAncestorFrame(contentFrame, frame, nsnull) ||
>+        nsLayoutUtils::IsProperAncestorFrame(frame, contentFrame, nsnull))
>+      PushUpdate();
>+  }
>+}

Is there a reason for not using the nsContentUtils methods for checking inheritence in this and the the other methods?

Comment 7

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #6)
> You can avoid this and the NodeWillBeDestroyed since you're inheriting from
> nsStubDocumentObserver by just declaring the methods of nsIDocumentObserver
> you're overloading.

Done.

> 
> Is there a reason for not using the nsContentUtils methods for checking
> inheritence in this and the the other methods?
> 

Erm, the most common one of all I suspect... ignorance.

Now changed to use nsContentUtils methods; so much simpler.

Comment 8

[tor]

(In reply to comment #7)
> > Is there a reason for not using the nsContentUtils methods for checking
> > inheritence in this and the the other methods?
> 
> Erm, the most common one of all I suspect... ignorance.

Common problem, given the size of the mozilla source tree.

> Now changed to use nsContentUtils methods; so much simpler.

I'm not sure the GetCommonAncestor() method is the one you want to use, since as long as the elements are in the same document they will have at least the root element as a common ancestor.  The combination of two checks in the previous patch is probably the approach to take, just using ContentIsDescendantOf() instead.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You should be using nsStubMutationObserver and AddMutationObserver

Comment 10

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #8)

> I'm not sure the GetCommonAncestor() method is the one you want to use, since
> as long as the elements are in the same document they will have at least the
> root element as a common ancestor.  The combination of two checks in the
> previous patch is probably the approach to take, just using
> ContentIsDescendantOf() instead.
> 

Done.

(In reply to comment #9)
> You should be using nsStubMutationObserver and AddMutationObserver
> 

Done.

Comment 11

[Hixie (not reading bugmail)]

I don't really understand this bug. Per the XML specs, xml:space takes two values, "preserve" and "default". "preserve" means "don't discard spaces", "default" means "do what you want". We treat "default" to mean "preserve".

What has this to do with SVG?

Comment 12

[Robert Longson [:longsonr]]

(In reply to comment #11)
> I don't really understand this bug. Per the XML specs, xml:space takes two
> values, "preserve" and "default". "preserve" means "don't discard spaces",
> "default" means "do what you want". We treat "default" to mean "preserve".
> 
> What has this to do with SVG?
> 

The Mozilla SVG implementation currently treats all text as if it had xml:space="default"; that is to say it compresses whitespace as per http://www.w3.org/TR/SVG/text.html#WhiteSpace

This bug makes us take the value of xml:space into account as per the SVG spec when we render the text.

Comment 13

[Hixie (not reading bugmail)]

Oh. SVG redefines xml:space. Sigh. Why am I not surprised.

Comment 14

[tor]

Comment on attachment 233973 [details] [diff] [review]
address additional review comments


> nsSVGTextContainerFrame::SetWhitespaceHandling()
> for (nsIFrame *frame = this; frame != nsnull; frame = frame->GetParent()) {
>+    nsIContent *content = frame->GetContent();
>+    if (content->HasAttr(kNameSpaceID_XML, nsGkAtoms::space)) {
>+      if (content->AttrValueIs(kNameSpaceID_XML, nsGkAtoms::space,
>+                               nsGkAtoms::preserve, eCaseMatters))
>+        whitespaceHandling = PRESERVE_WHITESPACE;
>+      break;
>+    }

Just call AttrValueIs - preceeding it with HasAttr doesn't save any processing, and wastes time when the attribute is there.

With that, r=tor.

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #14)

> Just call AttrValueIs - preceeding it with HasAttr doesn't save any processing,
> and wastes time when the attribute is there.
> 
> With that, r=tor.
> 

> Just call AttrValueIs - preceeding it with HasAttr doesn't save any processing,
> and wastes time when the attribute is there.
> With that, r=tor.

If the xml:space attribute is present at all I want to stop going up the tree. If it's "preserve" then I need to change the whitespace processing as well as to stop going up the tree. I could call FindAttrValueIn passing it space and default and check the index if that's what you are suggesting.

Comment 16

[tor]

(In reply to comment #15)
> If the xml:space attribute is present at all I want to stop going up the tree.
> If it's "preserve" then I need to change the whitespace processing as well as
> to stop going up the tree. I could call FindAttrValueIn passing it space and
> default and check the index if that's what you are suggesting.

Didn't notice the "break" - ignore what I said.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You can use FindAttrValueIn to using a single call both check if the attribute is there as well as check if it has the value "space"

Comment 18

[Robert Longson [:longsonr]]

Attachment: updated to use FindAttrValueIn

Comment 19

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

Using a document mutation observer for every SVG text element sounds horrible. I don't think we can afford that.

How about, when the space attribute is changed on an element, we search the subtree and the ancestors for SVG text element(s) and issue some sort of custom notification to them?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, that could easily be done using a single nsIMutationObserver which detects xml:space changes and then walks te subtree looking for nodes that match content->NodeInfo()->Equals(kNameSpaceID_SVG, nsGkAtoms::text)

Comment 21

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

Maybe we could have a single static observer for SVG that gets attached to any document that contains an outerSVG element?

Comment 22

[Robert Longson [:longsonr]]

Attachment: address superreview comments

Only one mutation observer now.

Comment 23

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

> text_frame

textFrame

UpdateTextFragmentTrees can be static.

+      frame->GetStateBits() & NS_STATE_IS_OUTER_SVG)

parenthesize

But why don't you have a single static observer object, and add it as an observer in nsSVGOuterSVGFrame::InitSVG, instead of growing nsSVGOuterSVGFrame?

Comment 24

[Robert Longson [:longsonr]]

Attachment: address additional superreview comments

(In reply to comment #23)
> > text_frame
> 
> textFrame

Done.

> 
> UpdateTextFragmentTrees can be static.

Done.

> 
> +      frame->GetStateBits() & NS_STATE_IS_OUTER_SVG)
> 
> parenthesize

Done.

> 
> But why don't you have a single static observer object, and add it as an
> observer in nsSVGOuterSVGFrame::InitSVG, instead of growing nsSVGOuterSVGFrame?
> 

Done as a static object now.

Comment 25

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

+  if (aNameSpaceID == kNameSpaceID_XML && aAttribute == nsGkAtoms::space) {

I think slightly better style is to turn this into an early return. Saves indentation.

+    nsIPresShell* presShell = aDocument->GetShellAt(0);
+    if (!presShell) {
+      return;
+    }

This should really be a loop over all presshells.

+    if (!frame) {
+      return;
+    }

This would become "continue"

Otherwise looks good!

Comment 26

[Robert Longson [:longsonr]]

Attachment: address additional superreview comments

(In reply to comment #25)
> +  if (aNameSpaceID == kNameSpaceID_XML && aAttribute == nsGkAtoms::space) {
> 
> I think slightly better style is to turn this into an early return. Saves
> indentation.

Done.

> 
> +    nsIPresShell* presShell = aDocument->GetShellAt(0);
> +    if (!presShell) {
> +      return;
> +    }
> 
> This should really be a loop over all presshells.

Done.

> 
> +    if (!frame) {
> +      return;
> +    }
> 
> This would become "continue"

Both return statements are now continue.

Comment 27

[Robert Longson [:longsonr]]

Comment on attachment 240120 [details] [diff] [review]
address additional superreview comments

meant to ask for sr

Comment 28

[Robert Longson [:longsonr]]

checked in.

Comment 29

[Boris Zbarsky [:bzbarsky]]

So why GetOwnerDoc()?  How is this supposed to interact with XBL2?  Please at least add an "// XXX XBL2/sXBL issue" comment or something?

Comment 30

[Robert Longson [:longsonr]]

(In reply to comment #29)
> So why GetOwnerDoc()?  How is this supposed to interact with XBL2?  Please at
> least add an "// XXX XBL2/sXBL issue" comment or something?
> 

Do you mean that

+nsSVGOuterSVGFrame::~nsSVGOuterSVGFrame()
+{
+  nsIDocument *doc = mContent->GetOwnerDoc();

Should have been

+nsSVGOuterSVGFrame::~nsSVGOuterSVGFrame()
+{
+  nsIDocument *doc = mContent->GetCurrentDoc();

I can produce another patch for this if that's the right thing to do.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I mean that depending on how you want SVG in the anonymous content of XBL2 bindings to behave it might need to be GetCurrentDoc(), GetOwnerDoc(), or something else.  The first question is what the desired behavior is.  For example, should the xml:space in the binding document matter?  Or in the bound document?

Comment 32

[Robert Longson [:longsonr]]

(In reply to comment #31)
> I mean that depending on how you want SVG in the anonymous content of XBL2
> bindings to behave it might need to be GetCurrentDoc(), GetOwnerDoc(), or
> something else.  The first question is what the desired behavior is.  For
> example, should the xml:space in the binding document matter?  Or in the bound
> document?
> 

The bound document I believe, that would be the svg output as I understand it from my brief perusal of XBL2. If I understand comment 11 correctly xml:space should do nothing when specified in the binding document, unless the bound document is also the binding document and xml:space occurs within an svg part.

Comment 33

[Boris Zbarsky [:bzbarsky]]

GetOwnerDoc will give you the binding document in XBL2.

Of course using GetCurrentDoc in this code currently is likely to lead to crashes... so I really would add that comment.

Oh, and when you _add_ the observer you put it on GetCurrentDoc().  The inconsistency there is _really_ unfortunate.

Comment 34

[Robert Longson [:longsonr]]

Attachment: address bz concerns

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 240473 [details] [diff] [review]
address bz concerns

So in the usual case (going from page A to page B), but the time ~nsSVGOuterSVGFrame is called there is in fact no current doc in mContent. Can this code handle the fact that sSVGMutationObserver will generally not be removed from the document?

Comment 36

[Robert Longson [:longsonr]]

Attachment: add bz comment

(In reply to comment #35)
> (From update of attachment 240473 [details] [diff] [review] [edit])
> So in the usual case (going from page A to page B), but the time
> ~nsSVGOuterSVGFrame is called there is in fact no current doc in mContent. Can
> this code handle the fact that sSVGMutationObserver will generally not be
> removed from the document?
> 

Oh dear, even moving the code to Destroy doesn't help.

Comment 37

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

(In reply to comment #35)
> Can this code handle the fact that sSVGMutationObserver will generally not be
> removed from the document?

I think so. We don't refcount sSVGMutationObserver anyway.

The only reason we need to try removing it is so that repeated creation/destruction of an nsSVGOuterSVGFrame doesn't cause the document's observer list to grow indefinitely. In fact it's probably *good* that we don't remove the observer on normal page traversals, that would waste time.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 240485 [details] [diff] [review]
add bz comment

Put the same comment on the GetCurrentDoc() call, and ok.

Comment 39

[Robert Longson [:longsonr]]

Comment on attachment 240485 [details] [diff] [review]
add bz comment

Checked in

Comment 40

[Robert Longson [:longsonr]]

Attachment: version checked in

Comment 41

[Jeff Schiller]

Urgh - should have asked for this in Fx2, didn't realize it wasn't going to be there.

Comment 42

[Boris Zbarsky [:bzbarsky]]

The patch would have needed to be seriously rewritten for the 1.8 branch anyway, and by that point the branch was in code-freeze except for release blockers.  So I doubt it would have happened even if you'd asked.

Comment 43

[Robert Longson [:longsonr]]

(In reply to comment #41)
> Urgh - should have asked for this in Fx2, didn't realize it wasn't going to be
> there.
> 

It's too big when you include its dependencies. I.e. bug 349879 and bug 343774 and some trunk bug for the mutation observer base class I think and probably others.

Comment 44

[Jeff Schiller]

Fair enough...

* steps on the Minefield *