Script feature support should depend on whether javascript is enabled

VERIFIED FIXED in mozilla6

Status

()

Core
SVG
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 528010 [details] [diff] [review]
patch

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

Updated

6 years ago
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
(Assignee)

Comment 3

6 years ago
Created attachment 528808 [details] [diff] [review]
return false for SVG images too
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+
(Assignee)

Comment 5

6 years ago
(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!
(Assignee)

Comment 7

6 years ago
Created attachment 529073 [details] [diff] [review]
hg changeset patch with final review comments addressed
Attachment #528808 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → longsonr
http://hg.mozilla.org/mozilla-central/rev/00f6e702f095
Status: NEW → RESOLVED
Last Resolved: 6 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 → ---
(Assignee)

Comment 10

6 years ago
Created attachment 529273 [details] [diff] [review]
new attempt at unreliable reftest
Attachment #529073 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=3ccc4d8ab375
(Assignee)

Comment 12

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 13

6 years ago
 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
(Assignee)

Comment 14

6 years ago
(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

6 years ago
(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.