27122 2014-10-21 17:42:26 +0000 MutationObserver.observe(node, {}) should throw 2014-10-22 14:11:07 +0000 1 1 1 Unclassified WebAppsWG DOM unspecified PC Linux RESOLVED FIXED P2 normal --- 1 bugs annevk jonas mike www-dom public-webapps-bugzilla oldest_to_newest 113533 0 bugs 2014-10-21 17:42:26 +0000 If nothing would be observed, observe() should just throw. This was the behavior at some point, but the spec seems to have changed. 113535 1 bugs 2014-10-21 18:19:05 +0000 "If either options' attributeOldValue or attributeFilter is present and options' attributes is omitted, set options' attributes to true. " should also check that attributeFilter has at least one value. 113536 2 jonas 2014-10-21 18:49:55 +0000 Note that this means that if we add the ability to observe more things in the future, like shadow-DOM stuff, that would mean that MutationObserver.observe(node, { newFeature: true }) would throw in downlevel browsers, whereas MutationObserver.observe(node, { newFeature: true, childList: true }) would not throw. This seems somewhat inconsistent and could force authors to wrap try/catches around their code just to suppress exceptions. 113537 3 bugs 2014-10-21 19:10:59 +0000 Well, MutationObserver.observe(node, { newFeature: true }) certainly should throw if that newFeature isn't supported. 113538 4 jonas 2014-10-21 19:22:12 +0000 Do you also think that MutationObserver.observe(node, { newFeature: true, childList: true }) should throw if newFeature is not supported? 113539 5 bugs 2014-10-21 19:26:51 +0000 Oh, I see. Yeah, that newFeature shouldn't cause throwing, unless having newFeature: true and childList: true doesn't make sense in the implementations which support newFeature. That is similar to observe(node, { attributeOldValue: true, attributes: false}); which throws. So, {} should throw, since it doesn't cause any node to be observed in anyway. 113540 6 jonas 2014-10-21 19:41:26 +0000 So do you think that MutationObserver.observe(node, { newFeature: true }) should throw in current browsers? Or is it specifically *only* empty objects that should cause an exception? 113541 7 bugs 2014-10-21 20:05:19 +0000 if observe(foo, { ... something...}) doesn't end up doing anything (not observe any changes), that should throw. Scripts should know asap that something is going wrong, not wait later to figure out that the expected MutationRecords won't be there. 113542 8 annevk 2014-10-21 20:13:29 +0000 (In reply to Olli Pettay from comment #0) > This was the behavior at some point, but the spec seems to have changed. [citation needed] (In reply to Olli Pettay from comment #1) > should also check that attributeFilter has at least one value. No, I recall we decided that checking for presence was what we wanted here. 113543 9 bugs 2014-10-21 20:23:59 +0000 (In reply to Anne from comment #8) > (In reply to Olli Pettay from comment #0) > > This was the behavior at some point, but the spec seems to have changed. > > [citation needed] Gecko's current behavior follows an older version of the spec. > (In reply to Olli Pettay from comment #1) > > should also check that attributeFilter has at least one value. > > No, I recall we decided that checking for presence was what we wanted here. [citation needed] ;) 113546 10 jonas 2014-10-21 22:49:09 +0000 (In reply to Olli Pettay from comment #7) > if observe(foo, { ... something...}) doesn't end up doing anything (not > observe any changes), that should throw. Scripts should know asap that > something is going wrong, not wait later to figure out that the expected > MutationRecords won't be there. You seem to consider "observing fewer things than expected" to be different from "observing nothing". Is that true? I.e. in both of MutationObserver.observe(node, { newFeature: true }) MutationObserver.observe(node, { newFeature: true, childList: true }) the newFeature observations won't be fired in a downlevel browser. Are you saying that's worse in the first case than in the second case? 113549 11 bugs 2014-10-21 23:11:50 +0000 That is mostly hypothetical, and if new features is to be added, we should make sure that backwards compatibility is sane. Is there any reason to not throw early if observe() doesn't do anything sane? (and it used to do that) Why would newFeature be handled differently to existing properties where we throw in some cases. 113550 12 bugs 2014-10-21 23:19:56 +0000 Other option is to never throw from observe() (but that would be just a bad API). Either we throw in all the cases observe() doesn't do anything sane, or we never throw. No special cases. Consistency, pretty please. 113552 13 jonas 2014-10-21 23:45:24 +0000 (In reply to Olli Pettay from comment #11) > Is there any reason to not throw early if observe() doesn't do anything sane? The reason is that "doesn't do anything sane" is going to vary from use-case to use-case. I.e. in some cases it might be quite sane to simply not receive certain notifications if the browser doesn't support them, in others not. For example if we add notifications for Shadow DOM, then not firing those in browsers that doesn't support Shadow DOM might be just fine. In such browsers the website might use a Shadow DOM polyfill which provides its own notification mechanism. The other reason is that we can't really be consistent. I.e. we can't make both of these throw, so it might be more consistent to make neither throw: MutationObserver.observe(node, { newFeature: true }) MutationObserver.observe(node, { newFeature: true, childList: true }) The reason we can't make both throw is that JS doesn't provide a good way to check that "no unknown properties are provided". Things like non-enumerable properties, properties on prototype objects, and proxies simply make checking for lack of unknown properties too hard/complex. 113560 14 bugs 2014-10-22 11:20:31 +0000 We're not adding any shadow DOM specific options atm. That is hypothetical still (observing changes in Shadow DOM would make its encapsulation weaker and weaker, though since it pretty much doesn't any any encapsulation, perhaps that doesn't really matter.) If one is using options which don't cause any observing would be a bug in application logic, and in such case there should be an exception thrown as early as possible. (In reply to Jonas Sicking from comment #13) > (In reply to Olli Pettay from comment #11) > The other reason is that we can't really be consistent. I.e. we can't make > both of these throw, so it might be more consistent to make neither throw: > > MutationObserver.observe(node, { newFeature: true }) > MutationObserver.observe(node, { newFeature: true, childList: true }) I'm not saying they both should throw. The latter is perfectly valid. This is no different to XHR.open("foobar", "http://www.w3.org"); Maybe in the future we'll support "foobar" method, but for now open() throws if invalid method name is passed to it as the first parameter. 113561 15 annevk 2014-10-22 11:28:26 +0000 That seems like a bad example. It shouldn't throw, and I don't think it does. 113563 16 bugs 2014-10-22 11:50:20 +0000 "If method is not a method, throw a SyntaxError exception." 113564 17 annevk 2014-10-22 12:37:56 +0000 Right, and "foobar" is a method per that definition. Anyway, I'm not sure why we'd start throwing here. Is that what any implementation does? 113565 18 bugs 2014-10-22 12:49:45 +0000 Make it " foo bar something which doesn't look like the current token in http1" ;) Gecko throws in this case var mo = new MutationObserver(function() {}); mo.observe(document, {}); since the spec used to require throwing in that case. http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/content/base/src/nsDOMMutationObserver.cpp#l456 And I don't know why the spec removed throwing in that case but kept throwing in some other cases. 113566 19 annevk 2014-10-22 13:03:07 +0000 You are correct. Originally we had https://github.com/whatwg/dom/commit/5527027d71d72293f1e5cbafbf5e938f026a3b50 which got changed in https://github.com/whatwg/dom/commit/a770b11981f4c147f98753cf726e44b14d17affc based on discussion in bug 23189. 113567 20 bugs 2014-10-22 13:06:38 +0000 And https://www.w3.org/Bugs/Public/show_bug.cgi?id=23189#c3 * If none of options' childList, attributes, and characterData is true, throw a TypeError. Something else what committed to the spec. 113568 21 annevk 2014-10-22 14:11:07 +0000 https://github.com/whatwg/dom/commit/4bf29a4ae4a1e5e4f1a5249f5fc7aa40d37b3f51 Thanks Olli!