Remove support for Node.hasAttributes()

RESOLVED WONTFIX

Status

()

Core
DOM: Core & HTML
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: ayg)

Tracking

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

Trunk
mozilla22
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Node.hasAttributes() has been removed from the spec: <http://dom.spec.whatwg.org/#dom-node-hasattributes>. We should try to remove it as well.
Tested already in <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/historical.html>.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Flags: in-testsuite+
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.
Created attachment 727198 [details] [diff] [review]
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
Attachment #727198 - Flags: review?(bzbarsky)
I'm not totally convinced either.  Peter?  Jonas?
Flags: needinfo?(peterv)
Flags: needinfo?(jonas)
Unless there is compat risk, or there are strong stated use cases, I think we should nuke it from orbit.
Flags: needinfo?(jonas)
Comment on attachment 727198 [details] [diff] [review]
Patch

r=me
Attachment #727198 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 7

4 years ago
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>.
(Reporter)

Updated

4 years ago
Keywords: addon-compat, dev-doc-needed
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!).
Flags: needinfo?(bzbarsky)
> Should I update all the other IIDs as Ms2ger says?

Yes.  I keep forgetting about that bit of annoyance.  :(
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7db1904193
https://hg.mozilla.org/mozilla-central/rev/cf7db1904193
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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.
Can that be fixed using comment 2?
Probably; I haven't tested it.
I ported the change suggested in comment 2 and it passed the broken unit tests:

https://hg.mozilla.org/comm-central/rev/cebc16631aa6
https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers
https://developer.mozilla.org/en-US/docs/DOM/Node.hasAttributes
Keywords: dev-doc-needed → dev-doc-complete
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

4 years ago
WebKit moved Node.prototype.hasAttributes and Node.prototype.attributes to Element.prototype. Removing hasAttributes completely seems like too large of a risk.
(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)
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.
Depends on: 856752
Adding a brand-new site-compat keyword.
Keywords: site-compat
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.
Spec bug asking for it to be re-added: https://bugs.webkit.org/show_bug.cgi?id=113580
I meant: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21522
Thank you, Aryeh.

I've backed this out on inbound in bug 856752.
Resolution: FIXED → WONTFIX
Blocks: 853865

Comment 26

4 years ago
(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!
Flags: needinfo?(peterv)
You need to log in before you can comment on or make changes to this bug.