687400 - Kill isSameNode

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This function simply implements the '==' operator. For us it's just useless code we're carrying around, and for JS authors it's directly harmful as it's slower than simply using '=='.

Patch attached which removes it.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Should we first warn about removal? Google code search shows that isSameNode is used
occasionally. (Although at least in some cases the code is absolutely bizarre; something like
if (node.isSameNode ? node.isSameNode(otherNode) : node == otherNode) ... )

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I was thinking about that too. It'd be easy to warn if we're fine with removing the custom quickstubs that we currently have, and then put the removal in next release.

Let me know if you prefer that?

Comment 3

[Mozilla RelEng Bot]

Try run for cda7df7c1a15 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cda7df7c1a15
Results (out of 27 total builds):
    success: 25
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cda7df7c1a15

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> I was thinking about that too. It'd be easy to warn if we're fine with
> removing the custom quickstubs that we currently have, and then put the
> removal in next release.
What has quickstubs to do with the warning?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 560844 [details] [diff] [review]
Patch to fix

But we really must warn about the removal and have the warning at least during 
one release.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Add a warning

Olli, feel free to land this if it passes review.

Comment 7

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

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

Comment 8

[John A. Bilicki III]

Is this patch a request to remove the method outright or to rewire what it does internally? If it's an outright removal request what is === compatibility in older browsers contrasting isSameNode? I am finding this invaluable in helping me iron out a Gecko range/selection bug.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You can always use === to test if two nodes are the same node.

So |a.isSameNode(b)| will produce the same result as |a === b| in all versions of gecko. Well.. except that some older versions of gecko doesn't support the former. So the latter is strictly an improvement. And it executes faster too.

Comment 10

[John A. Bilicki III]

Jonas, my question is if this bug is a request to remove the method outright or to rewire what it does internally?

If you've requested an outright removal of the method then can you please point out which version of the W3C DOM specification where it has been deprecated?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's a request to outright remove it as an experiment to see if it can be done without "breaking the web". The spec editor is waiting for feedback from that experiment before removing it. Please search the public-webapps archives for "isSameNode".

Comment 12

[John A. Bilicki III]

Then you have determined that the === operator is EXPLICITLY intended to work with comparing nodes? If so please reference where in which specification and version it is explicitly stated.

Comment 13

[j.j.]

http://www.ecma-international.org/publications/standards/Ecma-262.htm
Section 11.9.4

Comment 14

[John A. Bilicki III]

j.j., please clarify which point and how it's wording applies DIRECTLY to context of  matching nodes in the manner isSameNode works.

Jonas, Firefox does not exist for you to personally experiment with to see if you can break the web. If you are concerned about performance on other people's websites then your bug report should have been limited to suggesting a warning be made in the console in strict mode at best.

Comment 15

[j.j.]

[shrug]
7. Return true if x and y refer to the same object. Otherwise, return false.
Not enough?

Comment 16

[John A. Bilicki III]

Incorrect reference, that is under 11.9.3 (==), NOT 11.9.4 (===).

Comment 17

[j.j.]

Sorry, 11.9.6 step 7

Comment 18

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

John, if you need an EXPLICIT note in the current W3C DOM specification, please let me know and I'll make it happen.

Comment 19

[j.j.]

> note in the current W3C DOM
Isn't that "language-neutral"?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c201d75331

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/82c201d75331

taking a brief look at this bug's history looks like all patches landed.

Comment 22

[John A. Bilicki III]

The removal of this method is now breaking scripts on my site.

Thanks for not supporting standards and breaking the web!

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, this was done to better support cross browser behaviour since not all browsers support this function. It was also done to help JS authors as == is faster than isSameNode.

Also,isSameNode is likely going to to be removed from the specs, so there's only advantages to stop using it.

Comment 24

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

DOM4 has been updated accordingly.

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

Most add-on developers (and reviewers) test their add-ons looking for error messages, not warnings. Deprecation messages that we know we'll follow through should be errors IMO, to make them easier to spot. 

I see several add-ons on AMO that still use isSameNode, so I think we should extend the deprecation period with an error instead.

Comment 26

[Jean-Yves Perrier [:teoli]]

I've updated:
https://developer.mozilla.org/en/Firefox_10_for_developers
https://developer.mozilla.org/en/DOM/Node.isSameNode
and
https://developer.mozilla.org/en/DOM/Node

Comment 27

[Marat Tanalin]

Though I personally usually don't use isSameNode() method (but I _did_ use on server side), I believe that its removal is a wrong step.

DOM levels should be incremental, and if something is (to be) removed from new DOM4, this does not mean that it should be removed from existing implementations. Stable version of a major browser is noway a place for such experiments.

You are free to mark it as obsolete feature, but you should not remove it.

Comment 28

[John Nagle]

I just got the message
"TypeError: elt.isSameNode is not a function" in Firefox 8.
Did someone back-port this "fix"?

(Also, I suspect there may be a bug in comparing nodes with "==" when working through proxy objects in add-ons.  But I haven't confirmed this yet.)

Comment 29

[John Nagle]

Disregard previous message re FF 8. My bad.

Comment 30

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

(In reply to Marat Tanalin | tanalin.com from comment #27)
> Though I personally usually don't use isSameNode() method (but I _did_ use
> on server side), I believe that its removal is a wrong step.
> 
> DOM levels should be incremental, and if something is (to be) removed from
> new DOM4, this does not mean that it should be removed from existing
> implementations. Stable version of a major browser is noway a place for such
> experiments.
> 
> You are free to mark it as obsolete feature, but you should not remove it.

As one of the editors of DOM4, I can say that it is the intention that features that were removed from the specification are also removed from implementations, in order to simplify the platform as a whole. If features are to remain in implementations, they will remain in the specification as well.

Comment 31

[Arkadiusz Michalski (Spirit)]

It's back to the DOM spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27424
https://github.com/whatwg/dom/commit/97f0a958bc1dcb98265d116bafdc80ae1b059823
so now will back to Gecko?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Please file new bugs for new issues ;)