Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla10
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 560844 [details] [diff] [review]
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.
Attachment #560844 - Flags: review?(Olli.Pettay)

Comment 1

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

Updated

6 years ago
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
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

6 years ago
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/sicking@mozilla.com-cda7df7c1a15

Comment 4

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

6 years ago
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.
Attachment #560844 - Flags: review?(Olli.Pettay) → review+
Created attachment 562689 [details] [diff] [review]
Add a warning

Olli, feel free to land this if it passes review.
Assignee: nobody → jonas
Attachment #562689 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Attachment #562689 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [inbound]
Attachment #562689 - Flags: checkin+

Updated

6 years ago
Whiteboard: [inbound] → [leave open for remaining patch]
https://hg.mozilla.org/mozilla-central/rev/ef9284803c47

Comment 8

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

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

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

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

Comment 14

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

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

Comment 16

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

Comment 17

6 years ago
Sorry, 11.9.6 step 7
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

6 years ago
> note in the current W3C DOM
Isn't that "language-neutral"?
Checked in to inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c201d75331
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/82c201d75331

taking a brief look at this bug's history looks like all patches landed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for remaining patch]
Target Milestone: --- → mozilla10

Comment 22

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

Thanks for not supporting standards and breaking the web!
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.
DOM4 has been updated accordingly.
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.
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
Keywords: dev-doc-needed → dev-doc-complete
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

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

6 years ago
Disregard previous message re FF 8. My bad.
(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.

Updated

6 years ago
Depends on: 723415

Updated

6 years ago
Depends on: 723645
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?
Please file new bugs for new issues ;)

Updated

a year ago
Blocks: 1256299
You need to log in before you can comment on or make changes to this bug.