See https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c65 and see the page in attachment STRs: - open inspector - expand host element - click on button in the page ER: A #shadowroot should be displayed under the host element AR: Nothing happens So far I don't know which event I could use to be notified of such a change.

Starting looking at this again. (In reply to Olli Pettay [:smaug] from Bug 1053898 comment #66) > You mean some notification to tell if test-component constructor ends up > calling attachShadow and that creates some slots which get assigned? > I guess slotchange should fire. Normally that stays within the shadow dom, > but it should also propagate to > chrome event handler, so in practice to frame script. I am not sure on which object I should be listening to slotchange events in this case? I tried attaching listeners on the host element, on the parent document, but the event never fired when using the test case in attachment. Could you give me some pointers to know where we could listen to slotchange events? Also, in some cases, calling attachShadow might not result in a slotChange, if for instance the shadow dom doesn't contain any slot. But the markup view still needs to be updated to show a "#shadow-root" element inside the component. In this case, is there any other event I could listen to? I made a slightly different test page at: https://juliandescottes.github.io/webcomponents-playground/dynamic/ where the first component will have a slot and might be observable using slotchange as proposed, but the second doesn't.

slotchange fires on the <slot> element itself. I guess we should add a chrome-only "shadowrootadded" event or something of the sort to be able to detect attachShadow without slots. Would that work for you?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2) > slotchange fires on the <slot> element itself. I guess we should add a > chrome-only "shadowrootadded" event or something of the sort to be able to > detect attachShadow without slots. Would that work for you? Yes, that would be perfect!

I filed bug 1470545.

Clearing the ni? for smaug since Emilio already answered. Attached WIP patches that "simulate" the shadowrootattached event by polling the nodes for new shadow root elements. Looks like once we have the event, the rest should be pretty straightforward, the markup view reacts correctly when receiving this new mutation. Will cleanup and add tests when Bug 1470545 lands.

Will also need the _ref() fixes ongoing in Bug 1465873

Comment on attachment 8987454 [details] Bug 1449959 - Listen to shadowrootattached events to update markup view; https://reviewboard.mozilla.org/r/252692/#review260106 ::: devtools/server/actors/inspector/walker.js:1750 (Diff revision 2) > target: targetActor.actorID > }); > }, > > + onShadowrootattached: function(event) { > + const actor = this.getNode(event.target); You need to use composedTarget to get the right target in presence of nested shadow roots I'd think. ::: devtools/server/actors/inspector/walker.js:1750 (Diff revision 2) > target: targetActor.actorID > }); > }, > > + onShadowrootattached: function(event) { > + const actor = this.getNode(event.target); You need to use composedTarget to get the right target in presence of nested shadow roots I'd think.

Comment on attachment 8987454 [details] Bug 1449959 - Listen to shadowrootattached events to update markup view; https://reviewboard.mozilla.org/r/252692/#review260106 > You need to use composedTarget to get the right target in presence of nested shadow roots I'd think. Thanks! I switched to composedTarget but it would be great to have a test case that checks this. I am testing nested shadow roots in the mochitest attached in the last version of the patch, but this test works with both target or composedTarget. Can you share a scenario that requires composedTarget?

emilio: cf my question above try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6d0ff895721232fad0cbcf468fb621ddd7447a

Just creating a shadowRoot inside a shadow tree, like. <div id="outer"></div> outer.attachShadow({ mode: 'open' }).innerHTML = `<div></div>`; // target = outer; composedTarget = outer; let inner = outer.shadowRoot.firstElementChild; inner.attachShadow({ mode: 'open' }); // target = outer; composedTarget = inner; Should show the difference.

This scenario looks similar to my mochitest and running it in the browser BrowserContentToolbox (with `tabs[0].addEventListener("shadowrootattached", function (e) {tabs[0].content.console.log(e.target, e.composedTarget)})` to check the targets), I always get the same elements for both `target` and `composedTarget`

Oh, I guess since it's chrome-only dispatch we don't mungle the target as we walk up the target chain... never mind then, sorry for the alarm :)

Great, thanks for checking. Will revert to event.target then and submit for review. Thanks again for implementing this new event so quickly!

Comment on attachment 8987454 [details] Bug 1449959 - Listen to shadowrootattached events to update markup view; https://reviewboard.mozilla.org/r/252692/#review261090 What if a shadow root gets removed? Do our normal mutation observers figure that out and clean everything up properly? ::: devtools/client/inspector/markup/test/browser_markup_shadowdom_dynamic.js:95 (Diff revision 4) > + other-component > + slot2-1 > + slot1-1 > + slot1-2 > + inline text`; > + await assertMarkupViewAsTree(treeAfterTestAttach, "#root", inspector); The helper makes testing these scenarios a lot nicer!

