svg:script not working since 343730 checkin

RESOLVED FIXED

Status

()

Core
SVG
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: tor, Assigned: sicking)

Tracking

({regression})

Trunk
x86
Linux
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
Backing out the script changes in bug 343730 makes this SVG app work again.
(Reporter)

Comment 1

12 years ago
Created attachment 244760 [details]
simple testcase - mouseover rect for alert
(Reporter)

Comment 2

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

Updated

12 years ago
Keywords: regression
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.
Created attachment 244965 [details] [diff] [review]
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.
Assignee: general → bugmail
Status: NEW → ASSIGNED
Attachment #244965 - Flags: superreview?(bzbarsky)
Attachment #244965 - Flags: review?(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()
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 on attachment 244965 [details] [diff] [review]
Patch to fix

Ah, ok.  r+sr=bzbarsky with the spelling fix.
Attachment #244965 - Flags: superreview?(bzbarsky)
Attachment #244965 - Flags: superreview+
Attachment #244965 - Flags: review?(bzbarsky)
Attachment #244965 - Flags: review+

Updated

12 years ago
Flags: in-testsuite?
Comment on attachment 244965 [details] [diff] [review]
Patch to fix

> }
 
May be here should be PRBool type?
>+nsHTMLScriptElement::HasScriptContent()
>+{
>+  return
Attachment #244965 - Flags: review-
> May be here should be PRBool type?
> >+nsHTMLScriptElement::HasScriptContent()

Same for nsSVGScriptElement.cpp
Checked in about a day ago with the return-types fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Oh. Somehow I missed the mail about this getting fixed (seem to be doing that a lot). Thanks guys!
You need to log in before you can comment on or make changes to this bug.