359598 - svg:script not working since 343730 checkin

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Backing out the script changes in bug 343730 makes this SVG app work again.

Comment 1

[tor]

Attachment: simple testcase - mouseover rect for alert

Comment 2

[tor]

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

Comment 3

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

The main problem here is that nsSVGScriptElement doesn't implement DoneAddingChildren. However I also noticed that svg uses xlink:href to point to the url, not src as some of the new code assumes. I've got a patch in the works that takes care of this and some other things.

Comment 4

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

Attachment: Patch to fix

There we two major problems.

1. nsSVGScriptElement didn't override DoneAddingChildren so we never even got
   in to nsScriptElement::MaybeProcessScript

2. A couple of places assumed that the attribute name was src (rather than
   xlink:href for <svg:script>s)

I fixed this and also improved the code that checks if a script element is "empty" (i.e. doesn't link to a script or contain inline script). This finally makes it possible to first give a script a text-child and then set that textchilds .nodeValue without the script executing too early.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244965 [details] [diff] [review]
Patch to fix

>Index: content/base/public/nsContentUtils.h
>+   * NOTE! This method does not descend recursivly into elements.

"recursively"

>Index: content/svg/content/src/nsSVGScriptElement.cpp

>+nsSVGScriptElement::HasScriptContent()
>+{
>+  nsAutoString src;
>+  mHref->GetAnimVal(src);

Why use the animval instead of just HasAttr()

Comment 6

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

It's what GetScriptURI does. Not sure if it's possible to use animation elements to get an animated value without actually setting the attribute.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244965 [details] [diff] [review]
Patch to fix

Ah, ok.  r+sr=bzbarsky with the spelling fix.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 244965 [details] [diff] [review]
Patch to fix

> }
 
May be here should be PRBool type?
>+nsHTMLScriptElement::HasScriptContent()
>+{
>+  return

Comment 9

[Oleg Romashin (:romaxa)]

> May be here should be PRBool type?
> >+nsHTMLScriptElement::HasScriptContent()

Same for nsSVGScriptElement.cpp

Comment 10

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

Checked in about a day ago with the return-types fixed.

Comment 11

[Jonathan Watt [:jwatt]]

Oh. Somehow I missed the mail about this getting fixed (seem to be doing that a lot). Thanks guys!