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

Observe shadowRoot mutations #18

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

Conversation

valdrinkoshi
Copy link
Collaborator

@valdrinkoshi valdrinkoshi commented Aug 6, 2016

Fixes #17 by observing node changes on the shadowRoot of each InertRoot.

If a node doesn't have yet a shadow root, we patch the methods to create a shadow root and we add our observer then! This might be slow, ideally we'd need to observe shadowRoot creation, but this is not possible yet in ShadowDOM v1 WICG/webcomponents#204

@valdrinkoshi valdrinkoshi force-pushed the handle-shadow-roots branch 2 times, most recently from 3c0dba7 to 72d618f Compare August 6, 2016 00:38
@valdrinkoshi valdrinkoshi changed the title Handle shadow roots Observe shadowRoot mutations Aug 6, 2016
@valdrinkoshi
Copy link
Collaborator Author

@robdodson @alice FYI

@valdrinkoshi valdrinkoshi force-pushed the handle-shadow-roots branch 3 times, most recently from aedca2f to 5834723 Compare August 8, 2016 20:59
inert.js Outdated
const rootObserver = new MutationObserver(boundOnMutation);
const shadowRoot = this._rootElement.shadowRoot || this._rootElement.webkitShadowRoot;
if (shadowRoot) {
rootObserver.observe(shadowRoot, { childList: true, subtree: true });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

memo to self: we should be observing also the attributes..!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alice actually here we shoudn't observe attributes, as MutationObserver will observe the ones of the target. In this case, the target is the shadowRoot which won't get any attribute change. this._observer will take care of that.

test/index.js Outdated
shadowButton.textContent = 'Shadow button';
host.shadowRoot.appendChild(shadowButton);
// Give time to mutation observers.
setTimeout(function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

memo to self: use Promise instead of timeout

inert.js Outdated
@@ -86,6 +120,9 @@ class InertRoot {
this._observer.disconnect();
this._observer = null;

this._rootObserver.disconnect();
this._rootObserver = null;
Copy link
Member

Choose a reason for hiding this comment

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

Should we un-monkeypatch createShadowRoot/attachShadow here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, will update!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 👍

inert.js Outdated
this._rootElement.removeAttribute('aria-hidden');
// Restore original attachShadow and createShadowRoot.
delete this._rootElement.attachShadow;
delete this._rootElement.createShadowRoot;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This assumes that no-one else has patched these methods

@valdrinkoshi
Copy link
Collaborator Author

valdrinkoshi commented Oct 27, 2016

@alice @robdodson another approach could be to fire a shadow-root-created event on the patched attachShadow/createShadowRoot, since this seems to be in line with the plan for ShadowDOM v2 WICG/webcomponents#204.
That would translate in patching the HTMLElement.prototype.attachShadow/createShadowRoot and fire the event in the patched functions. The advantage is that when v2 is implemented, we can just remove the attachShadow patch while keeping the one for createShadowRoot.
WDYT?

@valdrinkoshi
Copy link
Collaborator Author

PS: when I run gulp I get this error:

$ gulp
[19:34:13] Using gulpfile ~/dev/inert/gulpfile.js
/Users/valdrin/homebrew/lib/node_modules/gulp/bin/gulp.js:129
    gulpInst.start.apply(gulpInst, toRun);
                  ^

TypeError: Cannot read property 'apply' of undefined
    at /Users/valdrin/homebrew/lib/node_modules/gulp/bin/gulp.js:129:19
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)
    at Module.runMain (module.js:609:11)
    at run (bootstrap_node.js:382:7)
    at startup (bootstrap_node.js:137:9)
    at bootstrap_node.js:497:3

@robdodson any idea why? I tried to follow this guide but no success https://gist.github.com/DanHerbert/9520689

@robdodson
Copy link
Collaborator

@valdrinkoshi which version of node and gulp are you running?

@valdrinkoshi
Copy link
Collaborator Author

@robdodson

valdrin-macbookpro:inert valdrin$ node -v
v7.0.0
valdrin-macbookpro:inert valdrin$ npm -v
3.10.9
valdrin-macbookpro:inert valdrin$ gulp -v
[11:28:03] CLI version 3.9.0
[11:28:03] Local version 4.0.0-alpha.2

@robdodson
Copy link
Collaborator

hm that seems fine... Personally I use nvm to manage my node versions (https://github.com/creationix/nvm) and haven't had issues...

@valdrinkoshi
Copy link
Collaborator Author

@robdodson could you share your node, npm, gulp versions?

@robdodson
Copy link
Collaborator

~/Desktop/poly-foo
❯ node -v
v6.3.0

~/Desktop/poly-foo
❯ npm -v
3.10.3

~/Desktop/poly-foo
❯ gulp -v
[14:17:16] CLI version 1.2.2

@valdrinkoshi
Copy link
Collaborator Author

valdrinkoshi commented Oct 27, 2016

Yay! made it! Had to update the gulp-cli.

Concerning this PR, it's ready for review 👌 I ended up preferring two methods to patch/unpatch attachShadow rather than firing an event, since I agree that firing an event when a shadow root is created kind of defeats the purpose of ShadowDOM (as commented in WICG/webcomponents#204): why should this be notified to the outside elements? it is useful only for tooling or polyfills - there's not a real use case.

@valdrinkoshi valdrinkoshi force-pushed the handle-shadow-roots branch 3 times, most recently from 568bbfe to 1c53778 Compare November 23, 2016 19:14
@robdodson
Copy link
Collaborator

@alice maybe we can spend some time reviewing this in Sydney?

@valdrinkoshi valdrinkoshi force-pushed the handle-shadow-roots branch from 1c53778 to 48353e2 Compare March 9, 2017 23:31
@yoavweiss yoavweiss changed the base branch from master to main March 5, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants