877450 - Create Developer Tool widget with subview

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

We kind of decided to just open the Web Console when "Developer Tools" is dragged into the menu panel - but that was before we had the subview model. Now that we have a model for subview, we should use that for Developer tools.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Tentatively, this is a want for M7 - but it wouldn't be the end of the world if it slipped to M8.

Comment 2

[Mike de Boer [:mikedeboer]]

Moved to M? for re-triage

Comment 4

[Madhava Enros [:madhava]]

So, basically adding this list http://cl.ly/image/0d202R1Q1X1S as a subpanel? Any adjustments while we're in there?

Comment 5

[Dão Gottwald [:dao]]

Will add-ons be able to extend the list like they can currently add items to the Web Developer menu?

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #5)
> Will add-ons be able to extend the list like they can currently add items to
> the Web Developer menu?

We *could* implement it the same way as the Help subview, which simply copies the menu items over each time the subview is opened.

Comment 7

[Justin Dolske [:Dolske]]

I'm calling this P5 because comment 0 is a "we'd like to", and AFAIK we're ok with the current widget. If we actually _need_ to implement the subview, then this should be P1/P2.

Comment 8

[Dão Gottwald [:dao]]

We need the subview if we want to expose add-on items from the Web Developer menu. There's also the "Get More Tools" item that isn't exposed anywhere else.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Justin Dolske [:Dolske] from comment #7)
> I'm calling this P5 because comment 0 is a "we'd like to", and AFAIK we're
> ok with the current widget. If we actually _need_ to implement the subview,
> then this should be P1/P2.

As discussed on IRC a couple of days ago, not having a subview really isn't working. We need this fixed.

Comment 10

[Paul Rouget [:paul]]

Can the DevTools team help? (and what is a subview?)

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul Rouget [:paul] from comment #10)
> Can the DevTools team help? (and what is a subview?)

Yes, help would be appreciated :-). A subview is a simple menu that appears when the user clicks on the button in the menu panel. The menu slides over to reveal the submenu, which we are calling a subview.

If you run the UX Nightly, you can see one today by clicking on the History button in the menu panel.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I'll take a crack at this one today. I'm going to try to use the same approach that the Help menu uses.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Doing some self review and testing on Windows and OSX before requesting review.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 772754 [details] [diff] [review]
Patch v1

What do you think of this, Gijs?

Comment 15

[:Gijs (he/him)]

Comment on attachment 772754 [details] [diff] [review]
Patch v1

Review of attachment 772754 [details] [diff] [review]:
-----------------------------------------------------------------

I'm very happy you figured out a fix for the Firebug issues we discussed on IRC, but I became very nitpicky on the actual implementation, so feedback+ and let me know what you think of some of my points? I don't feel strongly about all of them, necessarily, but there were enough that I guess it makes sense to do another patch and see where we end up.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +48,5 @@
>        <vbox id="PanelUI-helpItems"/>
>      </panelview>
> +
> +    <panelview id="PanelUI-developer" flex="1">
> +      <label value="&webDeveloperMenu.label;"/>

I wonder if this is going to be an issue for localizers (reusing the same string in a different place). But we're doing this in various places, so let's hope it's OK unless told otherwise.

