SVGSVGElement doesn't look into Element.prototype

RESOLVED FIXED in Firefox 13

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Arian Stolwijk, Assigned: bz)

Tracking

unspecified
mozilla14
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 wontfix, firefox13+ verified, firefox14 verified)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 610885 [details]
Element.svg.html

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.142 Safari/535.19

Steps to reproduce:

I tried this code:

Element.prototype.test = 'test';
var svg = document.getElementsByTagName('svg')[0];
console.assert(svg.test == 'test');



Actual results:

The assertion failed


Expected results:

the assertion shouldn't have failed.
(Reporter)

Comment 1

5 years ago
This bug breaks MooTools Element methods on <svg> elements:
https://github.com/mootools/mootools-core/issues/2331
Hey guys, any reason why SVG elements are failing to pick up their prototypes as Arian indicates? If this is not expected behavior, it could affect the assumptions and element-centric code of many developers, please advise.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #610885 - Attachment mime type: text/plain → text/html
Looks like SVGElement.prototype has Object.prototype as its proto instead of Element.prototype.  I'll look into why on Monday if no one else has before then.

If this is a regression, someone finding the regression range would be a big help.
(Reporter)

Comment 4

5 years ago
Maybe interesting to note is that:

Element.prototype.test = 'yo'
document.createElementNS('http://www.w3.org/2000/svg', 'svg').test 
// → undefined in FF, but 'yo' in chrome
document.createElement('svg').test
// → 'yo' in both FF and Chrome
(In reply to Arian Stolwijk from comment #4)
> Maybe interesting to note is that:
> 
> Element.prototype.test = 'yo'
> document.createElementNS('http://www.w3.org/2000/svg', 'svg').test 
> // → undefined in FF, but 'yo' in chrome
> document.createElement('svg').test
> // → 'yo' in both FF and Chrome

The latter creates an HTMLElement.
So the issue is that

   DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(SVGUnknownElement, nsIDOMSVGElement)

means that when we're setting up the proto for SVGElement we end up using the interfaceinfo for nsIDOMSVGElement for the classname.  So we recursively look up "SVGElement" on the window, look for .prototype on it, get nothing (since we're still setting it up) and end up using Object.prototype as the proto.

Peter, is the NO_CLASS_IF part here correct?  Why do we not want to use the parent in the NO_CLASS_IF case, exactly?
Probably a regression from bug 82235.
Blocks: 82235
Probably a regression from bug 589640.
Blocks: 589640
No longer blocks: 82235
The NO_CLASS_IF bit was added by me as a test bustage fix for bug 589640, but I didn't write down what was broken, sadly.

I tried pushing a backout of the NO_CLASS_IF bit to try, and it seems to pass tests...  It also fixes this bug as far as I can tell.
Created attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.
Attachment #613305 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Version: 11 Branch → unspecified
Attachment #613305 - Flags: review?(peterv) → review+
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

Requesting approval for m-c.  I think this should be a pretty safe fix, and it's a basic web compat issue that we should fix.
Attachment #613305 - Flags: approval-mozilla-central?
Whiteboard: [need review] → [need landing]
Just for posterity, the reason removing NO_CLASS_IF is ok is that SVGUnknownElement is the DOMCI for SVGElement, so the primary interface is the class interface (nsIDOMSVGElement).
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

[Triage Comment]
Approved for mozilla-central - this is a web compatibility bug, and we don't expect it to have a major effect on any of the main FN use cases.
Attachment #613305 - Flags: approval-mozilla-central? → approval-mozilla-central+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9898675c51
Flags: in-testsuite+
Whiteboard: [need landing]
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/ba9898675c51
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
It seems to me that we should put this regression fix on aurora/beta as well.
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

Yeah, that's not a bad idea.  I hadn't wanted to rush it into 12 before.

[Approval Request Comment]
Regression caused by (bug #): 589640 
User impact if declined: Some sites' JS doesn't work right.
Testing completed (on m-c, etc.): Checked in on m-c, passing tests.
Risk to taking this patch (and alternatives if risky): Slight risk that something
   could go wrong, but I'm not really sure what it would be.  Never bet against
   some website _somewhere_ breaking, though.  ;)
String changes made by this patch:
Attachment #613305 - Flags: approval-mozilla-beta?
Attachment #613305 - Flags: approval-mozilla-aurora?
Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

This made the cutover.
Attachment #613305 - Flags: approval-mozilla-aurora?
status-firefox12: --- → wontfix
tracking-firefox13: --- → ?
Attachment #613305 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[Triage Comment]
Approved for beta and updating the status flags to show it's not yet on beta (but made the cut to aurora) - please update flag once this has landed.
status-firefox13: --- → affected
status-firefox14: --- → fixed
tracking-firefox13: ? → ---
tracking-firefox13: --- → +
Can we move ahead with landing the approved patch on FF13?
Er, yes.  I didn't set the flag, so didn't get the notification mail....
Hm.  I _did_ set the flag.  I wonder what happened to that mail.

In any case, http://hg.mozilla.org/releases/mozilla-beta/rev/05cfe44826dd
status-firefox13: affected → fixed
Whiteboard: [qa+]

Comment 23

5 years ago
The automated test for this bug passed on all OSs on Firefox 13.0 beta:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12163160&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12163930&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12159488&full=1&branch=mozilla-beta

I have also tried to verify this fix manually and the assertion from comment 0 doesn't fail on Firefox 13.0b6. The problem is that it also doesn't fail on Firefox 12.0 on my workstation. 

Could someone please let me know how to reproduce this issue?
Until now I have tried loading the Element.svg.html attachment in Firefox 12.0 and looking in the Error and Web consoles for the assertion failure. Also tried with replacing the corresponding lines from that attachment with 

Element.prototype.test = 'test';
var svg = document.getElementsByTagName('svg')[0];
console.assert(svg.test == 'test');

and looking in the Error and Web consoles for the assertion failures.
> The problem is that it also doesn't fail on Firefox 12.0 on my workstation. 

That's because there is no console.assert method.

> Could someone please let me know how to reproduce this issue?

Replace console.assert by alert() and see what boolean gets alerted.

Comment 25

5 years ago
Boris, thank you for you suggestion. I didn't get any error about using an inexistent method, so I didn't know for sure what the problem was.

Verified as fixed with the guidelines from comment 0 and comment 24:
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120528154913
status-firefox13: fixed → verified

Comment 26

5 years ago
The automated test for this bug passed on all OSs on Firefox 14.0 beta too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407667&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407415&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12410453&full=1&branch=mozilla-beta

Also verified as fixed with the guidelines from comments 0 and 24 on:
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0 (20120605113340)

Updated

5 years ago
status-firefox14: fixed → verified
No point for developer docs for this.  It might have been useful in the Fx14 docs, but that ship has long sailed.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.