235826 - Implement <svg:script> tag

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Fritze]

This looks easy enough; we just follow the example set by nsHTMLScriptElement.
Unfortunately we can't use nsHTMLScriptElement directly, because the svg script
element needs to implement different DOM interfaces (nsIDOMSVGScriptElement -
yet to be checked into dom/public/idl/svg).

Complications:
- It looks like we need to make some changes in nsXMLContentSink::CreateElement
so that we can do similar special casing as is done for the html:script element.
(We need to set mConstrainSize=false and aAppendContent=false). yuck.

Any thoughts welcome.

Setting severity to major, because this is a major compatibility issue. People
just assume *no* DOM functionality is implemented in Mozilla SVG because of it.

Comment 1

[Hixie (not reading bugmail)]

bz: it seems that <svg:script> (and <svg:style>, and .class in svg, etc) should
all be really easy to do, but afri tells me they are less trivial than one would
imagine. Is there any way you can think of that would make adding these kind of
elements a lot easier? (I'm thinking especially of <svg:style>, in your case,
since that's your speciality. Let me know if you want a bug specially for that.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

<svg:style> _is_ easy.  All the style logic lives in nsStyleLinkElement.cpp.  So
you inherit from that and just implement the callbacks it uses to get your info
(like the sheet url, etc).  Isn't there a separate bug on <style>, though?

Comment 3

[Alex Fritze]

<svg:script> certainly required more refactoring than I would have hoped for.
Biggish patch coming up. I think we should move implementation of <svg:style> &
.class to other bugs.

Comment 4

[Alex Fritze]

Attachment: patch to implement <svg:script>

This patch implements <svg:script> (both inline and with xlink:href-syntax) for
svg & mixed xml documents. <svg:script> is XUL docs will most probably not work
(and looks like a lot of work to implement).

Testcase for inline script:
http://www.croczilla.com/~alex/svg-inline-script.svg
Testcase for external script:
http://www.croczilla.com/~alex/svg-external-script.svg

Comment 5

[Hixie (not reading bugmail)]

Just out of interest, how do you cope with cases like:

   <svg:script>
     text node
     <!-- comment node -->
     <?pi node?>
     <![CDATA[ node ]]>
     <element-node/>
     <nested> <elements> with text nodes </elements> </nested>
   </svg:script>

...? (I'm trying to work out how to say it should work if I ever get around to 
having to mention these cases in a spec.)

Comment 6

[Alex Fritze]

All the text nodes below the <script> element are being concatenated and set as
a single big textnode on the <script> element. Both direct and indirect text
nodes get concatenated. Elements themselves (other than textnodes) are ignored.
This looks like the sensible thing to do, but I'm not sure whether it is
correct. I don't really understand how '#PCDATA' is supposed to behave (SVG DTD:
<!ELEMENT script (#PCDATA) > ).

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's supposed to behave by throwing a validation error in any validating parser
on the markup Ian described.  We're not a validating parser, though.  So for us,
it makes sense to just loop over textnode kids of the textarea (direct kids) and
be done with it.

Comment 8

[Alex Fritze]

(In reply to comment #7)
> It's supposed to behave by throwing a validation error in any validating parser
> on the markup Ian described.  
I thought #PCDATA stands for *any* mixed content, so as far as a DTD-aware
parser is concerned, it should let anything pass, no?

Comment 9

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

Comment on attachment 143604 [details] [diff] [review]
patch to implement <svg:script>

You should probably have someone 'cvs copy' nsIScriptElement.h to it's new
destination so that we don't loose cvs-history on it.

Also, I'd really like to see a common baseclass for nsHTMLScriptElement and
nsSVGScriptElement.

>Index: content/base/src/nsScriptLoader.cpp
>@@ -501,12 +493,8 @@
>     request->mIsInline = PR_TRUE;
>     request->mURI = mDocument->GetDocumentURI();
> 
>-    nsCOMPtr<nsIScriptElement> scriptElement(do_QueryInterface(aElement));
>-    if (scriptElement) {
>-      PRUint32 lineNumber;
>-      scriptElement->GetLineNumber(&lineNumber);
>-      request->mLineNo = lineNumber;
>-    }
>+    PRUint32 lineNumber = aElement->GetScriptLineNumber();
>+    request->mLineNo = lineNumber;

Just do |request->mLineNo = aElement->GetScriptLineNumber();|

>Index: content/html/content/src/nsHTMLScriptElement.cpp
>+nsHTMLScriptElement::GetScriptURI()
>+{
>+  nsIURI *uri = nsnull;
>+  nsAutoString src;
>+  GetSrc(src);
>+  if (!src.IsEmpty()) {
>+    nsCOMPtr<nsIURI> baseURI = GetBaseURI();    
>+    NS_NewURI(&uri, src, nsnull, baseURI);
>+  }
>+  return uri;

You actually don't have to resolve against GetBaseURI(). GetSrc will do that
for you.

>Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp

You need to modify txMozillaXMLOutput::startElement so that it sets
mDontAddCurrent for all eSCRIPTs like you did in the XMLContentSink. (This is
currently handled by startHTMLElement)

Comment 10

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

oh, and you should probably have some code to support the 'contentScriptType'
attribute on the <svg:svg> element.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 143604 [details] [diff] [review]
patch to implement <svg:script>

This patch looks good in general, but as discussed on IRC, there's one thing I
don't like here:

- In content/base/public/nsIContent.h

+    /** svg elements */
+    eSVG		  = 0x00000040,
+    /** script elements */
+    eSCRIPT		  = 0x00000080

I'm fine with adding eSVG here, I think that's a good idea, but I don't think
there's enough of a need for eSCRIPT here. The only place where this would
really be used is in the content sink, and the reason there is that the code we
now have is kinda messed up, it needs to know if an element is script or not,
and that's IMO not good, and should be fixed over time. Once that's fixed,
there's no longer a need for this in nsIContent, so I don't think we should add
it at this point, in stead, we can look at the tag and the namespace of the
node (i.e. check the nodeinfo of the node...).

Comment 12

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

Alex: when you update this patch, note that ReplaceChildAt is no more.

Comment 13

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

Comment on attachment 143604 [details] [diff] [review]
patch to implement <svg:script>

marking r- to get this off my radar

Comment 14

[tor]

Attachment: update Alex's patch to tip

Comment 15

[tor]

Attachment: ... and remember the new files

Comment 16

[tor]

Attachment: handle most review comments

Takes into account most review comments.  Two exceptions:

  * common baseclass for ns{SVG,HTML}ScriptElement - two classes that
	are fairly small and don't share much code.  Seems a bit of
	overkill to add another class.

  * contenetScriptType - as I read the spec, this is only for the
	strings set as event attributes, not for <svg:script> for
	which type is a required attribute.

Comment 17

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

I still think it would be better with a common baseclass. While there's not much
code there the code is somewhat complex and nonobvious. Especially the
MaybeProcessScript function has had to have a few iterations before it worked as
we wanted to in all aspects, the last of which was very recent (april 9th).

I could be convinced of otherwise if you really don't want to though...

Comment 18

[tor]

Attachment: update previous patch to tip

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 151906 [details] [diff] [review]
update previous patch to tip

+PRBool
+nsHTMLScriptElement::IsContentOfType(PRUint32 aFlags) const
+{
+  return !(aFlags & ~(eELEMENT | eHTML));
+}

This looks identical to the inherited IsContentOfType(). Why override and
duplicate this code here?

- In nsSVGScriptElement::GetScriptText():

Any reason not to replace all the code in this method with a QI to nsIDOM3Node
and a call to nsIDOM3Node::GetTextContent()? Or if you don't want to do that,
move nsGenericHTMLElement::GetContentsAsText() to nsGenericElement (and
deCOMtaminate the method while you're at it) and call it from here too.

+PRBool
+nsSVGScriptElement::IsContentOfType(PRUint32 aFlags) const
+{
+  return !(aFlags & ~(eELEMENT | eSVG));
+}

Same here, isn't this same code inherited?

With that, sr=jst

Comment 20

[tor]

Attachment: update to tip again, apply jst's suggestions

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 152119 [details] [diff] [review]
update to tip again, apply jst's suggestions

- In nsGenericElement::GetContentsAsText():

+  aText.Truncate();
+  PRInt32 children = GetChildCount();
+  
+  nsCOMPtr<nsIDOMText> tc;
+  nsAutoString textData;
+
+  PRInt32 i;
+  for (i = 0; i < children; ++i) {
+    tc = do_QueryInterface(GetChildAt(i));
+    if (tc) {
+      if (aText.IsEmpty()) {
+	 tc->GetData(aText);
+      } else {
+	 tc->GetData(textData);
+	 aText.Append(textData);
+      }

I just realized that if you'd change nsTextFragment's AppendTo() to take an
nsAString  you could eliminate the nsAutoString and replace the empty check etc
in that loop with this:

  tc->Text()->AppendTo(aText);

I'll attach a patch with the necessary nsTextFragment changes. Feel free to
request a separate bug for this if you don't want to roll that in with this
change.

Comment 22

[Johnny Stenback (:jst)]

Attachment: Afore mentioned nsTextFragment changes.

Comment 23

[Johnny Stenback (:jst)]

Attachment: Afore mentioned nsTextFragment changes.

Duh, forgot to update the comment...

Comment 24

[tor]

Thanks, I think that sort of cleanup should probably be seperated from
this change.  I've filed bug 249572.

Comment 25

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

Comment on attachment 152119 [details] [diff] [review]
update to tip again, apply jst's suggestions

Since we're getting codeduplication between nsHTMLScriptElement and
nsSVGScriptElement, please sprinkle some "this code is duplicated in ...,
change there too if you make changes here"-comments in there.


Do we really need the eSVG enum? I don't really mind it, but isn't
|cont->IsContentOfType(eSVG)| always equal to |cont->NamespaceID() ==
kNameSpaceID_SVG|?

>Index: content/base/public/nsIScriptElement.h

Generate a new UUID for this interface since it's changed.

>Index: content/base/src/nsGenericElement.cpp
>+nsGenericElement::GetContentsAsText(nsAString& aText)

I think this would be more usefull as

nsGenericElement::AppendContentsAsText(PRBool aDeep, nsAString& aText)

But that can defenetly be changed in another bug (such as bug 249572).

>Index: content/base/src/nsScriptLoader.cpp
>@@ -386,24 +387,27 @@ nsScriptLoader::ProcessScriptElement(nsI
...
>-  // Check the language attribute first, so type can trump language.
>-  aElement->GetAttribute(NS_LITERAL_STRING("language"), language);
>-  if (!language.IsEmpty()) {
>-    isJavaScript = nsParserUtils::IsJavaScriptLanguage(language, &jsVersionString);
>+  nsCOMPtr<nsIDOMHTMLScriptElement> htmlScriptElement = do_QueryInterface(aElement);
>+  if (htmlScriptElement) {
>+    // Check the language attribute first, so type can trump language.
>+    htmlScriptElement->GetAttribute(NS_LITERAL_STRING("language"), language);
>+    if (!language.IsEmpty()) {
>+      isJavaScript = nsParserUtils::IsJavaScriptLanguage(language, &jsVersionString);
>+    }

Please add a comment as to why this is only done for html-script-elements so
that noone has to wonder in the future.

Hmm.. on second thought, could this maybe be integrated into the GetScriptType?
No biggie if it's a lot of work.

>Index: content/html/content/src/nsHTMLScriptElement.cpp
...
>+nsHTMLScriptElement::GetScriptURI()
>+{
>+  nsIURI *uri = nsnull;
>+  nsAutoString src;
>+  GetSrc(src);
>+  if (!src.IsEmpty()) {
>+    nsCOMPtr<nsIURI> baseURI = GetBaseURI();    
>+    NS_NewURI(&uri, src, nsnull, baseURI);
>+  }

No need to resolve against the baseURI here, GetSrc has already done that.

>Index: content/xml/document/src/nsXMLContentSink.cpp
...
>+  if (aNodeInfo->Equals(nsHTMLAtoms::script, kNameSpaceID_XHTML)
>+#ifdef MOZ_SVG
>+      || aNodeInfo->Equals(nsSVGAtoms::script, kNameSpaceID_SVG)
>+#endif
>+    ) {
>+    // Don't append the content to the tree until we're all
>+    // done collecting its contents
>+    mConstrainSize = PR_FALSE;
>+    mScriptLineNo = aLineNumber;
>+    *aAppendContent = PR_FALSE;
>+  }
>+

You've forgot to remove the similar (but not svg-aware) version of this code a
bit further down. So it appears twice now.

>Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp
>@@ -392,16 +392,20 @@ void txMozillaXMLOutput::startElement(co
>     }
> 
>     nsresult rv = mTableStateStack.push(NS_INT32_TO_PTR(mTableState));
>     if (NS_FAILED(rv)) {
>         return;
>     }
>     mTableState = NORMAL;
> 
>+    /* scrictly speaking we should be checking againast the HTML and SVG atoms, but
>+       since this ends up being a string compare we're ok for now */
>+    mDontAddCurrent = txHTMLAtoms::script->Equals(aName);

This is wrong. You don't want to do this for all elements named "script". I
would leave the old code in startHTMLElement and insteda insert something like
this:

if (aNsID == kNameSpaceID_XHTML) {
    startHTMLElement(element, PR_TRUE);
}
else if (aNsID == kNameSpaceID_SVG && txHTMLAtoms::script->Equals(aName) {
    mDontAddCurrent = PR_TRUE;
}

Also, i'm not sure I understand the comment on html vs. svg atoms. There is
only one atom containing the string 'script', that's the point of atoms.


Then there is the issue of the |mNotifier->AddScriptElement| call which
currently lives in |endHTMLElement|. endHTMLElement is only called for
(x)html-elements which means that svg-script-elements won't end up being added
to the notifier.

The best way to fix this would probably be to just add code on line 266 after
the call to endHTMLElement that checks explicitly for svg-script-elements and
add them there.

Comment 26

[tor]

(In reply to comment #25)
> (From update of attachment 152119 [details] [diff] [review])
> Since we're getting codeduplication between nsHTMLScriptElement and
> nsSVGScriptElement, please sprinkle some "this code is duplicated in ...,
> change there too if you make changes here"-comments in there.

Ok, added a few comments of that type to the nontrivial common methods.

> Do we really need the eSVG enum? I don't really mind it, but isn't
> |cont->IsContentOfType(eSVG)| always equal to |cont->NamespaceID() ==
> kNameSpaceID_SVG|?

We could probably get away without it, but IsContentOfType is a commonly
used method and we should probably be consistent with the rest of the code.

> >Index: content/base/src/nsGenericElement.cpp
> >+nsGenericElement::GetContentsAsText(nsAString& aText)
> 
> I think this would be more usefull as
> 
> nsGenericElement::AppendContentsAsText(PRBool aDeep, nsAString& aText)
> 
> But that can defenetly be changed in another bug (such as bug 249572).

I think that should be left for another bug.

> >Index: content/base/src/nsScriptLoader.cpp
> >@@ -386,24 +387,27 @@ nsScriptLoader::ProcessScriptElement(nsI
> ...
> >-  // Check the language attribute first, so type can trump language.
>
> Please add a comment as to why this is only done for html-script-elements so
> that noone has to wonder in the future.

> Hmm.. on second thought, could this maybe be integrated into the GetScriptType?
> No biggie if it's a lot of work.

Wouldn't be that big a deal, the HTML GetScriptType would just need to
return a correct mimetype string if the language attribute was there.
Added a comment for now, changing GetScriptType seems like it could
come later.

> >Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp

Waved a dead chicken over transformiix.

Comment 27

[tor]

Attachment: apply sicking's comments

Comment 28

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

Comment on attachment 152438 [details] [diff] [review]
apply sicking's comments

>Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp
>@@ -259,16 +259,32 @@ void txMozillaXMLOutput::endElement(cons
>     // Handle html-elements
>     if ((mOutputFormat.mMethod == eHTMLOutput && aNsID == kNameSpaceID_None) ||
>         aNsID == kNameSpaceID_XHTML) {
>         nsCOMPtr<nsIDOMElement> element = do_QueryInterface(mCurrentNode);
>         NS_ASSERTION(element, "endElement'ing non-element");
>         endHTMLElement(element);
>     }
> 
>+    // Handle svg script elements
>+    if ((mOutputFormat.mMethod == eHTMLOutput && aNsID == kNameSpaceID_None) ||
>+        aNsID == kNameSpaceID_SVG) {

This should just be |if (aNsID == kNameSpaceID_SVG) {|. HTML is handled by
endHTMLElement.

>+        nsCOMPtr<nsIContent> content = do_QueryInterface(mCurrentNode);
>+        NS_ASSERTION(content, "Can't QI to nsIContent");
>+        nsIAtom *atom = content->Tag();
>+        
>+        if (mNotifier && atom == txHTMLAtoms::script) {

And this could just be |if (txHTMLAtoms::script->Equals(aName))|. That way you
don't need the QI and can merge the to ifs.

With that, r=me

Comment 29

[tor]

Attachment: more sicking changes

Comment 30

[tor]

Checked in.  Thanks to Alex for doing all the hard original work and
letting me do the final polish.