search

whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.58k stars 295 forks source link

Unspecified behavior when NodeFilter#acceptNode returns null #1075

Open kapouer opened 2 years ago

kapouer commented 2 years ago

In https://dom.spec.whatwg.org/#traversal I can't find an answer to:

What happens when acceptNode returns null (or undefined) ?

Maybe it needs to be specified ?

annevk commented 2 years ago

https://dom.spec.whatwg.org/#concept-node-filter calls https://webidl.spec.whatwg.org/#call-a-user-objects-operation which in step 14 (through quite a bit of indirection) ends up running https://webidl.spec.whatwg.org/#es-unsigned-short which does https://webidl.spec.whatwg.org/#abstract-opdef-converttoint which does https://tc39.es/ecma262/#sec-tonumber at some point which treats null as 0 and undefined as NaN (which then becomes an exception).

So I'd say this is defined. I haven't checked if this matches implementations.

kapouer commented 2 years ago

Implementations cast null-ish to zero, then zero magically becomes FILTER_SKIP = 3.

annevk commented 2 years ago

Interesting, yeah I guess that warrants tests and some change. Returning 4 or higher might also be weird?

kapouer commented 2 years ago 
const doc = (new DOMParser()).parseFromString('<html>', 'text/html');
doc.createTreeWalker(doc, NodeFilter.SHOW_ELEMENT, {
 acceptNode: node => node.tagName == "HTML" ? 7 : 1
}).nextNode()
==
doc.createTreeWalker(doc, NodeFilter.SHOW_ELEMENT, {
 acceptNode: node => node.tagName == "HTML" ? 3 : 1
}).nextNode()
kapouer commented 2 years ago

I would like to know what "needs tests" entails ?

annevk commented 2 years ago

Essentially creating a PR for https://github.com/web-platform-tests/wpt/tree/master/dom/traversal to cover this behavior. That's the repository we use for tests covering the standard.

Also, all non-number return values seem to be treated as FILTER_SKIP, rather than coerced. That means that acceptNode() has to be amended to return any instead of unsigned short. And it seems that if the value is a number, Math.floor() is applied to it.

However, according to https://searchfox.org/mozilla-central/source/dom/webidl/NodeFilter.webidl#32 Firefox does have the "accurate" Web IDL for this. @petervanderbeken are we simply not enforcing that as required (see https://github.com/whatwg/dom/issues/1075#issuecomment-1106410304)? Makes me wonder if there's a larger issue here or maybe I'm just missing something.

petervanderbeken commented 2 years ago

https://webidl.spec.whatwg.org/#es-unsigned-short forwards to https://webidl.spec.whatwg.org/#abstract-opdef-converttoint, which in step 8 (after calling ToNumber) does: If x is NaN, +0, +∞, or −∞, then return +0.

So undefined-> NaN -> +0 and null -> +0-> +0.

TreeWalker's nextNode() only checks for the value not being FILTER_REJECT or the value being FILTER_ACCEPT, so it seems correct that any other value is treated as similar to FILTER_SKIP (see https://dom.spec.whatwg.org/#ref-for-concept-node-filter%E2%91%A7).

annevk commented 2 years ago

Ah good. It still seems worth creating some tests for the exceptional cases, e.g., 7, 1n, or { valueOf: () => { throw 1 } }, across the various method calls to see they indeed end up being treated as a result that is not explicitly handled (when they don't result in throwing). But there's no problem with the specification here after all.