652442 - Script feature support should depend on whether javascript is enabled

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

http://www.w3.org/TR/SVG11/feature#Script should only resolve if javascript is enabled.

Comment 1

[Daniel Holbert [:dholbert]]

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

Hm... Is "javascript.enabled" really the way we want to check for javascript being enabled?

In e.g. SVG-as-an-image (and external resource documents, too, I think), javascript is disabled as far as the SVG document is concerned, but the "javascript.enabled" pref won't reveal that.

If an author is taking the trouble to query whether scripting is enabled, they'd probably want their query to fail in those cases, right?

Comment 2

[Daniel Holbert [:dholbert]]

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

Ah, nevermind -- it looks like nsSVGFeatures' methods are all static, so I guess they can't tell what sort of document they're being called for.

So, if we want to have special handling for the image/resource doc case, we'd probably need to do that up two levels, in the non-static method nsGenericElement:::IsSupported.  And that's probably too generic of a place to have special-handling code for SVG...

Do you think it'd be appropriate to fail this feature-check for image/resource docs?  (If so, perhaps you could file a followup on that?)

r=dholbert on this, either way.

Comment 3

[Robert Longson [:longsonr]]

Attachment: return false for SVG images too

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 528808 [details] [diff] [review]
return false for SVG images too

>+nsSVGFeatures::HaveFeature(nsISupports* aObject, const nsAString& aFeature)
> {
>-#define SVG_SUPPORTED_FEATURE(str) if (aFeature.Equals(NS_LITERAL_STRING(str).get())) return PR_TRUE;
>+  if (aFeature.EqualsLiteral("http://www.w3.org/TR/SVG11/feature#Script")) {
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(aObject));
>+    if (content) {
>+      nsIDocument *doc = content->GetCurrentDoc();
>+      if (doc && doc->IsBeingUsedAsImage()) {
>+        // no scripting in SVG images
>+        return PR_FALSE;

This wants:
s/IsBeingUsedAsImage/IsResourceDoc/
s/in SVG images/in SVG images and external resource documents/

(Assuming we want to consider scripting unsupported in external resource documents, which I imagine we do)
That'll need a testcase using an external resource doc, too, then
  (e.g. <use xlink:href="someOtherFile.svg#something">)

>+++ b/content/svg/content/src/nsSVGFeatures.h
>    * Check whether we support the given feature string.
>    *
>    * @param aFeature one of the feature strings specified at
>    *    http://www.w3.org/TR/SVG11/feature.html
>    */
>   static PRBool
>-  HaveFeature(const nsAString& aFeature);
>+  HaveFeature(nsISupports* aObject, const nsAString& aFeature);

This documentation needs updating with the new @param.

Also, why is |aObject| nsISupports rather than just nsIContent?  If it's always going to be nsIContent, can we just make it that? (then we won't need the QI dance inside of the HaveFeature impl)

> private:
>   /**
>    * Check whether we support the given list of feature strings.
>    *
>    * @param aFeatures a whitespace separated list containing one or more of the
>    *   feature strings specified at http://www.w3.org/TR/SVG11/feature.html
>    */
>   static PRBool
>-  HaveFeatures(const nsSubstring& aFeatures);
>+  HaveFeatures(nsISupports* aObject, const nsSubstring& aFeatures);

This documentation needs updating with the new @param, too.


>+++ b/layout/reftests/svg/as-image/script100x100.svg
[...]
>+++ b/layout/reftests/svg/as-image/svg-image-script-1.svg

Could you use a different failure-color in script100x100.svg vs. in svg-image-script-1.svg?  Right now they both use red, so if this test failed, we wouldn't be able to figure out what part had failed. (the image rendering vs. the script feature-detection)

r=dholbert with the above addressed.

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> Also, why is |aObject| nsISupports rather than just nsIContent?  If it's always
> going to be nsIContent, can we just make it that? (then we won't need the QI
> dance inside of the HaveFeature impl)

HaveFeature can be called with things other than nodes e.g. nsDOMAttribute or  nsDOMImplementation. These aren't derived from nsIContent.

I'll do the other comments.

Comment 6

[Daniel Holbert [:dholbert]]

Ah, right. I figured as much, but I didn't notice any non-nsIContent-passing callers when I first read the patch.

Looking back now, I see that  nsGenericElement::InternalIsSupported will pass in aObject which is an nsISupports.  Makes sense.  Thanks!

Comment 7

[Robert Longson [:longsonr]]

Attachment: hg changeset patch with final review comments addressed

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/00f6e702f095

Comment 9

[Dão Gottwald [:dao]]

Backed out, since svg-image-script-2.svg failed intermittently.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1304086959.1304088668.21248.gz

http://hg.mozilla.org/mozilla-central/rev/96ea841d7f4a

Comment 10

[Robert Longson [:longsonr]]

Attachment: new attempt at unreliable reftest

Comment 11

[Robert Longson [:longsonr]]

http://tbpl.mozilla.org/?tree=Try&rev=3ccc4d8ab375

Comment 12

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/1be13b4c6d80
Thanks for trying this before Dao,my apologies for the intermittent problem with the previous reftest

Comment 13

[George Carstoiu]

 Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Ubuntu 11.04, Win7 x86, WinXP and Mac OS X 10.6 using the following steps:
1. Go to  http://www.w3.org/TR/SVG11/images/script/script01.svg
2. Click on red circle
3. Disable javascript
4. Reload page and click again on the red circle

Action performed only if js is enabled.

Setting status Verified Fixed.

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
>  Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
> 

That's not what this fix is about. Here's how you test this fix though.

1. Enable javascript
2. Goto http://www.codedread.com/svgtest.svg
3. Check that http://www.w3.org/TR/SVG11/feature#Script displays in green (it's about 3/4 of the way down the list)
4. Disable javascript
5. Refresh the page
6. Check that http://www.w3.org/TR/SVG11/feature#Script displays in red now.

Comment 15

[George Carstoiu]

(In reply to comment #14)
> (In reply to comment #13)
> >  Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
> > 
> 
> That's not what this fix is about. Here's how you test this fix though.
> 
> 1. Enable javascript
> 2. Goto http://www.codedread.com/svgtest.svg
> 3. Check that http://www.w3.org/TR/SVG11/feature#Script displays in green
> (it's about 3/4 of the way down the list)
> 4. Disable javascript
> 5. Refresh the page
> 6. Check that http://www.w3.org/TR/SVG11/feature#Script displays in red now.

Thanks for the heads-up.

Re-checked using steps from Comment 14 and everything seems in order.