Closed Bug 1443923 Opened 7 years ago Closed 6 years ago

Provide an "Open in Debugger" link from a Custom Element node in the markup view to navigate to the Custom Element class definition

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- verified

People

(Reporter: bgrins, Assigned: jdescottes)

References

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 2 obsolete files)

Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition;
(deleted), text/x-review-board-request
bgrins
: review+
Details
show_element_definition.gif
(deleted), image/gif
Details
custom-element-badge.png
(deleted), image/png
Details
Bug 1443923 - part1: Return script location for shadow host in NodeActor form;
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1443923 - part3: Add custom badge in markup view to open custom element definition;
(deleted), text/x-review-board-request
bgrins
: review+
Details
icons instead of badges
(deleted), image/png
Details
Bug 1443923 - part6: Update markup view when a custom element is defined;
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item;
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1443923 - part4: Expose isCustomElementName to DevTools via InspectorUtils;
(deleted), text/x-review-board-request
emilio
: review+
Details
Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools;
(deleted), text/x-review-board-request
masayuki
: review+
Details
When we encounter a Custom Element in the markup view, it would be helpful to link to the class definition in the debugger to see what that element is doing and set breakpoints. For example, with this page: data:text/html,<my-element><script>customElements.define('my-element', class MyElement extends HTMLElement { connectedCallback() { this.innerHTML='connected'; }})</script> We see a markup tree like: <html> <body> <my-element>connected</my-element> </body> </html> I'd like to have an "Open in Debugger" link somewhere next to the element similar to the event listeners popup that would take you the `class MyElement extends HTMLElement` line.
We can get a reference to the class by querying the Custom Element Registry, for example: `customElements.get('my-element')`
Severity: normal → enhancement
Priority: -- → P3
Product: Firefox → DevTools
Priority: P3 → P2
Here is a first implementation, that simply adds a context menu element for shadow hosts for which we can find a definition to jump to. Still needs: - tests - discussion around the menu-item name and position In a next step we will probably want to surface this in a more visible way, but I figured a context menu item would be a good start. Note: I was playing with this feature on https://shop.polymer-project.org/ and most custom element definitions can't be jumped to. For instance `window.customElements.get("app-header")`. Logging this in the console also displays a function which is not linked to the debugger. On https://www.polymer-project.org/, most components jump to a generic adapter file, hiding the "real" component definition. The point is that we might have to do the same as what we do for events, and try to find the real component definition for frameworks such as polymer.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached image show_element_definition.gif (deleted) —
> On https://www.polymer-project.org/, most components jump to a generic adapter file, hiding the "real" component definition. The point is that we might have to do the same as what we do for events, and try to find the real component definition for frameworks such as polymer. I assume this doesn't apply to Firefox frontend development (web payments started using it)? How much can we re-use of the existing event mapping here? Or would we recreate a lot of the infrastructure? Web Components developers tend to add semantic sugar that can obscure the real definitions; so it seems be important on first sight. > In a next step we will probably want to surface this in a more visible way, but I figured a context menu item would be a good start. Since it will not show up everywhere, optional menu elements would be near impossible to discover. We should make this more visible and give it some prime real estate with a badge (similar to grid & evt). My strawperson proposal would be the label `custom` and title `Open Custom Element Definition`. Adding a `…` to the `custom` label might also be appropriate as the click opens a new panel. ni? Martin for input on if a badge is appropriate here with the current & future use of badges in mind.
Flags: needinfo?(mbalfanz)
Attached image custom-element-badge.png (deleted) —
I quickly mocked it up in an "extreme" scenario where we could have badges for a custom element, event(s) and a layout type (flex in this case). I feel that things can get a little busy. All badges are semantically different, but would currently look the same. This way the user looses the ability to quickly scan for information. If we want to have different types of badges I think we need to differentiate them visually as well. Another idea would be to make it more customizable what is displayed. I imagine something similar to the filters in console, where we can check/uncheck different kinds of information. Victoria: I'd appreciate if you jump in on this :-)
Flags: needinfo?(mbalfanz) → needinfo?(victoria)
(In reply to :Harald Kirschner :digitarald from comment #7) > > On https://www.polymer-project.org/, most components jump to a generic adapter file, hiding the "real" component definition. The point is that we might have to do the same as what we do for events, and try to find the real component definition for frameworks such as polymer. > > I assume this doesn't apply to Firefox frontend development (web payments > started using it)? > Probably not. To be honest, beyond the polymer test website, I don't have a good idea of how people use/want-to-use webcomponents right now. > How much can we re-use of the existing event mapping here? Or would we > recreate a lot of the infrastructure? Web Components developers tend to add > semantic sugar that can obscure the real definitions; so it seems be > important on first sight. > I think the current event mapping won't help. It's mostly a collection of framework-specific methods to find the real event listeners for some popular frameworks/libraries ("if react...", "if jquery..."). > > In a next step we will probably want to surface this in a more visible way, but I figured a context menu item would be a good start. > > Since it will not show up everywhere, optional menu elements would be near > impossible to discover. We should make this more visible and give it some > prime real estate with a badge (similar to grid & evt). I agree that the context-menu item is not great for discoverability. Maybe that can be enough internally for other teams (moving XBL to webcomponents for instance). And I share the concerns expressed by Martin about having too many badges with different behaviors etc... I think this concern was already present regardless of this bug. Maybe the badges need to be visually different, maybe they need to be stacked, filtered... For now I implemented both, context-menu item and badge (with "custom…". But I think it might make sense to split this in two and decide about the badge in a separate bug. On a separate note, I really like having the badge because it also shows that the node is a custom-element, without having to check if there is a shadow root inside.
(In reply to :Harald Kirschner :digitarald from comment #7) > > On https://www.polymer-project.org/, most components jump to a generic adapter file, hiding the "real" component definition. The point is that we might have to do the same as what we do for events, and try to find the real component definition for frameworks such as polymer. > > I assume this doesn't apply to Firefox frontend development (web payments > started using it)? It should work properly on web payments and other code that's just doing customElements.define() directly. We can confirm this by testing on <xul:deck> and <xul:stringbundle> in the Browser Toolbox, both of which are Custom Elements. If we get it working with vanilla JS that will be a great first step.
Attached image icons instead of badges (deleted) —
Building on Martin's mockup, could we replace the 'custom' badges with the jump-to-debugger icon (or other icon)? (attached, top image) I'm currently not too worried about badge overload since they all tend to reveal some unique/important functionality and I'm assuming it's not often that they show up on the same element. But, I'm keeping this icon idea in mind as a possibility for flex/grid as well. (bottom image)
Flags: needinfo?(victoria)
The new version attached here could in theory show links for xul:deck. However, MozDeck is defined as a class without constructor so we hit the following issue https://bugzilla.mozilla.org/show_bug.cgi?id=1473272. This version handles custom elements defined without shadow dom. However the markup view is not updated when customElements.define is called. So for now this only works in "static" scenarios, where custom elements are defined before the markup view is displayed. To handle dynamic scenarios, we either need to listen to customElements.isDefined for each new tag name we encounter https://searchfox.org/mozilla-central/source/dom/webidl/CustomElementRegistry.webidl#15 (and send a mutation to all impacted nodes). Or get a new event from platform, direcly linked to the node.
Implemented a solution based on customElements.whenDefined() to react to new custom element definitions. Seems to work fine. I pushed the changes to talos: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f210747fa64269455b091fff937c1bfea232f3bb&newProject=try&newRevision=f564fea57a67354af45a03cc272afa9394308c38&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 There is a 3% regression on expand manychildren, but note that I also did a push without the 4th changeset (so without attaching whenDefined on any new tagname), and it shows the same perf regression. So that's coming from one of the first 3 changesets.
Blocks: 1476235
Comment on attachment 8990492 [details] Bug 1443923 - part6: Update markup view when a custom element is defined; https://reviewboard.mozilla.org/r/255564/#review264530 ::: devtools/server/actors/inspector/walker.js:292 (Diff revision 2) > this._activePseudoClassLocks.has(actor)) { > this.clearPseudoClassLocks(actor); > } > + > + if (this._monitoredNames.has(actor.rawNode.localName)) { > + this._monitoredNames.get(actor.rawNode.localName).delete(actor); Is there a way to unbind whenDefined()? Seems like we could leave quite a number of hanging listeners around when nodes get deleted or when devtools closes. Maybe check with Emilio for risk here and potentially other options (maybe a single event that gets called with any newly-defined tag name as an arg)? ::: devtools/server/actors/inspector/walker.js:376 (Diff revision 2) > + * not match an already existing custom element. > + */ > + isValidCustomElementName: function(name, customElements) { > + try { > + // True if name is not already used. > + return !customElements.get(name); Do we actually throw on invalid names? It doesn't mention throwing in https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-get, and I can't reproduce the throwing behavior locally. Instead of catching an error, we may need to expose `nsContentUtils::IsCustomElementName`, although doing `name.includes("-")` may be a quick enough approximation (assuming that we properly clean up listeners when we are done).
Attachment #8990492 - Flags: review?(bgrinstead)
Comment on attachment 8989697 [details] Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition; https://reviewboard.mozilla.org/r/254708/#review264524 Code changes look fine ::: commit-message-b1b87:1 (Diff revision 7) > +Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition;;r=bgrins nit: there are two semicolons at the end ::: devtools/client/locales/en-US/inspector.properties:372 (Diff revision 7) > > +# LOCALIZATION NOTE (inspectorCustomElementDefinition.label): This is the label > +# shown in the inspector contextual-menu for custom elements to which a shadow root has > +# been attached. Clicking on the menu item will open the Debugger on the custom element > +# definition location. > +inspectorCustomElementDefinition.label=Show Element Definition Here we say "Show Element Definition" but in the tooltiptext on the badge we say: "Open custom-element definition" I'd like to unify on this if we can. A couple of notes: - Show vs Open. Maybe "Show" is better since we already use "Show DOM Properties" - Other context menu items use "Node" instead of "Element". I think I'd rather get the term "Custom Element" in place as opposed to "definition", if space is a concern in the context menu. Here's what I'm thinking (feel free to suggest changes): Context menu: "Show Custom Element" Tooltip text: "Show Custom Element definition"
Attachment #8989697 - Flags: review?(bgrinstead) → review+
Comment on attachment 8990236 [details] Bug 1443923 - part3: Add custom badge in markup view to open custom element definition; https://reviewboard.mozilla.org/r/255264/#review264528 ::: commit-message-9ced5:1 (Diff revision 4) > +Bug 1443923 - part3: Add custom badge in markup view to open custom-element definition;r=bgrins Nit: "custom element" ::: devtools/client/inspector/markup/views/element-editor.js:195 (Diff revision 4) > this.elt.appendChild(this.displayBadge); > + > + this.customBadge = this.doc.createElement("div"); > + this.customBadge.classList.add("markup-badge"); > + this.customBadge.dataset.custom = "true"; > + this.customBadge.textContent = "custom…"; This is intended to be unlocalized, right? ::: devtools/client/locales/en-US/inspector.properties:84 (Diff revision 4) > markupView.event.tooltiptext=Event listener > > +# LOCALIZATION NOTE (markupView.custom.tooltiptext) > +# Used in a tooltip that appears when the user hovers over 'custom' badge in > +# the markup view. Only displayed on custom elements with a shadow root attached. > +markupView.custom.tooltiptext=Open custom-element definition Nit: I think this should be "custom element" and we should probably have a localization note to not translate that term since it's jargon.
Attachment #8990236 - Flags: review?(bgrinstead) → review+
Comment on attachment 8990235 [details] Bug 1443923 - part1: Return script location for shadow host in NodeActor form; https://reviewboard.mozilla.org/r/255262/#review264534
Attachment #8990235 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #29) > Comment on attachment 8990492 [details] > Bug 1443923 - part4: Update markup view when a customElement is defined; > > https://reviewboard.mozilla.org/r/255564/#review264530 > > ::: devtools/server/actors/inspector/walker.js:292 > (Diff revision 2) > > this._activePseudoClassLocks.has(actor)) { > > this.clearPseudoClassLocks(actor); > > } > > + > > + if (this._monitoredNames.has(actor.rawNode.localName)) { > > + this._monitoredNames.get(actor.rawNode.localName).delete(actor); > > Is there a way to unbind whenDefined()? Seems like we could leave quite a > number of hanging listeners around when nodes get deleted or when devtools > closes. Maybe check with Emilio for risk here and potentially other options > (maybe a single event that gets called with any newly-defined tag name as an > arg)? > Emilio: do you think we could have an event to be notified when a new custom element is defined, similar to what you provided us for new shadow-roots attached? If it's not possible can you check the implementation here relying on whenDefined() and give some feedback? Thanks!
Flags: needinfo?(emilio)
I'm not that familiar with custom elements, but yeah, it should be straight-forward adding the same block of code with a different event name in: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/dom/base/CustomElementRegistry.cpp#959 Not sure whether that's better than whenDefined(), probably Olli would be the person to weigh-in. There's the chance of a lot of promises getting created and kept alive in the global, but I don't expect too many valid custom-element names to be used for non-custom elements? In any case it may be worth landing this using whenDefined for now and do the event dance if it becomes a perf issue, I'd think.
Flags: needinfo?(emilio)
I'd like to be sure what the behavior is when one of the whenDefined promise callbacks fires after the devtools toolbox has been closed and connection torn down. Does it throw in the actor / protocol?
(In reply to Emilio Cobos Álvarez (:emilio) from comment #34) > > In any case it may be worth landing this using whenDefined for now and do > the event dance if it becomes a perf issue, I'd think. Thanks for the feedback Emilio. I realized that my previous implementation was not working when using several windows, each with their own custom element registry, so the last implementation fixes that. I also isolated all this code to a dedicated class, so that it will be easier to remove it if/when we get an event for custom element definitions. Note that even if we get an event like that, we would need the event to be fired for each element/node to which the new definition applies. I added a last patch with an attempt to expose IsCustomElementName.
(In reply to Brian Grinstead [:bgrins] from comment #40) > I'd like to be sure what the behavior is when one of the whenDefined promise > callbacks fires after the devtools toolbox has been closed and connection > torn down. Does it throw in the actor / protocol? Good catch, we get an unhandled rejection: JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/custom-element-watcher.js, line 91: TypeError: this.watchedRegistries is null JavaScript error: , line 0: Promise rejection value is a non-unwrappable cross-compartment wrapper. Will fix this
(In reply to Julian Descottes [:jdescottes][:julian] from comment #41) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #34) > > > > In any case it may be worth landing this using whenDefined for now and do > > the event dance if it becomes a perf issue, I'd think. > > Thanks for the feedback Emilio. > > I realized that my previous implementation was not working when using > several windows, each with their own custom element registry, so the last > implementation fixes that. I also isolated all this code to a dedicated > class, so that it will be easier to remove it if/when we get an event for > custom element definitions. Note that even if we get an event like that, we > would need the event to be fired for each element/node to which the new > definition applies. Having an event emitted on the node itself does seem like the least amount of devtools code required. Alternatively if that is too complex on the platform side, couldn't we keep a Map of localName -> Set(Node) similar to what you are doing with registryMap in the current patch, and swap out the whenDefined handler with an event emitted on the window/document? That would still prevent the leaky callback that gets fired once the rest of devtools is torn down.
(In reply to Brian Grinstead [:bgrins] from comment #43) > (In reply to Julian Descottes [:jdescottes][:julian] from comment #41) > > (In reply to Emilio Cobos Álvarez (:emilio) from comment #34) > > > > > > In any case it may be worth landing this using whenDefined for now and do > > > the event dance if it becomes a perf issue, I'd think. > > > > Thanks for the feedback Emilio. > > > > I realized that my previous implementation was not working when using > > several windows, each with their own custom element registry, so the last > > implementation fixes that. I also isolated all this code to a dedicated > > class, so that it will be easier to remove it if/when we get an event for > > custom element definitions. Note that even if we get an event like that, we > > would need the event to be fired for each element/node to which the new > > definition applies. > > Having an event emitted on the node itself does seem like the least amount > of devtools code required. Alternatively if that is too complex on the > platform side, couldn't we keep a Map of localName -> Set(Node) similar to > what you are doing with registryMap in the current patch, and swap out the > whenDefined handler with an event emitted on the window/document? That would > still prevent the leaky callback that gets fired once the rest of devtools > is torn down. Firing an element on the global when customElements.define is called is doable (and pretty easy). Firing it for each upgrade may be too costly (you need to call it even from the parser, which is not great)
Attachment #8993609 - Flags: feedback?(emilio)
Attachment #8993353 - Flags: feedback?(emilio)
Emilio, I tried to tackle the patches to expose IsCustomElementName and to add a new "customelementdefined" event. I know almost nothing about C++ so they might be terrible :) Let me know if it's worth trying to land here, or if I should rather open follow-up bugs and defer to you & Olli.
Comment on attachment 8993353 [details] Bug 1443923 - part4: Expose isCustomElementName to DevTools via InspectorUtils; Looks fine. Maybe you want to use a namespace URI as a string or something instead of a boolean? Though perhaps the boolean is fine for now. No big deal either way.
Attachment #8993353 - Flags: feedback?(emilio) → feedback+
Comment on attachment 8993609 [details] Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools; https://reviewboard.mozilla.org/r/258308/#review265358 ::: dom/base/CustomElementRegistry.cpp:972 (Diff revision 1) > + > + JS::Rooted<JS::Value> detail(aCx, JS::StringValue(nameJsStr)); > + RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, nullptr); > + event->InitCustomEvent(aCx, > + NS_LITERAL_STRING("customelementdefined"), > + /* aCanBubble = */ true, You can use the `CanBubble` and `Canelable` enums instead fwiw.
Comment on attachment 8993609 [details] Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools; Hmm, I'm not really sure what's the best way to do this. Given Olli is on vacation, Masayuki, mind taking a look at this and seeing if there's a better way of exposing the defined custom element name?
Attachment #8993609 - Flags: feedback?(emilio) → feedback?(masayuki)
Comment on attachment 8993609 [details] Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools; https://reviewboard.mozilla.org/r/258308/#review265378 Hmm, looks like no better way to create custom element with detail value. Using XPCOM listener would be better for performance, but I guess that not using XPCOM is better. ::: dom/base/CustomElementRegistry.cpp:969 (Diff revision 1) > + RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, nullptr); > + event->InitCustomEvent(aCx, Looks like that this creates untrusted event since you don't create trusted WidgetEvent here. See: https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/dom/events/Event.cpp#60,62,77,81,107 Perhaps, it does not matter for now, but it looks odd.
Thank you for the feedback! (In reply to Emilio Cobos Álvarez (:emilio) from comment #51) > Comment on attachment 8993353 [details] > Bug 1443923 - part6: Expose isCustomElementName to DevTools via > InspectorUtils; > > Looks fine. Maybe you want to use a namespace URI as a string or something > instead of a boolean? Though perhaps the boolean is fine for now. No big > deal either way. Sounds like a good move, but I couldn't figure out a good way to convert from a URI as a string to the value expected by nsContentUtils. The only conversion code I found was https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/base/nsDOMAttributeMap.cpp#373-383 . I am not sure this is worth the added complexity. Any simpler way I could handle this? (In reply to Emilio Cobos Álvarez (:emilio) from comment #52) > Comment on attachment 8993609 [details] > Bug 1443923 - part8: Emit chrome-only event customelementdefined for > DevTools; > > https://reviewboard.mozilla.org/r/258308/#review265358 > > ::: dom/base/CustomElementRegistry.cpp:972 > (Diff revision 1) > > + > > + JS::Rooted<JS::Value> detail(aCx, JS::StringValue(nameJsStr)); > > + RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, nullptr); > > + event->InitCustomEvent(aCx, > > + NS_LITERAL_STRING("customelementdefined"), > > + /* aCanBubble = */ true, > > You can use the `CanBubble` and `Canelable` enums instead fwiw. I tried using CanBubble::eYes, but got compilation errors such as `cannot initialize a parameter of type 'bool' with an rvalue of type 'mozilla::CanBubble'`. (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #54) > Comment on attachment 8993609 [details] > Bug 1443923 - part8: Emit chrome-only event customelementdefined for > DevTools; > > https://reviewboard.mozilla.org/r/258308/#review265378 > > Hmm, looks like no better way to create custom element with detail value. > > Using XPCOM listener would be better for performance, but I guess that not > using XPCOM is better. > > ::: dom/base/CustomElementRegistry.cpp:969 > (Diff revision 1) > > + RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, nullptr); > > + event->InitCustomEvent(aCx, > > Looks like that this creates untrusted event since you don't create trusted > WidgetEvent here. > > See: > https://searchfox.org/mozilla-central/rev/ > 8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/dom/events/Event.cpp#60,62,77,81,107 > Perhaps, it does not matter for now, but it looks odd. Thanks! I am now setting setTrusted(true), as I have seen it done at https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/xml/nsXMLPrettyPrinter.cpp#192-196 . Let me know if there is a better way to do that. Note that both changes get indirectly tested via the mochitest from part5, but I can add dedicated tests if needed.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #59) > Thank you for the feedback! > > (In reply to Emilio Cobos Álvarez (:emilio) from comment #51) > > Comment on attachment 8993353 [details] > > Bug 1443923 - part6: Expose isCustomElementName to DevTools via > > InspectorUtils; > > > > Looks fine. Maybe you want to use a namespace URI as a string or something > > instead of a boolean? Though perhaps the boolean is fine for now. No big > > deal either way. > > Sounds like a good move, but I couldn't figure out a good way to convert > from a URI as a string to the value expected by nsContentUtils. The only > conversion code I found was > https://searchfox.org/mozilla-central/rev/ > ad36eff63e208b37bc9441b91b7cea7291d82890/dom/base/nsDOMAttributeMap.cpp#373- > 383 . I am not sure this is worth the added complexity. Any simpler way I > could handle this? Most code just calls RegisterNamespace() directly, see all the GetElementsByTagNameNS, etc... In any case, if you feel it's not worth the complexity, that's fine. > (In reply to Emilio Cobos Álvarez (:emilio) from comment #52) > > Comment on attachment 8993609 [details] > > Bug 1443923 - part8: Emit chrome-only event customelementdefined for > > DevTools; > > > > https://reviewboard.mozilla.org/r/258308/#review265358 > > > > ::: dom/base/CustomElementRegistry.cpp:972 > > (Diff revision 1) > > > + > > > + JS::Rooted<JS::Value> detail(aCx, JS::StringValue(nameJsStr)); > > > + RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, nullptr); > > > + event->InitCustomEvent(aCx, > > > + NS_LITERAL_STRING("customelementdefined"), > > > + /* aCanBubble = */ true, > > > > You can use the `CanBubble` and `Canelable` enums instead fwiw. > > I tried using CanBubble::eYes, but got compilation errors such as `cannot > initialize a parameter of type 'bool' with an rvalue of type > 'mozilla::CanBubble'`. Ah, right, looks like I didn't overload InitCustomEvent... no big deal anyway.
Comment on attachment 8993353 [details] Bug 1443923 - part4: Expose isCustomElementName to DevTools via InspectorUtils; https://reviewboard.mozilla.org/r/258126/#review265620 ::: layout/inspector/InspectorUtils.h:281 (Diff revision 3) > > + /** > + * Check if the provided name can be custom element name. > + * > + * @param DOMString aName > + * @param boolean aIsXULNameSpace I don't think the `@param` annotations are useful, given the type already includes the comment, I'd remove them but feel free to leave them if you want. ::: layout/inspector/InspectorUtils.cpp:737 (Diff revision 3) > nsIPresShell::SCROLL_OVERFLOW_HIDDEN); > } > > + > +bool > +InspectorUtils::IsCustomElementName(GlobalObject& aGlobalObject, nit: I wouldn't add the `aGlobalObject` name given it's unused, that is: ``` bool InspectorUtils::IsCustomElementName(GlobalObject&, const nsAString aName, bool aIsXULNameSpace) { ... } ``` And maybe the same on the declaration, there's no point in giving it a name which is a subset of the name of the type imo. But not a big deal either way :)
Attachment #8993353 - Flags: review?(emilio) → review+
> I am now setting setTrusted(true), as I have seen it done at > https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/xml/nsXMLPrettyPrinter.cpp#192-196 > . Let me know if there is a better way to do that. Another possible way is, like to do this: WidgetEvent widgetEvent(true, eVoidEvent); RefPtr<CustomEvent> event = NS_NewDOMCustomEvent(doc, nullptr, &widgetEvent); event->InitCustomEvent(aCx, NS_LITERAL_STRING("customelementdefined"), /* CanBubble */ true, /* Cancelable */ true, detail); If this works, Event::ConstructorInit() does not need to create WidgetEvent instance in the heap: https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/events/Event.cpp#60,77,81,107 *However*, looks like that this is not available with AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) because this does not move Event::mEvent from stack to the heap with calling Event::DuplicatePrivateData(): https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/events/AsyncEventDispatcher.h#97-103 https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/events/Event.cpp#526-538 I guess that this is a bug of AsyncEventDispatcher because there is no MOZ_ASSERT nor comment about this. So, current your approach must be the only way here.
Comment on attachment 8993609 [details] Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools; https://reviewboard.mozilla.org/r/258308/#review265622 So, looks fine to me.
Attachment #8993609 - Flags: review?(masayuki) → review+
Thanks for the reviews! Applied the suggestions and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14689efd95185d2829cc771d50d2eca0e462dc0
Comment on attachment 8993608 [details] Bug 1443923 - part7: Use InspectorUtils.isCustomElementName in inspector actor; https://reviewboard.mozilla.org/r/258306/#review265774 ::: devtools/server/actors/inspector/custom-element-watcher.js:81 (Diff revision 5) > } > } > > _shouldWatchDefinition(nodeActor) { > const doc = nodeActor.rawNode.ownerDocument; > - if (!doc) { > + if (!doc || !doc.documentElement) { Is this an actual case we can hit? I'm fine with leaving the check in but I doubt we consistently check that throughout serverside code, so we should audit callers in another bug if there's a test case to hit this.
Attachment #8993608 - Flags: review?(bgrinstead) → review+
Comment on attachment 8992303 [details] Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item; https://reviewboard.mozilla.org/r/257170/#review265776 ::: devtools/server/actors/inspector/custom-element-watcher.js:33 (Diff revision 4) > */ > this.watchedRegistries = new WeakMap(); > } > > destroy() { > + this.destroyed = true; If we folded together the event listening into the initial implementation (part 4), then I don't think we'd need to handle the case where we receive `_onCustomElementDefined`, right?
Comment on attachment 8990492 [details] Bug 1443923 - part6: Update markup view when a custom element is defined; https://reviewboard.mozilla.org/r/255564/#review265778 ::: devtools/server/actors/inspector/custom-element-watcher.js:53 (Diff revision 3) > + const registryMap = this.watchedRegistries.get(registry); > + > + const name = nodeActor.rawNode.localName; > + if (!registryMap.has(name)) { > + // Add a listener to be notified if this name gets a custom element definition. > + registry.whenDefined(name).then(() => this._onCustomElementDefined(name, registry)); Can you fold part 9 into this so we don't have an intermediate state where we have dangling promises and workaround for them (i.e. in part 5)? I realize it's a bit of pain as we also need to move part 8 up in front of the now-folded-together 4/9, but I think it'll lead to a cleaner history and less cases to handle.
Comment on attachment 8993610 [details] Bug 1443923 - part9: Listen to "customelementdefined" in inspector actor; https://reviewboard.mozilla.org/r/258310/#review265780 See comment on part 4 - this looks totally fine, but I'd like to have one more look together with that patch.
Attachment #8993610 - Flags: review?(bgrinstead)
Comment on attachment 8992303 [details] Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item; https://reviewboard.mozilla.org/r/257170/#review265784 Thanks! Will take one more look at this in the next version ::: devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger.js:93 (Diff revision 4) > + > + // We have to wait until the debugger has fully loaded the source otherwise > + // we will get unhandled promise rejections. > + info("Wait until source is loaded in the debugger"); > + await waitForLoadedSource(debuggerContext, "data:"); > + await waitForTime(1000); What's this timeout for? Is there maybe an event we can listen to instead? ::: devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger.js:105 (Diff revision 4) > + ok(menuItem, selector + ": The menu item was found in the contextual menu"); > + ok(!menuItem.disabled, selector + ": The menu item is not disabled"); > + > + info("Click on `Jump to Definition` and verify that the debugger opens."); > + onDebuggerReady = toolbox.getPanelWhenReady("jsdebugger"); > + inspector.jumpToCustomElementDefinition(); Any reason in particular to directly call jumpToCustomElementDefinition rather than clicking on the menuItem?
Attachment #8992303 - Flags: review?(bgrinstead)
Thanks for the review, some answers below. (In reply to Brian Grinstead [:bgrins] from comment #78) > Comment on attachment 8990492 [details] > > Can you fold part 9 into this so we don't have an intermediate state where > we have dangling promises and workaround for them (i.e. in part 5)? > > I realize it's a bit of pain as we also need to move part 8 up in front of > the now-folded-together 4/9, but I think it'll lead to a cleaner history and > less cases to handle. Also folded part 7. I agree about having a cleaner history. Was keeping it separated like that to be able to artifact-build up to part 5 at least :) > What's this timeout for? Is there maybe an event we can listen to instead? > Will add a comment, but the debugger can easily throw unhandled promise rejections until https://github.com/devtools-html/debugger.html/pull/6189 gets released. I thought waiting for the source to load was enough to avoid failures, but I still get some when running with --verify. I am pushing for the PR to finally be included in a debugger release with https://github.com/devtools-html/debugger.html/issues/6679 . > ::: > devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger. > js:105 > (Diff revision 4) > > + ok(menuItem, selector + ": The menu item was found in the contextual menu"); > > + ok(!menuItem.disabled, selector + ": The menu item is not disabled"); > > + > > + info("Click on `Jump to Definition` and verify that the debugger opens."); > > + onDebuggerReady = toolbox.getPanelWhenReady("jsdebugger"); > > + inspector.jumpToCustomElementDefinition(); > > Any reason in particular to directly call jumpToCustomElementDefinition > rather than clicking on the menuItem? I did that based on other markup view context-menu tests, but I'll try with click (non-artifact build in progress). (In reply to Brian Grinstead [:bgrins] from comment #76) > Comment on attachment 8993608 [details] > Bug 1443923 - part7: Use InspectorUtils.isCustomElementName in inspector > actor; > > https://reviewboard.mozilla.org/r/258306/#review265774 > > ::: devtools/server/actors/inspector/custom-element-watcher.js:81 > (Diff revision 5) > > } > > } > > > > _shouldWatchDefinition(nodeActor) { > > const doc = nodeActor.rawNode.ownerDocument; > > - if (!doc) { > > + if (!doc || !doc.documentElement) { > > Is this an actual case we can hit? I'm fine with leaving the check in but I > doubt we consistently check that throughout serverside code, so we should > audit callers in another bug if there's a test case to hit this. It is an issue with nodes contained in a <template> tag (actually makes some tests fail). In general, checking for an object before checking subproperties is not unexpected, but I will add a comment.
Attachment #8993608 - Attachment is obsolete: true
Attachment #8993610 - Attachment is obsolete: true
Comment on attachment 8989697 [details] Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition; https://reviewboard.mozilla.org/r/254708/#review264524 > Here we say "Show Element Definition" but in the tooltiptext on the badge we say: "Open custom-element definition" > > I'd like to unify on this if we can. A couple of notes: > - Show vs Open. Maybe "Show" is better since we already use "Show DOM Properties" > - Other context menu items use "Node" instead of "Element". > > I think I'd rather get the term "Custom Element" in place as opposed to "definition", if space is a concern in the context menu. Here's what I'm thinking (feel free to suggest changes): > > Context menu: "Show Custom Element" > Tooltip text: "Show Custom Element definition" Applied suggested changes but did not capitalize the tooltip text as suggested. Our tooltips usually only have the first word capitalized and "custom element" does not seem capitalized on its own (checked MDN for instance).
Comment on attachment 8990236 [details] Bug 1443923 - part3: Add custom badge in markup view to open custom element definition; https://reviewboard.mozilla.org/r/255264/#review264528 > This is intended to be unlocalized, right? Right, all of our badges are unlocalized at the moment. > Nit: I think this should be "custom element" and we should probably have a localization note to not translate that term since it's jargon. Regarding this particular item, I removed the hyphen. However "custom element" is localized on MDN (eg https://developer.mozilla.org/fr/docs/Web/Web_Components/Using_custom_elements "éléments personnalisés"), so I'm not 100% sure I should advise not to localize here :/ Will ask l10n peers.
Comment on attachment 8992303 [details] Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item; https://reviewboard.mozilla.org/r/257170/#review265784 > Any reason in particular to directly call jumpToCustomElementDefinition rather than clicking on the menuItem? Turns out clicking on the menu item seems to work. Not sure why other markupview tests are doing that.
Attachment #8989697 - Flags: review?(francesco.lodolo)
Comment on attachment 8990236 [details] Bug 1443923 - part3: Add custom badge in markup view to open custom element definition; Francesco, can you take a look at part 2 and 3 for the localization changes? In particular I am not sure whether we should ask localizers to keep "custom element" in English. Since it seems translated on MDN, I would say it should be translated in DevTools as well?
Attachment #8990236 - Flags: review?(francesco.lodolo)
Comment on attachment 8989697 [details] Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition; https://reviewboard.mozilla.org/r/254708/#review265974 Looking at the MDN page and https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element, I think it should be localizable.
Attachment #8989697 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8990236 [details] Bug 1443923 - part3: Add custom badge in markup view to open custom element definition; https://reviewboard.mozilla.org/r/255264/#review265976
Attachment #8990236 - Flags: review?(francesco.lodolo) → review+
Thanks Francesco! There are some leaks on debug builds, currently investigating those.
Comment on attachment 8990492 [details] Bug 1443923 - part6: Update markup view when a custom element is defined; https://reviewboard.mozilla.org/r/255564/#review266080 ::: devtools/server/actors/inspector/custom-element-watcher.js:80 (Diff revision 7) > + unmanageNode(nodeActor) { > + if (Cu.isDeadWrapper(nodeActor.rawNode)) { > + return; > + } > + > + const win = nodeActor.rawNode.ownerGlobal; What's the case where win is null here? And is it a case we need to handle in manageNode as well? ::: devtools/server/actors/inspector/custom-element-watcher.js:111 (Diff revision 7) > + } > + > + _onCustomElementDefined(event) { > + const doc = event.target; > + const registry = doc.defaultView.customElements; > + const registryMap = this.watchedRegistries.get(registry); This should check to make sure registryMap exists, right? It seems like it'd be possible to not yet have this registry in the watchedRegistry map if manageNode hasn't yet been called with the right kind of node.
Attachment #8990492 - Flags: review?(bgrinstead) → review+
Comment on attachment 8992303 [details] Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item; https://reviewboard.mozilla.org/r/257170/#review266088 ::: devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger.js:121 (Diff revision 8) > + > + // We have to wait until the debugger has fully loaded the source otherwise > + // we will get unhandled promise rejections. > + await waitForLoadedSource(debuggerContext, "data:"); > + > + // Have to wait until https://github.com/devtools-html/debugger.html/pull/6189 It looks like that https://github.com/devtools-html/debugger.html/pull/6189 merged at the beginning of May - is it not back in m-c? If not, when will it be?
Attachment #8992303 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #102) > Comment on attachment 8992303 [details] > Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item; > > https://reviewboard.mozilla.org/r/257170/#review266088 > > ::: > devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger. > js:121 > (Diff revision 8) > > + > > + // We have to wait until the debugger has fully loaded the source otherwise > > + // we will get unhandled promise rejections. > > + await waitForLoadedSource(debuggerContext, "data:"); > > + > > + // Have to wait until https://github.com/devtools-html/debugger.html/pull/6189 > > It looks like that https://github.com/devtools-html/debugger.html/pull/6189 > merged at the beginning of May - is it not back in m-c? If not, when will it > be? Whenever https://github.com/devtools-html/debugger.html/issues/6679 gets sorted out :)
(In reply to Brian Grinstead [:bgrins] from comment #101) > Comment on attachment 8990492 [details] > Bug 1443923 - part6: Update markup view when a custom element is defined; > > https://reviewboard.mozilla.org/r/255564/#review266080 > > ::: devtools/server/actors/inspector/custom-element-watcher.js:80 > (Diff revision 7) > > + unmanageNode(nodeActor) { > > + if (Cu.isDeadWrapper(nodeActor.rawNode)) { > > + return; > > + } > > + > > + const win = nodeActor.rawNode.ownerGlobal; > > What's the case where win is null here? And is it a case we need to handle > in manageNode as well? > I think this is also about elements in <template> tags. Which gets checked via node.ownerDocument.documentElement in manageNode, via shouldWatch[...]. Let's use something more explicit. > ::: devtools/server/actors/inspector/custom-element-watcher.js:111 > (Diff revision 7) > > + } > > + > > + _onCustomElementDefined(event) { > > + const doc = event.target; > > + const registry = doc.defaultView.customElements; > > + const registryMap = this.watchedRegistries.get(registry); > > This should check to make sure registryMap exists, right? It seems like it'd > be possible to not yet have this registry in the watchedRegistry map if > manageNode hasn't yet been called with the right kind of node. Good catch, fixed.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 702a0f22880a1c64ec6e02df7b459f4edb459dd2 -d 7cec8c9008b7: rebasing 474462:702a0f22880a "Bug 1443923 - part1: Return script location for shadow host in NodeActor form;r=bgrins" merging devtools/server/actors/inspector/node.js merging devtools/shared/fronts/node.js rebasing 474463:1f63cb7533d4 "Bug 1443923 - part2: Add inspector menu-item to jump to custom element definition;r=bgrins,flod" merging devtools/client/locales/en-US/inspector.properties rebasing 474464:001e45a0ee70 "Bug 1443923 - part3: Add custom badge in markup view to open custom element definition;r=bgrins,flod" merging devtools/client/locales/en-US/inspector.properties merging devtools/client/themes/markup.css rebasing 474465:7a66073b0a33 "Bug 1443923 - part4: Expose isCustomElementName to DevTools via InspectorUtils;r=emilio" merging layout/inspector/InspectorUtils.cpp rebasing 474466:707fe04a51da "Bug 1443923 - part5: Emit chrome-only event customelementdefined for DevTools;r=masayuki" merging dom/base/CustomElementRegistry.cpp rebasing 474467:a6593086980c "Bug 1443923 - part6: Update markup view when a custom element is defined;r=bgrins" merging devtools/server/actors/inspector/walker.js rebasing 474468:5a8609c72de9 "Bug 1443923 - part7: Add mochitest for custom badge and contextmenu item;r=bgrins" (tip) merging devtools/client/inspector/markup/test/browser.ini warning: conflicts while merging devtools/client/inspector/markup/test/browser.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ab697dedb5f part1: Return script location for shadow host in NodeActor form;r=bgrins https://hg.mozilla.org/integration/autoland/rev/807be50b8d33 part2: Add inspector menu-item to jump to custom element definition;r=bgrins,flod https://hg.mozilla.org/integration/autoland/rev/1afbc57f63c4 part3: Add custom badge in markup view to open custom element definition;r=bgrins,flod https://hg.mozilla.org/integration/autoland/rev/371903552cdd part4: Expose isCustomElementName to DevTools via InspectorUtils;r=emilio https://hg.mozilla.org/integration/autoland/rev/978445460a4a part5: Emit chrome-only event customelementdefined for DevTools;r=masayuki https://hg.mozilla.org/integration/autoland/rev/4c2375ea466f part6: Update markup view when a custom element is defined;r=bgrins https://hg.mozilla.org/integration/autoland/rev/a1b1af182dfa part7: Add mochitest for custom badge and contextmenu item;r=bgrins
Depends on: 1478665
No longer depends on: 1478665
Depends on: 1479470
Depends on: 1479505
I have reproduced this issue using Firefox 60.0a1 (2018.03.07) on Windows 10 x64. I can confirm this issue is fixed, I verified using Firefox 63.0b3 on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
Added a section to the detail page for the inspector: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Custom_element_definition I also added the following to the Firefox 63 release notes (since this feature was added in 63): The Page Inspector includes a link to the class definition for a custom element. ({{bug(1443923)}}).
Flags: needinfo?(jdescottes)
The updated MDN page looks good, thanks Irene!
Flags: needinfo?(jdescottes)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: