Julian Descottes [:jdescottes] Assignee
	Description • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 2 • 6 years ago

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?

	Julian Descottes [:jdescottes] Assignee
	Comment 3 • 6 years ago

(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!

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 6 years ago

I filed bug 1470545.

	Julian Descottes [:jdescottes] Assignee
	Comment 7 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 8 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 10 • 6 years ago
mozreview-review

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 12 • 6 years ago
mozreview-review-reply

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?

	Julian Descottes [:jdescottes] Assignee
	Comment 13 • 6 years ago

emilio: cf my question above

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6d0ff895721232fad0cbcf468fb621ddd7447a

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 15 • 6 years ago

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`

	Emilio Cobos Álvarez (:emilio)
	Comment 16 • 6 years ago

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 :)

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 6 years ago

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

	Brian Grinstead [:bgrins]
	Comment 19 • 6 years ago
mozreview-review

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!

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 6 years ago

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?

	Julian Descottes [:jdescottes] Assignee
	Comment 23 • 6 years ago

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!

	Alexandre Poirot [:ochameau]
	Comment 24 • 6 years ago
mozreview-review

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 25 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 26 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 28 • 6 years ago

Ok, thanks for the confirmation Emilio!

Added comments to walker.js, try is green, will land.

	Pulsebot
	Comment 29 • 6 years ago

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

	Raul Gurzau (:RaulG)
	Comment 30 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3928fa2e88ee

	Cristina Coroiu [:ccoroiu]
	Comment 32 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3928fa2e88ee

	Daniel Bodea [:danibodea]
	Comment 33 • 6 years ago

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).

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 34 • 6 years ago

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.