Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new mutation observer init options elementByAttributeFilter and elementFilter #885

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AndrewRot
Copy link

@AndrewRot AndrewRot commented Aug 12, 2020

Adds additional options when constructing a MutationObserver.

MutationObserverInit now contains two additional options: elementByAttributeFilter and elementFilter which are used to enhance the precision of filtering DOM nodes to observe.

See #398 for the background.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
    • … TODO
  • Implementation bugs are filed:
    • Chrome: None yet
    • Firefox: None yet
    • Safari: None yet

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@AndrewRot AndrewRot force-pushed the new-mutation-observer-init-options branch from 624f14f to 020ee4a Compare August 12, 2020 22:33
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@AndrewRot AndrewRot changed the title Add new mutation observer init options elementHasAttributeNameFilter and elementLocalNameFilter Add new mutation observer init options elementFilterByAttribute and elementLocalNameFilter Aug 13, 2020
@annevk
Copy link
Member

annevk commented Aug 14, 2020

@AndrewRot can Shape Security sign https://participate.whatwg.org/agreement? Previously that ended up not happening, but not sure what the reason was.

@AndrewRot AndrewRot force-pushed the new-mutation-observer-init-options branch from e37b25c to 19960d9 Compare August 28, 2020 16:20
@AndrewRot

This comment has been minimized.

dom.bs Outdated Show resolved Hide resolved
@AndrewRot AndrewRot force-pushed the new-mutation-observer-init-options branch from d00431a to d1c9cb6 Compare August 31, 2020 16:03
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated
{{MutationObserverInit/elementFilterByAttribute}} is present, then:

<ol>
<li><p>Let <var>nodeAttributesList</var> be the set of local names of node's attributes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dom.bs Outdated
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated
<li>
<p>For each <var>node</var> in <var>addedNodes</var></a>.

<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dom.bs Outdated
<p>If none of the following are true

<ul class=brief>
<li>if <var>options</var>'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dom.bs Outdated
<ul class=brief>
<li>if <var>options</var>'s
{{MutationObserverInit/elementLocalNameFilter}} is present, and
"<code>node.localName</code>" is in {{MutationObserverInit/elementLocalNameFilter}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@annevk
Copy link
Member

annevk commented Sep 10, 2020

@mfreed7 any thoughts from Google on this?

@rniwa
Copy link
Collaborator

rniwa commented Sep 11, 2020

FWIW, we're supportive of this proposal.

@AndrewRot
Copy link
Author

@annevk yes - I've gotten written approval to sign off on the agreement from our legal department as well as my team managers.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for confirming the IPR stuff, that's great! There's a couple things remaining with the PR (note that several comments apply to multiple places) and the other thing we'll need here are tests. Do you think you can work on those?

dom.bs Outdated

<ol>
<li><p>Let <var>nodeAttributesList</var> be the set of local names of <var>node</var>'s
attributes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"local names" and "attributes" should xref the relevant definitions. (We should also have a test with two attributes that have the same local name and see if this is all working as designed.)

dom.bs Outdated
<p>If none of the following are true

<ul class=brief>
<li>if <var>options</var>'s {{MutationObserverInit/elementFilter}} is present, and the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the nested "if" works here. This needs rephrasing.


<ul class=brief>
<li>if <var>options</var>'s {{MutationObserverInit/elementFilter}} is present, and the
<a>node</a>'s <var>localName</var> is in {{MutationObserverInit/elementFilter}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite confusing. Isn't node the variable and local name the field?

dom.bs Outdated
attribute name in <var>nodeAttributesList</var>
</ul>

<p>then <a for=queue>enqueue</a> <var>record</var> node in <var>filteredAddedNodes</var>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might mean <a for=list>append</a> <var>node</var> to <var>filteredAddedNodes</var> here?

Base automatically changed from master to main January 15, 2021 07:32
@AndrewRot AndrewRot changed the title Add new mutation observer init options elementFilterByAttribute and elementLocalNameFilter Add new mutation observer init options elementByAttributeFilter and elementFilter Sep 13, 2021
@AndrewRot
Copy link
Author

@annevk looks like agreement was filled out. Though I just added my manager as a secondary contact.

All comments appear to have been addressed. At this point, the only thing missing seems to be the tests? I can start on those.

@annevk
Copy link
Member

annevk commented Sep 23, 2021

@AndrewRot that sounds great, thank you! Once the tests are there I'm happy to give this another pass.

@bershanskiy
Copy link

Has there been any progress on the tests? I'm working on a product which would benefit from this Mutation Observer filter feature, and might be able to assist with tests if there is interest.

@annevk
Copy link
Member

annevk commented Dec 13, 2022

@mfreed7 ping on Chromium interest. @smaug---- Gecko?

@bershanskiy I'd be happy to pick up the remaining specification work assuming @AndrewRot no longer has the bandwidth if you can sort the tests. Thanks for volunteering!

@AndrewRot
Copy link
Author

Hi @bershanskiy & @annevk - I'm glad this feature is useful for others! Yes, feel free to take the reins from here. I do not currently have the bandwidth to allocate for this feature.

</ol>

<li><p>Assert: either <var>filteredAddedNodes</var> or <var>filteredRemovedNodes</var>
<a for=set>is not empty</a>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we assert this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these are now defined in scope. Check if filteredAddedNodes length > 0 or filteredRemovedNodes length > 0.

attribute names in <var>nodeAttributesList</var>
</ul>

<p>then <a for=list>append</a> <var>node</var> to <var>filterRemovedNodes</var>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be confused here.
Don't we want something like
if elementFilter is present, and elementByAttributeFilter is not present and localName is in elementfilter, or if
elementByAttributeFilter is present and elementFilter is not present and elementByAttributeFilter contains any of the attribute names in nodeAttribute list ... or combination of those two

@smaug----
Copy link
Collaborator

Gecko would implement this or some variant of it.
(which isn't super surprising since looks like I filed #398 )

@mfreed7
Copy link
Contributor

mfreed7 commented Dec 15, 2022

@mfreed7 ping on Chromium interest. @smaug---- Gecko?

@bershanskiy I'd be happy to pick up the remaining specification work assuming @AndrewRot no longer has the bandwidth if you can sort the tests. Thanks for volunteering!

Looks like a good addition to MutationObserver which would avoid boilerplate JS and speed things up a lot. I'm supportive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants