Effective children #2548

kevinpschaaf · 2015-10-08T17:19:28Z

No description provided.

…extContent`

Conflicts: src/lib/dom-api.html

…xample, when attributes change under Shadow DOM.

Conflicts: src/lib/dom-api.html

…tifications under shadow dom.

Conflicts: test/unit/utils-content.html test/unit/utils-elements.html

googlebot · 2015-10-08T17:19:30Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

kevinpschaaf · 2015-10-08T17:21:20Z

src/lib/dom-api-mutation.html

+        for (var i=0, h, n; (i < elements.length) && (n=elements[i]); i++) {
+          if (this._isContent(n)) {
+            n.__observeNodesMap = n.__observeNodesMap || new WeakMap();
+            if (n.__observeNodesMap.get(this) == null) {


n.__observeNodesMap.has(this)

Is the check necessary? If everything is working correctly, should only ever observe one content once?

kevinpschaaf · 2015-10-13T17:01:29Z

src/lib/dom-api-mutation.html

+            // attribute tracking?
+            this._observer.observe(this.node, {
+              childList: true,
+              attributes: true,


Sucks that there's no childList + attributes combo that actually means "any attribute changes on my childList".

So any change (add, remove, attribute change) to any of its light children descendants will incur the cost of an _observeContentElements iteration (once) and a levenshtein (per listener).

Seems potentially expensive enough to keep the attributes option, since it allows you to remove both attributes + subtree.

Is there an option to auto-enable attributes if you encounter a select on a <content> while observing? Or is that too complicated?

…t select> that depends on attributes; add tests.

kevinpschaaf · 2015-10-14T01:47:19Z

src/lib/dom-api-observe-distributed-nodes.html

+                this._scheduleNotify.bind(this));
+              if (this._hasAttrSelect()) {
+                Polymer.dom(host).observer.enableShadowAttributeTracking();
+                this._observer._alwaysCallListener = true;


_alwaysCallListener needs to get passed up via enableShadowTracking?

…ibutes under native Shadow DOM; +minor factoring

kevinpschaaf · 2015-10-14T20:08:25Z

LGTM! 👍 🏆 children: [ 👦 👧 👶 ] changed: [ 🚸 ]

Effective children

samccone · 2015-10-14T20:22:14Z

@sorvell / @kevinpschaaf any design docs around this feature / why it is good / how/when to use it?

sorvell · 2015-10-14T20:26:08Z

Will be forthcoming after discussion with @arthurevans ...

arthurevans · 2015-10-14T23:46:29Z

What I have learned so far is:

use cases for the effective children APIs may be a trifle abstruse. But perhaps not so much as I think.
observeNodes is nifty.

For example, consider a tab panel element that we use like this:

<simple-tab-panel>
  <simple-tab></simple-tab>
  <simple-tab></simple-tab>
  <div class="tab-page"></div>
  <div class="tab-page"></div>
</simple-tab-panel>

The contract is simply: clicking the first tab shows the first tab-page, and so on. But since I'm a good citizen, I want to generate unique IDs for each of these elements and set up ARIA attributes to link them: aria-controls="tab-page-someuniqueid".

It's easy to do this once in ready, but what if the user adds tabs later? I used to have to add a mutation observer, which still wouldn't catch the case where one of my light DOM children is a <content> element that may have children distributed into it.

Now I can do something like:

Polymer.dom(this.$.tabContainer).observeNodes(tabsChanged);

Which is pretty nifty, right?

arthurevans · 2015-10-14T23:47:35Z

See also: Chris's PR, above, for example niftiness.

samccone · 2015-10-14T23:54:20Z

Wooot for observeNodes makes sense thx @arthurevans , is there a way to unregister an observation?

nm I found unobserveNodes

cdata · 2015-10-14T23:54:27Z

@sorvell @arthurevans The asynchrony involved in using a MutationObserver to measure added and removed items suggests that IronSelectableBehavior might be redesigned to handle items that "don't exist yet" a little more gracefully. This is because an item "not existing" could mean A) the item doesn't actually exist as a child of the selectable, or B) the item exists but the MutationObserver hasn't told us about it yet. Right now, IronSelectableBehavior is designed around the assumption that the set of items is whatever you get when you querySelectorAll for items, and this is a fundamentally different approach that does not take full advantage of the change notifications you get from observeNodes.

cdata · 2015-10-14T23:55:06Z

@samccone Yup, see https://github.com/PolymerElements/iron-selector/blob/observe-nodes/iron-selectable.html#L144

samccone · 2015-10-14T23:55:29Z

Q: should unobserveNodes automatically (Polymer do this for us) be called when an element is detached?

cdata · 2015-10-14T23:59:13Z

@samccone I don't think that you necessarily want that always, although I admit it is usually dubious for a user to want to respond to these types of changes while detached.

samccone · 2015-10-15T00:02:25Z

I only wonder because of the potential for users to shoot themselves in the foot memory leak wise with this non private API.

cdata · 2015-10-15T00:06:51Z

In support of what you are saying, listeners automatically "unlisten" when you detach (I think).

cdata · 2015-10-15T00:12:45Z

Okay, I take that back. listeners does not automatically "unlisten" upon detach.

samccone · 2015-10-15T00:17:38Z

yeah so, imho unregistering automatically is a huge win for users from a memory and leak standpoint.. with the ability to conditionally opt out of this behavior

Can anyone come up with a case why this should not act this way?

ebidel · 2015-10-15T00:21:45Z

Can you open an issue to discuss? This PR probably isn't the right place to
suggest new features :)

On Wed, Oct 14, 2015 at 5:17 PM Sam Saccone [email protected]
wrote:

yeah so, imho unregistering automatically is a huge win for users from a
memory and leak standpoint.. with the ability to conditionally opt out of
this behavior

Can anyone come up with a case why this should not act this way?

—
Reply to this email directly or view it on GitHub
#2548 (comment).

Steven Orvell added 17 commits August 3, 2015 16:11

Adds getEffectiveChildNodes, getEffectiveChildren, `getEffectiveT…

