849661 - Remove support for Node.hasAttributes()

Status: RESOLVED WONTFIX

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

Description

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

Node.hasAttributes() has been removed from the spec: <http://dom.spec.whatwg.org/#dom-node-hasattributes>. We should try to remove it as well.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Tested already in <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/historical.html>.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

We actually use this method in parser/htmlparser/tests/mochitest/parser_datreader.js.  Now that .attributes is on Element instead of Node, the correct replacement of node.hasAttributes() is node.attributes && node.attributes.length, right?  That's a bit verbose, especially when "node" is the result of a method.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

I'm not totally convinced we actually want to do this, especially since .attributes is no longer on Node per spec.  But if we do, here's a patch.  WebKit seems not to support the method, so probably there's no big compat risk.

https://tbpl.mozilla.org/?tree=Try&rev=f084e2ba16a6

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not totally convinced either.  Peter?  Jonas?

Comment 5

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

Unless there is compat risk, or there are strong stated use cases, I think we should nuke it from orbit.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727198 [details] [diff] [review]
Patch

r=me

Comment 7

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

Strictly speaking, you should also update the uuids for all subclasses of nsIDOMNode; fortunately, there's a script: <http://people.mozilla.org/~sfink/uploads/update-uuids>.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Should I update all the other IIDs as Ms2ger says?  I don't think anyone's ever asked me to do that before (maybe I broke some extensions!).

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Should I update all the other IIDs as Ms2ger says?

Yes.  I keep forgetting about that bit of annoyance.  :(

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Unless there is compat risk, or there are strong stated use cases, I think
> we should nuke it from orbit.

It's used in <https://developer.mozilla.org/en-US/docs/JXON>, and you've just broken comm-central's JXON code as a result.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Can that be fixed using comment 2?

Comment 14

[Joshua Cranmer [:jcranmer]]

Probably; I haven't tested it.

Comment 15

[Mark Banner (:standard8)]

I ported the change suggested in comment 2 and it passed the broken unit tests:

https://hg.mozilla.org/comm-central/rev/cebc16631aa6

Comment 16

[Tom S. (please needinfo tschuster)]

https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers
https://developer.mozilla.org/en-US/docs/DOM/Node.hasAttributes

Comment 17

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Comment 18

[Erik Arvidsson]

WebKit moved Node.prototype.hasAttributes and Node.prototype.attributes to Element.prototype. Removing hasAttributes completely seems like too large of a risk.

Comment 19

[Mike Kaply [:mkaply]]

(In reply to Erik Arvidsson from comment #18)
> WebKit moved Node.prototype.hasAttributes and Node.prototype.attributes to
> Element.prototype. Removing hasAttributes completely seems like too large of
> a risk.

As in don't remove it from Node?

If you don't, you lose the ability to easily check for it before using it (which is the easy fix for the JXON code)

Comment 20

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

The fact that webkit still has this certainly puts the risk involved here into a different light.

I don't have strong opinions though. Ideal would be to go through the safe route of warning+measuring (using telemetry) and then removing only if usage is small enough. But that's a lot of work so I don't have a strong opinion.

But please be careful before issuing statements like the one in comment 3.

Comment 21

[Kohei Yoshino [:kohei]]

Adding a brand-new site-compat keyword.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Sorry for comment 3 -- I don't know what test I was using, but probably I was using a non-element node like "document" and didn't think to check an element.  I don't see the value in trying to remove this if everyone does in fact support it; I'd be amazed if it doesn't cause at least a few pages to break.

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Spec bug asking for it to be re-added: https://bugs.webkit.org/show_bug.cgi?id=113580

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I meant: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21522

Comment 25

[Boris Zbarsky [:bzbarsky]]

Thank you, Aryeh.

I've backed this out on inbound in bug 856752.

Comment 26

[Günter]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> 
> It's used in <https://developer.mozilla.org/en-US/docs/JXON>, and you've
> just broken comm-central's JXON code as a result.

That's excactly what happed to my Reminderfox implementation for CalDAV support following that JXON page!
It was working up to TB 21 but failed with TB22.0a2!