This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 27424 - Node.isSameNode() has non-trivial usage
Summary: Node.isSameNode() has non-trivial usage
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-25 09:26 UTC by Philip Jägenstedt
Modified: 2016-03-16 06:21 UTC (History)
9 users (show)

See Also:


Attachments

Description Philip Jägenstedt 2014-11-25 09:26:11 UTC
https://dom.spec.whatwg.org/#dom-node-issamenode

Chrome use counter data is around 0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/118

The implementation is trivial so there's little to gain from removal.

It looks like Gecko is the only engine to not support it at all, having removed it in 2012:
https://developer.mozilla.org/en-US/docs/Web/API/Node.isSameNode

In IE it behaves as if:
boolean isSameNode(Node other);

In Blink it behaves as if:
boolean isSameNode(optional Node? other = null);
Comment 1 Anne 2014-11-25 09:35:08 UTC
I believe Jonas was in favor of removing this. Given that Gecko succeeded, why can't Blink?
Comment 2 Philip Jägenstedt 2014-11-25 10:15:01 UTC
It could succeed, it just seems like a harmless thing to keep around, and since it's supported everywhere except Gecko brining it back would be the simplest path to agreement between spec and browsers.

Somewhat on topic, is there any data showing that isEqualNode() is required for Web compat while isSameNode() isn't?
Comment 3 Anne 2014-11-25 10:18:20 UTC
Well, isSameNode() is also known as === in JavaScript. isEqualNode() is actually a novel primitive. I think that's why we decided to only try to remove the former.
Comment 4 Philip Jägenstedt 2014-11-25 10:43:33 UTC
OK, let's leave isEqualNode() out of the discussion.

Jonas, how badly do you want to keep isSameNode() out of Gecko?
Comment 5 Boris Zbarsky 2014-11-25 14:02:43 UTC
I think the reasoning here was that having isSameNode was making web authors use it instead of ===, because they were afraid that the test was somehow different.  Which it's not; except it's slower.
Comment 6 Philip Jägenstedt 2014-12-09 19:25:12 UTC
I gave it my best shot, but in the end we decided to neither deprecate nor remove this in Blink, on the grounds that it doesn't warrant taking even a small risk:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/CpIy0Pzcg4E/k0SgkniDDNUJ

So, I'd like to again request that this is revived in the spec. I appreciate that Mozilla took the risk in moving first with this and many other removals, but revival really seems like the fastest way to resolve this for good.
Comment 7 Boris Zbarsky 2014-12-09 19:28:20 UTC
Quite honestly, if Blink is not willing to remove this, just like they're not willing to remove their various webkit-prefixed stuff, that doesn't mean the spec should include it, unless it's actually needed for web compat.

Put another way, if we think the web compat impact of this (of which there is none that anyone knows about) requires it to be put in the spec, we should add the things that have _more_ web compat impact, like various webkit-prefixed stuff, to the spec first.
Comment 8 Philip Jägenstedt 2014-12-09 20:51:17 UTC
Yes, webkit-prefixed and other webkit-only APIs are a bigger problem, I've removed many of them and will try to remove as many more as possible. I don't know what to do about the ones required for Web compat, a Prefixes Standard seems like one way to go.

I don't know if isSameNode() is required for Web compat for non-Gecko browsers, and currently have no way of finding out other than attempting the removal. If isSameNode() remains in the spec, maybe someone working on Safari or IE will have better luck.

If spec'd, I think it should be isSameNode(Node? node), like isEqualNode.
Comment 9 Philip Jägenstedt 2014-12-09 23:15:51 UTC
(In reply to Philip Jägenstedt from comment #8)
> If isSameNode() remains in the spec, maybe someone working on
> Safari or IE will have better luck.

s/in/out of/
Comment 10 Anne 2014-12-10 12:39:37 UTC
It would be interesting to know what Microsoft and Apple are doing with this method. I'm inclined to leave this bug open until either Gecko feels forced to implement this or a new browser (say Servo) feels forced to implement this.
Comment 11 Travis Leithead [MSFT] 2014-12-10 23:34:17 UTC
(In reply to Anne from comment #10)
> It would be interesting to know what Microsoft and Apple are doing with this
> method. I'm inclined to leave this bug open until either Gecko feels forced
> to implement this or a new browser (say Servo) feels forced to implement
> this.

Well, IE is going to leave it in unless it's going to pose a problem (inertia is to not change). If it's ultimately not needed and Chrome can remove it (0.01 is lower than their tolerance limit of 0.03 to pull a feature out as I recall), then I think IE would feel good in removing it too.

Given that, not specing it is fine by me :-)
Comment 12 Philip Jägenstedt 2014-12-10 23:47:53 UTC
I proposed removing it in Blink (and thus Chrome) but despite the 0.01% usage no amount of risk was deemed justified given how trivial this is to support, see comment #6.
Comment 13 Jonas Sicking (Not reading bugmail) 2014-12-11 00:29:23 UTC
FWIW, at mozilla we've thought of removing this function as a performance optimization. I.e. that the fallback code path that sites hopefully provide is likely faster than calling the function.

I.e. we expected code like

if (a.isSameNode ? a.isSameNode(b) : a === b) {
  // a is same as b
}

or

if (!Node.prototype.isSameNode) {
  Node.prototype.isSameNode = function(other) { return this === other; };
}


I don't recall if we made that assumption based on code in the wild or not.
Comment 14 Philip Jägenstedt 2014-12-17 10:35:51 UTC
Ryosuke, do you have anything to say on the prospects of removing isSameNode() in WebKit?
Comment 15 Ryosuke Niwa 2014-12-17 14:13:08 UTC
(In reply to Philip Jägenstedt from comment #14)
> Ryosuke, do you have anything to say on the prospects of removing
> isSameNode() in WebKit?

I don't have a strong opinion either way, and I haven't talked with my colleagues yet but my gut feeling is that we're not gonna remove this function; at least that's what I'm going to advocate if anyone suggests to remove this function in WebKit.

As far as I've searched through GitHub [1], most of code samples I found that uses isSameNode are WebKit/Blink tests. This contradicts the claim that removing isSameNode is a meaningful performance optimization or that many Web authors rely on this function instead of ===.

I can be convinced otherwise with a presence of more data indicating having this function is more harmful than worth the potential compatibility benefit.

[1] https://github.com/search?l=javascript&p=80&q=isSameNode%28&ref=searchresults&type=Code&utf8=✓
Comment 16 Ryosuke Niwa 2014-12-17 14:23:31 UTC
I'll also add that one seemingly non-test code [2] I found to use isSameNode didn't seem to have any fallback.  This indicates that not only is this function usage low but in a few cases where it's used, it's required for compatibility.

[2] https://github.com/satisfeet/quetzal/blob/9b7dd19fa89a2084af3c12b6d32bdfdeba1fc9d4/app/products/views/show/index.js
Comment 17 Philip Jägenstedt 2014-12-17 15:34:15 UTC
Thanks, Ryosuke!

Lacking any enthusiasm for removal from IE or WebKit representatives, I think isSameNode() is here to stay, however useless. Here's my spec:

partial interface Node {
  boolean isSameNode(Node? node);
}

The isSameNode(node) method must return true if node and context object are the same object, and false otherwise.

In Blink the argument is optional, but if the above makes it into the spec I'll fix that.
Comment 18 Anne 2015-08-03 09:24:11 UTC
(In reply to Boris Zbarsky from comment #7)
> Put another way, if we think the web compat impact of this (of which there
> is none that anyone knows about) requires it to be put in the spec, we
> should add the things that have _more_ web compat impact, like various
> webkit-prefixed stuff, to the spec first.

Given that we're now doing that in https://compat.spec.whatwg.org/ do you think we should add this method back?

Philip, we can make the argument optional. In that case it should simply return false, right?
Comment 19 Philip Jägenstedt 2015-08-03 11:16:02 UTC
As of https://codereview.chromium.org/978223002 the argument is non-optional in Blink, matching the signature of isEqualNode().

Whether it actually needs to be nullable for web compat I don't know, but trying to find out doesn't seem worthwhile.
Comment 20 Boris Zbarsky 2015-08-03 15:17:20 UTC
> do you think we should add this method back?

I don't know.  I was just saying the "blink won't remove it" argument was weaker for this method than for lots of prefixed stuff is all.
Comment 21 Anne 2016-03-14 13:27:02 UTC
Adding it back since Gecko can't win this alone: https://github.com/whatwg/dom/commit/97f0a958bc1dcb98265d116bafdc80ae1b059823. 😟
Comment 22 Philip Jägenstedt 2016-03-14 13:54:44 UTC
Thanks, I like the comment too. "Fortunately" there are lots more interop hazards to work through that'll require our joint efforts to resolve.
Comment 23 Philip Jägenstedt 2016-03-16 06:21:17 UTC
Tests requested in https://github.com/w3c/web-platform-tests/issues/2671