::: browser/components/customizableui/content/panelUI.js
@@ +190,5 @@
>        tempPanel.addEventListener("popuphidden", function panelRemover() {
>          tempPanel.removeEventListener("popuphidden", panelRemover);
>          tempPanel.removeEventListener("command", PanelUI._onWidgetPanelCommand);
> +        let evt = document.createEvent("CustomEvent");
> +        evt.initCustomEvent("ViewHiding", true, true, viewNode);

Nit: please use new CustomEvent("ViewHiding"), and only specify cancelable/bubbles on the parameter object if they're necessary. I'm not sure why we need to send viewNode if it's also the target of the event...

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +193,5 @@
> +    onViewShowing: function(aEvent) {
> +      // Populate the subview with whatever menuitems are in the developer
> +      // menu. We skip menu elements, because the menu panel has no way
> +      // of dealing with those right now.
> +      let doc = aEvent.target.ownerDocument;

You removed a lot of nullchecks. Why were those there and/or can we really get rid of them? :-)

@@ +196,5 @@
> +      // of dealing with those right now.
> +      let doc = aEvent.target.ownerDocument;
> +      let win = doc.defaultView;
> +
> +      let items = this.getElementsByTagName("vbox")[0];

Nit: This node has an ID. Why can't we use that? It's also the lastChild of the view node, and that's probably still better than getElementsByTagName()[0]...

@@ +200,5 @@
> +      let items = this.getElementsByTagName("vbox")[0];
> +      let menu = doc.getElementById("menuWebDeveloperPopup");
> +      let attrs = ["oncommand", "onclick", "label", "key", "disabled",
> +                   "command"];
> +      let menuItems = menu.querySelectorAll("menuitem,menuseparator");

This is recursive, so it'll extract menuitems/separators from submenus if they're included in the menu. Seeing as we're walking through this list later anyway, how about making that for loop just walk:

for (let node of menu.children) {
  if (node.hidden)
    continue;
  if (node.localName == "menuseparator") {
    item = doc.createElementNS(...);
  } else if (node.localName == "menuitem") {
    item = doc.createElementNS(...);
  } else {
    continue;
  }
  ...

That'll also save us a querySelectorAll call and I suspect it won't really be any slower.

@@ +213,5 @@
> +          item = doc.createElementNS(kNSXUL, "menuseparator");
> +        } else {
> +          item = doc.createElementNS(kNSXUL, "toolbarbutton");
> +        }
> +        for (let attrName of attrs) {

Nit: let attr of attrs or let attrName of attrNames

@@ +216,5 @@
> +        }
> +        for (let attrName of attrs) {
> +          if (!node.hasAttribute(attrName))
> +            continue;
> +          item.setAttribute(attrName, node.getAttribute(attrName));

Nit: I *think* this can be:

let attrVal = node.getAttribute(attrName);
if (attrVal)
  item.setAttribute(attrName, attrVal);

Which I find slightly neater, but hey, either works, and I'm not actually that fussed.

@@ +223,5 @@
>        }
> +      items.appendChild(fragment);
> +
> +      aEvent.originalTarget
> +            .addEventListener("command", win.PanelUI.onCommandHandler);

What's originalTarget here, and could we use plain target? Or is that a dumb idea?

@@ +228,5 @@
> +    },
> +    onViewHiding: function(aEvent) {
> +      let doc = aEvent.target.ownerDocument;
> +      let win = doc.defaultView;
> +      let items = this.getElementsByTagName("vbox")[0];

See above.

@@ +231,5 @@
> +      let win = doc.defaultView;
> +      let items = this.getElementsByTagName("vbox")[0];
> +      while (items.firstChild) {
> +        items.firstChild.remove();
> +      }

Hrm. Can we just items.remove(), and reappend? I think that might be slightly faster... but I'm not sure.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 772754 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 772754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +48,5 @@
> >        <vbox id="PanelUI-helpItems"/>
> >      </panelview>
> > +
> > +    <panelview id="PanelUI-developer" flex="1">
> > +      <label value="&webDeveloperMenu.label;"/>
> 
> I wonder if this is going to be an issue for localizers (reusing the same
> string in a different place). But we're doing this in various places, so
> let's hope it's OK unless told otherwise.
> 

Yep.

> ::: browser/components/customizableui/content/panelUI.js
> @@ +190,5 @@
> >        tempPanel.addEventListener("popuphidden", function panelRemover() {
> >          tempPanel.removeEventListener("popuphidden", panelRemover);
> >          tempPanel.removeEventListener("command", PanelUI._onWidgetPanelCommand);
> > +        let evt = document.createEvent("CustomEvent");
> > +        evt.initCustomEvent("ViewHiding", true, true, viewNode);
> 
> Nit: please use new CustomEvent("ViewHiding"), and only specify
> cancelable/bubbles on the parameter object if they're necessary. I'm not
> sure why we need to send viewNode if it's also the target of the event...
> 

Ok, switched to new CustomEvent constructor. I like that new way of doing it - didn't realize I was using the old-school method. :)

As for the viewNode detail - I don't recall why that's being passed. You're right - it's the target of the event, so it's not providing any new information. I've filed bug 891939 to investigate.

> ::: browser/components/customizableui/src/CustomizableWidgets.jsm
> @@ +193,5 @@
> > +    onViewShowing: function(aEvent) {
> > +      // Populate the subview with whatever menuitems are in the developer
> > +      // menu. We skip menu elements, because the menu panel has no way
> > +      // of dealing with those right now.
> > +      let doc = aEvent.target.ownerDocument;
> 
> You removed a lot of nullchecks. Why were those there and/or can we really
> get rid of them? :-)
> 

Not 100% sure. It seems overcautious to me - if an Event is fired and heard by onViewShowing, it must have had a target (in the case of onViewShowing and onViewHiding, that target would be a subview). And the ViewShowing and ViewHiding events should only be fired on items that are about to show, or about to hide - so they must still belong to a document. And that document must have a window, shouldn't it?

> @@ +196,5 @@
> > +      // of dealing with those right now.
> > +      let doc = aEvent.target.ownerDocument;
> > +      let win = doc.defaultView;
> > +
> > +      let items = this.getElementsByTagName("vbox")[0];
> 
> Nit: This node has an ID. Why can't we use that? It's also the lastChild of
> the view node, and that's probably still better than
> getElementsByTagName()[0]...
> 

Good call.

> @@ +200,5 @@
> > +      let items = this.getElementsByTagName("vbox")[0];
> > +      let menu = doc.getElementById("menuWebDeveloperPopup");
> > +      let attrs = ["oncommand", "onclick", "label", "key", "disabled",
> > +                   "command"];
> > +      let menuItems = menu.querySelectorAll("menuitem,menuseparator");
> 
> This is recursive, so it'll extract menuitems/separators from submenus if
> they're included in the menu.

D'oh! Good catch.

> Seeing as we're walking through this list
> later anyway, how about making that for loop just walk:
> 
> for (let node of menu.children) {
>   if (node.hidden)
>     continue;
>   if (node.localName == "menuseparator") {
>     item = doc.createElementNS(...);
>   } else if (node.localName == "menuitem") {
>     item = doc.createElementNS(...);
>   } else {
>     continue;
>   }
>   ...
> 
> That'll also save us a querySelectorAll call and I suspect it won't really
> be any slower.
> 

Yep, done.

> @@ +213,5 @@
> > +          item = doc.createElementNS(kNSXUL, "menuseparator");
> > +        } else {
> > +          item = doc.createElementNS(kNSXUL, "toolbarbutton");
> > +        }
> > +        for (let attrName of attrs) {
> 
> Nit: let attr of attrs or let attrName of attrNames
> 

Done.

> @@ +216,5 @@
> > +        }
> > +        for (let attrName of attrs) {
> > +          if (!node.hasAttribute(attrName))
> > +            continue;
> > +          item.setAttribute(attrName, node.getAttribute(attrName));
> 
> Nit: I *think* this can be:
> 
> let attrVal = node.getAttribute(attrName);
> if (attrVal)
>   item.setAttribute(attrName, attrVal);
> 
> Which I find slightly neater, but hey, either works, and I'm not actually
> that fussed.
> 

Sure, changed.

> @@ +223,5 @@
> >        }
> > +      items.appendChild(fragment);
> > +
> > +      aEvent.originalTarget
> > +            .addEventListener("command", win.PanelUI.onCommandHandler);
> 
> What's originalTarget here, and could we use plain target? Or is that a dumb
> idea?
> 

Target is just fine, I believe. Fixed.

> @@ +228,5 @@
> > +    },
> > +    onViewHiding: function(aEvent) {
> > +      let doc = aEvent.target.ownerDocument;
> > +      let win = doc.defaultView;
> > +      let items = this.getElementsByTagName("vbox")[0];
> 
> See above.
> 

Fixed.

> @@ +231,5 @@
> > +      let win = doc.defaultView;
> > +      let items = this.getElementsByTagName("vbox")[0];
> > +      while (items.firstChild) {
> > +        items.firstChild.remove();
> > +      }
> 
> Hrm. Can we just items.remove(), and reappend? I think that might be
> slightly faster... but I'm not sure.

Good idea.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Er, no idea why my generated patch swaps out all of panelUI.js....lemme fix that.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2

My editor was doing something strange. Fixed it.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 773362 [details] [diff] [review]
Patch v1.2

Ok, how's this?

Comment 20

[:Gijs (he/him)]

Comment on attachment 773362 [details] [diff] [review]
Patch v1.2

Review of attachment 773362 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Thanks! Landed on UX as https://hg.mozilla.org/projects/ux/rev/0f07f81c289f

Comment 22

[Tim Nguyen :ntim]

You should probably change the tooltip to Developer Tools, not Toggle Developer Tools right ?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to ntim007 from comment #22)
> You should probably change the tooltip to Developer Tools, not Toggle
> Developer Tools right ?

That's a good idea. I'll file a bug.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Filed bug 894375.

Comment 25

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/0f07f81c289f