f34fb45

…extContent`

Adds Polymer.dom(element).observeChildren(callback) api

6499e83

Merge branch 'master' into effective-children

1ca065f

Factor dom-api's into separate helpers.

effedcb

Merge branch 'master' into effective-children

393ba40

Conflicts: src/lib/dom-api.html

Add mutation tracking for distributedNodes.

b11f86b

Add Polymer.dom().notifyObservers method to 'kick' observers, for e…

07261e4

…xample, when attributes change under Shadow DOM.

Merge branch 'master' into effective-children

65abbd0

Merge branch 'master' into effective-children

19aa6eb

Conflicts: src/lib/dom-api.html

Merge branch 'master' into effective-children

1774f57

Add optional attribute tracking to support better distributed node no…

8242a98

…tifications under shadow dom.

Merge branch 'master' into effective-children

4099342

Merge branch 'master' into effective-children

ff2c088

Merge branch 'master' into effective-children

c09296e

add observeNodes tests.

bd90b57

make tests work on polyfill.

9ff2ee4

Merge branch 'master' into effective-children

b40060a

Conflicts: test/unit/utils-content.html test/unit/utils-elements.html

googlebot added the cla: no label Oct 8, 2015

kevinpschaaf reviewed Oct 8, 2015
View reviewed changes

Merge branch 'master' into effective-children

e11a4f3

kevinpschaaf reviewed Oct 13, 2015
View reviewed changes

Steven Orvell added 3 commits October 13, 2015 11:14

Make shadow attribute tracking automatic based on detecting a <conten…

54911a7

…t select> that depends on attributes; add tests.

Add docs

0ede79a

Add <content>.getDistributedNodes observation. Refactor flush.

8b1face

kevinpschaaf reviewed Oct 14, 2015
View reviewed changes

ensure distribution observers see all changes that can come from attr…

344f5cc

…ibutes under native Shadow DOM; +minor factoring

kevinpschaaf added a commit that referenced this pull request Oct 14, 2015

Merge pull request #2548 from Polymer/effective-children

c00c772

Effective children

kevinpschaaf merged commit c00c772 into master Oct 14, 2015

kevinpschaaf deleted the effective-children branch October 14, 2015 20:08

cdata mentioned this pull request Oct 14, 2015

Observe nodes PolymerElements/iron-selector#73

Merged

samccone mentioned this pull request Oct 15, 2015

Automatically unregister listeners and node observers on detachment #2567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Effective children #2548

Effective children #2548

kevinpschaaf commented Oct 8, 2015

googlebot commented Oct 8, 2015

kevinpschaaf Oct 8, 2015

kevinpschaaf Oct 9, 2015

kevinpschaaf Oct 13, 2015

kevinpschaaf Oct 14, 2015

kevinpschaaf commented Oct 14, 2015

samccone commented Oct 14, 2015

sorvell commented Oct 14, 2015

arthurevans commented Oct 14, 2015

arthurevans commented Oct 14, 2015

samccone commented Oct 14, 2015

cdata commented Oct 14, 2015

cdata commented Oct 14, 2015

samccone commented Oct 14, 2015

cdata commented Oct 14, 2015

samccone commented Oct 15, 2015

cdata commented Oct 15, 2015

cdata commented Oct 15, 2015

samccone commented Oct 15, 2015

ebidel commented Oct 15, 2015

Effective children #2548

Effective children #2548

Conversation

kevinpschaaf commented Oct 8, 2015

googlebot commented Oct 8, 2015

kevinpschaaf Oct 8, 2015

Choose a reason for hiding this comment

kevinpschaaf Oct 9, 2015

Choose a reason for hiding this comment

kevinpschaaf Oct 13, 2015

Choose a reason for hiding this comment

kevinpschaaf Oct 14, 2015

Choose a reason for hiding this comment

kevinpschaaf commented Oct 14, 2015

samccone commented Oct 14, 2015

sorvell commented Oct 14, 2015

arthurevans commented Oct 14, 2015

arthurevans commented Oct 14, 2015

samccone commented Oct 14, 2015

cdata commented Oct 14, 2015

cdata commented Oct 14, 2015

samccone commented Oct 14, 2015

cdata commented Oct 14, 2015

samccone commented Oct 15, 2015

cdata commented Oct 15, 2015

cdata commented Oct 15, 2015

samccone commented Oct 15, 2015

ebidel commented Oct 15, 2015