1483660 - Annotate UA Widget Shadow Root in DevTools Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Inspector currently considers UA Widget Shadow Root as normal, closed, Shadow Root. It's probably less confusing to the developers if

1) It's hidden behind a pref like devtools.inspector.showAllAnonymousContent
2) It is marked as "UA Shadow Root" or "UA Widget" or "generated" when shown.

We don't do (2) even for XBL anonymous content and native anonymous content though.

Comment 1

[Brian Grinstead [:bgrins]]

Agreed with (1) - we should hide this for most users.

Comment 2

[Brian Grinstead [:bgrins]]

Once Bug 1431255 lands, this should be visible with `data:text/html,<video controls />`

Comment 3

[ExE Boss]

I agree with (1) too, possibly calling the pref something like `devtools.inspector.showUserAgentShadowRoots` or something.

Comment 4

[Julian Descottes [:jdescottes]]

Chrome has a DevTools preference for this, and when enabled they show them as "#shadow-root (user agent)". I don't think we need to expose this in our Settings panel here.

As additional information, the shadowdom UA widgets are currently disabled even though Bug 1431255 landed. You need to flip dom.ua_widget.enabled to true to enable them.

The pref will be flipped to true for NIGHTLY in https://bugzilla.mozilla.org/show_bug.cgi?id=1484048
As long as it is Nightly-only I don't think we _have_ to fix it for 63, but nevertheless we should address this soon.

Comment 5

[Julian Descottes [:jdescottes]]

Tim: can you confirm there is no plan to turn on dom.ua_widget.enabled on beta and release in 63?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #5)
> Tim: can you confirm there is no plan to turn on dom.ua_widget.enabled on
> beta and release in 63?

No. I am hoping to enable it on Nightly 64.

Comment 7

[Julian Descottes [:jdescottes]]

Is there a way to differentiate a regular closed shadow root from a user-agent shadowroot?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I didn't expose the getter to JS, just the setter. It should be easy to expose a method on ShadowRoot and have it return ShadowRoot#mIsUAWidget.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Or perhaps changing this to a property so it's usable for both tests and devtools.

https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/dom/webidl/ShadowRoot.webidl#48-50

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Or not, because of bug 1484474.

Comment 11

[Julian Descottes [:jdescottes]]

Tim: we are checking if we should prioritize this bug in Fx64. Is there a chance that UA widgets will be flipped beyond Nightly and will ride the trains in 64? Can it slip to 65?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

At the current pace, I guess it would slip to 65. Will comment if otherwise.

Comment 13

[Julian Descottes [:jdescottes]]

Tim, I have a basic implementation ready but I have a couple questions:
- is there an added value to use a new pref instead of `devtools.inspector.showAllAnonymousContent`?
- is there any use case where a UA widget might have children that should be visible to a developer? Right now I simply return an empty children list as soon as I detect a UA widget.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1483660 - Expose ShadowRoot::isUAWidget() to chrome code;r=smaug

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1483660 - Hide UA Widget shadow root in inspector by default;r=ladybenko

Depends on D10536

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1483660 - Add mochitest for UA widgets in inspector;r=ladybenko

Depends on D10537

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1483660 - Add server test for UA widget support in inspector;r=ladybenko

Depends on D10538

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #13)
> Tim, I have a basic implementation ready but I have a couple questions:
> - is there an added value to use a new pref instead of
> `devtools.inspector.showAllAnonymousContent`?

I don't think there is other than naming, UA Widgets aren't exactly "anonymous content".

> - is there any use case where a UA widget might have children that should be
> visible to a developer? Right now I simply return an empty children list as
> soon as I detect a UA widget.

UA Widget Shadow DOM should be visible to the developer (when pref'd on) for Firefox development.

Over bug 1425874 I have over implemented <marquee> with UA Widget and <slot> so web developers should definitely be able to see the content of <marquee> slotted in.

Comment 19

[Julian Descottes [:jdescottes]]

Thanks Tim! I just changed the implementation in order keep regular children visible (don't know why I didn't think about that earlier but anyway).

This means that for the following authored markup

  <video controls>
    <div>some content</div>
  </video>

The markup view now shows

  devtools.inspector.showUserAgentShadowRoots = false:
  <video controls>
    <div>some content</div>
  </video>
 
  devtools.inspector.showUserAgentShadowRoots = true:
  <video controls>
    #shadow-root (closed)
    <div>some content</div>
  </video>

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can verify that the patch does what desired for video controls. Thank you so much!

I am going to build your patch on top of bug 1425874 to see if it behaves correctly.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... and <marquee> work as expected too.

Comment 22

[Julian Descottes [:jdescottes]]

thanks for testing Tim!

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

No worries, thank you for working on this. There is actually an improvement over XBL <marquee> since we would hide the web content child together with XBL anonymous content, while in UA Widget we hide the root but continue to list web content under it.

Comment 24

[Julian Descottes [:jdescottes]]

Will land the first patch and leave open, so that devtools peers can review the other patches and use artifact builds

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/648d56b39b4c
Expose ShadowRoot::isUAWidget() to chrome code;r=smaug

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/648d56b39b4c

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c5e2473ba3a
Hide UA Widget shadow root in inspector by default;r=ladybenko
https://hg.mozilla.org/integration/autoland/rev/f6ca26f35f11
Add mochitest for UA widgets in inspector;r=ladybenko
https://hg.mozilla.org/integration/autoland/rev/bd0cbc059da3
Add server test for UA widget support in inspector;r=ladybenko

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5c5e2473ba3a
https://hg.mozilla.org/mozilla-central/rev/f6ca26f35f11
https://hg.mozilla.org/mozilla-central/rev/bd0cbc059da3