Script feature support should depend on whether javascript is enabled

VERIFIED FIXED in mozilla6

Status

()

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
http://www.w3.org/TR/SVG11/feature#Script should only resolve if javascript is enabled.
Attachment #528010 - Attachment is patch: true
Attachment #528010 - Flags: review?(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 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.
Attachment #528010 - Flags: review?(dholbert) → review+
Version: unspecified → Trunk
Attachment #528010 - Attachment is obsolete: true
Attachment #528808 - Flags: review?(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.
Attachment #528808 - Flags: review?(dholbert) → review+
(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.
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!
Attachment #528808 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → longsonr
http://hg.mozilla.org/mozilla-central/rev/00f6e702f095
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #529073 - Attachment is obsolete: true
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
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
 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.
Status: RESOLVED → VERIFIED
(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.
(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.
Depends on: 695677
You need to log in before you can comment on or make changes to this bug.