Bug 19463 - MutationRecord added/removedNodes could return empty array, not null
MutationRecord added/removedNodes could return empty array, not null
Status: RESOLVED FIXED
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC Linux
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 09:58 UTC by Olli Pettay
Modified: 2012-11-20 18:17 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Olli Pettay 2012-10-11 09:58:41 UTC
This is the behavior in Gecko, and it makes the API, IMO, easier
to use and more consistent with other nodelist getter APIs.
Comment 1 Anne 2012-10-11 14:22:10 UTC
Sorry WebKit for not catching this sooner. Olli is right, they should never have been allowed to be null to begin with.

https://github.com/whatwg/dom/commit/aad67c8f0cd78f3a9157acd481384ae22b5662de
http://dom.spec.whatwg.org/
Comment 2 Adam Klein 2012-10-11 18:39:58 UTC
(In reply to comment #1)
> Sorry WebKit for not catching this sooner. Olli is right, they should never
> have been allowed to be null to begin with.

Luckily WebKit already had these empty for type = 'childList'; I'd hope that there's not much code that looks at added/removed nodes for non-childList records. Just curious, what's the advantage of having those non-null on attributes/characterData records?
Comment 3 Olli Pettay 2012-10-11 18:48:25 UTC
No need to null check if you have some generic stuff handling
MutationRecords.
Comment 4 Rafael Weinstein 2012-10-12 20:13:42 UTC
my $.02:

If you are processing every mutation as if it were a childlist, you're doing something wrong and likely to get the wrong answer. It should be an invariant that a childlist mutation record has at least one node in either of added/removed nodes and code which tolerates both being empty is probably wrong.

I think that having an empty array for added/removed in the case of non-childlist changes suggests something misleading about how the API works.
Comment 5 Anne 2012-10-12 20:17:56 UTC
Olli, do you feel strongly about this? It kinda makes sense I guess to make them null in the non-childList case. (Personally I thought we should have had separate record interfaces...)
Comment 6 Anne 2012-10-12 20:18:35 UTC
Reopening so we can reach some kind of conclusion.
Comment 7 Olli Pettay 2012-10-13 13:56:06 UTC
I don't feel strongly about this.
But I think returning always an array makes the API more consistent.
addedNodes and removedNodes tell always which nodes where added or removed,
and in cases of non-childList they are empty, since nothing was added or
removed.
Comment 8 Olli Pettay 2012-10-13 14:00:30 UTC
Somewhat similar happens for example with
document.querySelectorAll("foo").
If there are no foos, it is empty list, not null which is returned.
Comment 9 Anne 2012-10-17 09:08:19 UTC
I think the point is that the other NodeList accessors are never exposed in a context where the NodeList can only be empty.

This is one of the reasons why I would have preferred distinct MutationRecord interfaces.

Anyway, still not sure what to do.
Comment 10 Olli Pettay 2012-10-17 11:11:41 UTC
(In reply to comment #4)
>  It should be an
> invariant that a childlist mutation record has at least one node in either
> of added/removed nodes and code which tolerates both being empty is probably
> wrong.
Why such invariant?

> I think that having an empty array for added/removed in the case of
> non-childlist changes suggests something misleading about how the API works.
I see addedNodes and removedNodes as part of the MutationRecord.
You should always know whether something was added by checking
record.addedNodes.length.
Comment 11 Rafael Weinstein 2012-10-18 21:20:14 UTC
(In reply to comment #10)
> (In reply to comment #4)
> >  It should be an
> > invariant that a childlist mutation record has at least one node in either
> > of added/removed nodes and code which tolerates both being empty is probably
> > wrong.
> Why such invariant?

What Anne said. You're usually the consistency guy =-). You don't find this to be inconsistent with existing APIs?

> 
> > I think that having an empty array for added/removed in the case of
> > non-childlist changes suggests something misleading about how the API works.
> I see addedNodes and removedNodes as part of the MutationRecord.
> You should always know whether something was added by checking
> record.addedNodes.length.
Comment 12 Olli Pettay 2012-10-18 21:39:29 UTC
(In reply to comment #11)
> What Anne said. You're usually the consistency guy =-). You don't find this
> to be inconsistent with existing APIs?
> 
No. This is consistent with existing APIs.
You get empty Nodelist all the time from various functions 
(I know, we're dealing attributes here), but not null.


Also, it would be inconsistent to have empty nodelist in case type
is childList, but null in other cases.
Comment 13 Rafael Weinstein 2012-10-18 22:10:42 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > What Anne said. You're usually the consistency guy =-). You don't find this
> > to be inconsistent with existing APIs?
> > 
> No. This is consistent with existing APIs.
> You get empty Nodelist all the time from various functions 
> (I know, we're dealing attributes here), but not null.
> 
> 
> Also, it would be inconsistent to have empty nodelist in case type
> is childList, but null in other cases.

I don't have any further defense for my position. I suppose it's stylistic. My $.02 is that null for the case of non-childlist is correct. I defer to Anne to make the call. FWIW, This doesn't strike me as "make-or-break" either way.
Comment 14 Anne 2012-11-08 14:12:53 UTC
Resolving as FIXED per comment 1 meaning they are not nullable. Rationale is that when e.g. logging everything to a console obj.addedNodes.length would fail.
Comment 15 Anne 2012-11-08 14:36:07 UTC
Argh. I just noticed that every other member does use the nullable pattern. E.g. either a string or null (rather than simply empty string). Olli?
Comment 16 Olli Pettay 2012-11-08 15:38:12 UTC
What has that to do with this bug.
null is ok for string attributes IMO, and even needed in cases like
oldValue, where the oldValue might be "".
Comment 17 Anne 2012-11-08 15:40:55 UTC
Meh.
Comment 18 Rafael Weinstein 2012-11-20 18:13:48 UTC
Ok. Just to be clear (because it looks like WebKit needs to change), the decision here is that addedNodes and removedNodes must be empty NodeLists in the case of non-childList mutation type?
Comment 19 Olli Pettay 2012-11-20 18:16:10 UTC
Yes.
Comment 20 Olli Pettay 2012-11-20 18:17:00 UTC
(as they are also in case of childList when no nodes are added or removed.)