Re: [whatwg/dom] Add new mutation observer init options elementFilterByAttribute and elementLocalNameFilter (#885)

@annevk commented on this pull request.

Generally this looks great, thanks for working on this! I have left a couple of formatting nits and I'm happy to help with those once we're further along.

We'll need a lot of tests to cover the various conditions and new behavior of `observe()`.

I'm a little concerned about the agreement. Typically a person in a management or a very senior engineer would sign it (or at least be an alternate), but neither appears to be the case here, assuming I have interpreted your title correctly. As it seems this is a very big company, have legal et al signed off on this?

> @@ -3320,6 +3320,8 @@ dictionary MutationObserverInit {
   boolean attributeOldValue;
   boolean characterDataOldValue;
   sequence<DOMString> attributeFilter;
+  sequence<DOMString> elementFilterByAttribute;
+  sequence<DOMString> elementLocalNameFilter;

Shall we name this `elementFilter` to match `attributeFilter`?

> @@ -3594,6 +3625,60 @@ run these steps:
 <ol>
  <li><p>Assert: either <var>addedNodes</var> or <var>removedNodes</var> <a for=set>is not empty</a>.
 
+ <li><p>If either <var>options</var>'s' {{MutationObserverInit/elementLocalNameFilter}} or
+ {{MutationObserverInit/elementFilterByAttribute}} is present, then:
+
+ <ol>
+  <li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.

What is node referring to here? I feel like this is lacking some references to other variables.

> @@ -3594,6 +3625,60 @@ run these steps:
 <ol>
  <li><p>Assert: either <var>addedNodes</var> or <var>removedNodes</var> <a for=set>is not empty</a>.
 
+ <li><p>If either <var>options</var>'s' {{MutationObserverInit/elementLocalNameFilter}} or
+ {{MutationObserverInit/elementFilterByAttribute}} is present, then:

Since this `<p>` isn't the only child of the `<li>`, it needs to be on its own line, indented one. This also goes for the following `<ol>`, which is not indented accurately.

> @@ -3594,6 +3625,60 @@ run these steps:
 <ol>
  <li><p>Assert: either <var>addedNodes</var> or <var>removedNodes</var> <a for=set>is not empty</a>.
 
+ <li><p>If either <var>options</var>'s' {{MutationObserverInit/elementLocalNameFilter}} or
+ {{MutationObserverInit/elementFilterByAttribute}} is present, then:
+
+ <ol>
+  <li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.
+
+  <li><p>Let <var>filteredAddedNodes</var> be an empty <a for=/>list</a>.
+
+  <li><p>Let <var>filterRemovedNodes</var> be an empty <a for=/>list</a>.
+
+  <li>
+   <p>For each <var>node</var> in <var>addedNodes</var></a>.

A for each should end in a `:`.

> @@ -3594,6 +3625,60 @@ run these steps:
 <ol>
  <li><p>Assert: either <var>addedNodes</var> or <var>removedNodes</var> <a for=set>is not empty</a>.
 
+ <li><p>If either <var>options</var>'s' {{MutationObserverInit/elementLocalNameFilter}} or
+ {{MutationObserverInit/elementFilterByAttribute}} is present, then:
+
+ <ol>
+  <li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.
+
+  <li><p>Let <var>filteredAddedNodes</var> be an empty <a for=/>list</a>.
+
+  <li><p>Let <var>filterRemovedNodes</var> be an empty <a for=/>list</a>.
+
+  <li>
+   <p>For each <var>node</var> in <var>addedNodes</var></a>.
+
+   <li>

I would expect a for each to be followed by a nested list so there's an `<ol>` missing here I think.

> +
+ <ol>
+  <li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.
+
+  <li><p>Let <var>filteredAddedNodes</var> be an empty <a for=/>list</a>.
+
+  <li><p>Let <var>filterRemovedNodes</var> be an empty <a for=/>list</a>.
+
+  <li>
+   <p>For each <var>node</var> in <var>addedNodes</var></a>.
+
+   <li>
+     <p>If none of the following are true
+
+     <ul class=brief>
+      <li>if <var>options</var>'s

s/if// I think? Same below. If not I'd try to reword these to be more clearly true/false statements.

> +  <li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.
+
+  <li><p>Let <var>filteredAddedNodes</var> be an empty <a for=/>list</a>.
+
+  <li><p>Let <var>filterRemovedNodes</var> be an empty <a for=/>list</a>.
+
+  <li>
+   <p>For each <var>node</var> in <var>addedNodes</var></a>.
+
+   <li>
+     <p>If none of the following are true
+
+     <ul class=brief>
+      <li>if <var>options</var>'s
+      {{MutationObserverInit/elementLocalNameFilter}} is present, and
+      "<code>node.localName</code>" is in {{MutationObserverInit/elementLocalNameFilter}}

What is node.localName and why is it formatted this way? Is that a special value?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/pull/885#pullrequestreview-485741098