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

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

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

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?

(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 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?

(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.

(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.

You should be using nsStubMutationObserver and AddMutationObserver

(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.

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?

(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.

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

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.

(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.

(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.

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"

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?

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)

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

Only one mutation observer now.

> 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?

(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.

+ 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!

(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 on attachment 240120 [details] [diff] [review] address additional superreview comments meant to ask for sr

checked in.

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

(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.

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?

(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.

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 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?

(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.

(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 on attachment 240485 [details] [diff] [review] add bz comment Put the same comment on the GetCurrentDoc() call, and ok.

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

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

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.

(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.

Fair enough... * steps on the Minefield *