Thanks for the review! My initial try run had a failure on debug mode for devtools/server/tests/browser/browser_webextension_inspected_window.js, can't reproduce locally so here is another push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=813d077273f8bff895cdf56f935a03e0d028eabd (In reply to Brian Grinstead [:bgrins] from comment #19) > Comment on attachment 8987454 [details] > Bug 1449959 - Listen to shadowrootattached events to update markup view; > > https://reviewboard.mozilla.org/r/252692/#review261090 > > What if a shadow root gets removed? Do our normal mutation observers figure > that out and clean everything up properly? > When I searched how I could delete or update existing shadow roots, I didn't find anything, so I assume this is not possible? Emilio, do you know if there is a way to detach or update shadow roots already attached?

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7037694f3ff51520929556163a10b937671f8b Alex: flagging you for review mainly about the usage of chromeEventHandler. In my first patch, I used to read this.targetActor.chromeEventHandler from the walkerActor in initialize() and destroy(). However this could fail during destroy() because the actorPool can destroy the targetActor before destroying the walkerActor. In this case calling this.targetActor.chromeEventHandler throws (docShell null). To workaround this I am keeping a reference on the chromeEventHandler in the walkerActor, but I'd prefer you to have a look at this and let me know if something looks bad to you or if there is a cleaner way to get access to chromeEventHandler here. Thanks!

Comment on attachment 8987454 [details] Bug 1449959 - Listen to shadowrootattached events to update markup view; https://reviewboard.mozilla.org/r/252692/#review261420 (In reply to Julian Descottes [:jdescottes][:julian] from comment #23) > However this could fail during destroy() because the actorPool can destroy > the targetActor before destroying the walkerActor. In this case calling > this.targetActor.chromeEventHandler throws (docShell null). To workaround > this I am keeping a reference on the chromeEventHandler in the walkerActor, Keeping a reference is fine. It may be useful to drop a comment in destroy about why the reference is stored. > but I'd prefer you to have a look at this and let me know if something looks > bad to you or if there is a cleaner way to get access to chromeEventHandler > here. Ideally we would fetch it from the targetActor's getter. I'm not expecting the chromeEventHandler to change on iframe switching, that's why it is fine keeping a reference to it. But other similar getters on target actor do change: docShell, docShells, window, ... So it can be confusing to start storing reference like this. Otherwise, I'm not sure about the precise race you are hitting. * `_detach` is called before we nullify the `docShell` attribute from the actor: https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#522-527 * we do destroy the actors pool from `_detach`: https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#918-921 * note that `_detach` already handles the case where `docShell` is already null: https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#896-898 The comment suggest this is a race where the docshell is already destroyed, and so there is no value in unregistering the event listener. Such race is different from a race within actor destroy codepath.

Thanks for the review! Some details about the race condition. I triggered it when running devtools/server/tests/browser/browser_accessibility_node.js (also applies to other tests from this folder). The issues is triggered when closing the client https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/devtools/server/tests/browser/browser_accessibility_node.js#53 This leads to destroying all actors in the actorPool. First the targetActor, with the following stack trace: exit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/targets/browsing-context.js:512:17 frameTargetPrototype.exit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/targets/frame.js:80:3 destroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/targets/browsing-context.js:504:5 removeActor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/common.js:241:7 APDestroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/common.js:204:7 onClosed/<@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1882:35 onClosed@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1882:5 close@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:731:7 close@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1493:7 onDisconnect<@resource://devtools/server/startup/frame.js:121:7 exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14 MessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:124:5 @resource://devtools/server/startup/frame.js:19:4 Then the walker actor: destroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/walker.js:223:17 destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:928:9 destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1001:5 destroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/inspector.js:87:5 removeActor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/common.js:241:7 APDestroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/common.js:204:7 onClosed/<@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1882:35 onClosed@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1882:5 close@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:731:7 close@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1493:7 onDisconnect<@resource://devtools/server/startup/frame.js:121:7 exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14 MessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:124:5 @resource://devtools/server/startup/frame.js:19:4 And as I said, I used to call this.targetActor.chromeEventHandler in the destroy of the WalkerActor, but since we have already called browsing-context::exit() (which sets docShell to null at https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/devtools/server/actors/targets/browsing-context.js#524-527), the getter throws.

You can't remove a shadow root. We have a very special code path for the XML pretty-printer, but I don't think you should be concerned about it.

Ok, thanks for the confirmation Emilio! Added comments to walker.js, try is green, will land.

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3928fa2e88ee Listen to shadowrootattached events to update markup view;r=bgrins,ochameau

I have verified this fix on the 3 main OSes (Windows 10, Ubuntu 16.04 and Mac OS X 10.12.6) and the latest Nightly (v63.0a1 2018-07-05).

I can confirm this issue is fixed on beta, I verified using Firefox 63.0b3 on Windows 10 x64, link from comment 1, based on comment 33.