740811 - SVGSVGElement doesn't look into Element.prototype

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arian Stolwijk]

Attachment: 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.

Comment 1

[Arian Stolwijk]

This bug breaks MooTools Element methods on <svg> elements:
https://github.com/mootools/mootools-core/issues/2331

Comment 2

[Daniel Buchner [:dbuc]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Arian Stolwijk]

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

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Probably a regression from bug 82235.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Probably a regression from bug 589640.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Make SVGElement.prototype be Element.prototype again.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Peter Van der Beken [:peterv]]

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 13

[Alex Keybl [:akeybl]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9898675c51

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ba9898675c51

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me that we should put this regression fix on aurora/beta as well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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:

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 613305 [details] [diff] [review]
Make SVGElement.prototype be Element.prototype again.

This made the cutover.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

[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.

Comment 20

[Alex Keybl [:akeybl]]

Can we move ahead with landing the approved patch on FF13?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er, yes.  I didn't set the flag, so didn't get the notification mail....

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Ioana (away)]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 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

[Ioana (away)]

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

Comment 26

[Ioana (away)]

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)

Comment 27

[Boris Zbarsky [:bzbarsky]]

No point for developer docs for this.  It might have been useful in the Fx14 docs, but that ship has long sailed.