1266419 - Create an HTML replacement for Toolbars and Toolbar buttons

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Toolbars and toolbar buttons are currently implemented using XUL markup. We want to change it and make sure the UI is generated using HTML elements.

In this case it's probably better to replace markup in-place. Introducing React components might be done later (in a follow up).

There are following toolbars related to this report:

- The main Toolbox toolbar (tabbar, but contains registered command buttons)
- Main Inspector panel toolbar (with search box)
- Rules side panel toolbar
- Computed side panel toolbar
- Animation side panel toolbar

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-frames-menu.patch

So, first I had to solve the drop down menu for selecting iframes. I used API implemented in bug 1257613. Brian, what do you think?

Further patch (converting XUL -> HTML) will yet follow.

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Here the the second part.

- Toolbox buttons are HTML
- Toolbox doc & close buttons are also HTML
- The Inspector buttons & toolbar are HTML

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Forgot to note, there is one known problem. The focus management works differently int HTML and XUL and I am not sure how to fix this. This test shows that (it fails).
devtools/client/framework/test/browser_toolbox_keyboard_navigation.js 

I am going to file a follow up for it and we should discuss how important this is (could be rather harder to fix).

Honza

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: html-icon-sizing.png

some of the UI differences I see with the patches applied

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: breadcrumbs-layout-browser-toolbox.png

I noticed this causes the breadcrumbs layout to get messed up when overflowing / inspecting large DOM.  You can see this if you inspect into a page like cnn.com, or if you inspect the toolbox with the Browser Toolbox.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Created attachment 8749270 [details]
> breadcrumbs-layout-browser-toolbox.png
> 
> I noticed this causes the breadcrumbs layout to get messed up when
> overflowing / inspecting large DOM.  You can see this if you inspect into a
> page like cnn.com, or if you inspect the toolbox with the Browser Toolbox.

This could be worked around by setting display: -moz-box on #inspector-breadcrumbs-toolbar until the breadcrumbs converted, but this also causes layout issues on all the tools (see webconsole toolbar, debugger toolbar, etc).  I'm thinking the 'flex' layout should be opt-in for now unless if we can tackle all of the layout issues here.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Created attachment 8749269 [details]
> html-icon-sizing.png
> 
> some of the UI differences I see with the patches applied
Fixed 


(In reply to Brian Grinstead [:bgrins] from comment #6)
> This could be worked around by setting display: -moz-box on
> #inspector-breadcrumbs-toolbar until the breadcrumbs converted, but this
> also causes layout issues on all the tools (see webconsole toolbar, debugger
> toolbar, etc).  I'm thinking the 'flex' layout should be opt-in for now
> unless if we can tackle all of the layout issues here.
Ah, I see, these panels are not ready for HTML layout. Okay, I did it specifically for the Inspector toolbar.

Honza

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8749634 [details] [diff] [review]
bug1266419.patch

Tim, do you have time to do a review of this patch?

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8749634 [details] [diff] [review]
bug1266419.patch

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

We have the '.devtools-button' class that is tailored for HTML buttons. Can you use that for the command-buttons and the inspector toolbar buttons ?

You can use the ::before pseudo element to set the image (use the background-image property).

'.devtools-toolbarbutton' is made mainly for XUL toolbarbuttons, so it's not appropriate here.

::: devtools/client/inspector/inspector.xul
@@ +165,3 @@
>          class="devtools-toolbar"
>          nowindowdrag="true">
> +        <html:button id="inspector-element-add-button"

You're removing the tooltip, not sure if that's intentional

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8749634 [details] [diff] [review]
bug1266419.patch

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

::: devtools/client/themes/toolbars.css
@@ +1011,5 @@
>  .theme-light .devtools-tab[icon-invertable] > image,
> +.theme-light #toolbox-dock-buttons > button,
> +.theme-light .command-button-invertable,
> +.theme-light .devtools-closebutton,
> +.theme-light button.devtools-toolbarbutton,

Agreed with ntim - the buttons should be using the .devtools-button class

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: weird-text-button-states.png

See the text buttons like "Import" in this screenshot on the light theme

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8749147 [details] [diff] [review]
bug1266419-frames-menu.patch

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

::: devtools/client/framework/toolbox.js
@@ +1733,5 @@
> +   *                 destroy {Boolean}: Set to true if destroyed
> +   *                 parentID {Number}: ID of the parent frame (not set
> +   *                                    for top level window)
> +   */
> +  _updateFrames: function (event, data) {

Are there any tests for this feature?  Seems likely they'll need to be updated

::: devtools/client/framework/toolbox.xul
@@ +115,5 @@
>      <toolbar class="devtools-tabbar">
>        <hbox id="toolbox-picker-container" />
>        <hbox id="toolbox-tabs" flex="1" role="tablist" />
>        <hbox id="toolbox-buttons" pack="end">
>          <toolbarbutton id="command-button-frames"

This should switch to an html button and the .devtools button class.  I believe there is also an [open] attribute that can be set on it for different styling while the popup is opened.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8749634 [details] [diff] [review]
bug1266419.patch

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

::: devtools/client/themes/toolbars.css
@@ +100,5 @@
>    border-width: 0;
>    border-bottom-width: 1px;
>    border-style: solid;
>    height: 24px;
> +  line-height: 15px;

This change also makes some toolbars (computed view) unintentionally smaller. Can you undo it ?

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

(In reply to Tim Nguyen :ntim from comment #9)
> Comment on attachment 8749634 [details] [diff] [review]
> bug1266419.patch
> 
> Review of attachment 8749634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We have the '.devtools-button' class that is tailored for HTML buttons. Can
> you use that for the command-buttons and the inspector toolbar buttons ?
Done 

> ::: devtools/client/inspector/inspector.xul
> @@ +165,3 @@
> >          class="devtools-toolbar"
> >          nowindowdrag="true">
> > +        <html:button id="inspector-element-add-button"
> 
> You're removing the tooltip, not sure if that's intentional
Fixed

(In reply to Brian Grinstead [:bgrins] from comment #10)
> ::: devtools/client/themes/toolbars.css
> @@ +1011,5 @@
> >  .theme-light .devtools-tab[icon-invertable] > image,
> > +.theme-light #toolbox-dock-buttons > button,
> > +.theme-light .command-button-invertable,
> > +.theme-light .devtools-closebutton,
> > +.theme-light button.devtools-toolbarbutton,
> 
> Agreed with ntim - the buttons should be using the .devtools-button class
Yes, this place is also fixed.


(In reply to Brian Grinstead [:bgrins] from comment #11)
> Created attachment 8749858 [details]
> weird-text-button-states.png
> 
> See the text buttons like "Import" in this screenshot on the light theme
Should be fixed too (including the button in the Net panel)


(In reply to Brian Grinstead [:bgrins] from comment #12)
> > +  _updateFrames: function (event, data) {
> 
> Are there any tests for this feature?  Seems likely they'll need to be
> updated
True, there are some (e.g. framework\test\browser_toolbox_window_title_frame_select.js)
I need to look into this yet. I think we'll need to change the way how the test works...


> ::: devtools/client/framework/toolbox.xul
> @@ +115,5 @@
> >      <toolbar class="devtools-tabbar">
> >        <hbox id="toolbox-picker-container" />
> >        <hbox id="toolbox-tabs" flex="1" role="tablist" />
> >        <hbox id="toolbox-buttons" pack="end">
> >          <toolbarbutton id="command-button-frames"
> 
> This should switch to an html button and the .devtools button class.  I
This is done in the second patch

> believe there is also an [open] attribute that can be set on it for
> different styling while the popup is opened.
Where is the "open" attribute? I don't see it.

The order of patches:

1) bug1266419-frames-menu.patch
2) bug1266419.patch


Honza

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> > believe there is also an [open] attribute that can be set on it for
> > different styling while the popup is opened.
> Where is the "open" attribute? I don't see it.

The styles are set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#295 and here's an example of a place where it's set: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/views/options-view.js#92

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8750393 [details] [diff] [review]
bug1266419.patch

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

::: devtools/client/framework/toolbox.xul
@@ +121,5 @@
> +                     class="command-button command-button-invertable devtools-button"
> +                     title="&toolboxFramesTooltip;"
> +                     hidden="true">
> +          <html:div class="image"></html:div>
> +          <html:div class="drop-down"></html:div>

Since you're using the .devtools-button class, you can omit both of these elements.

You can just add the arrow on .devtools-button and set the image on .devtools-button::before.

::: devtools/client/themes/inspector.css
@@ +30,5 @@
>  
>  /* Expand/collapse panel toolbar button */
>  
>  #inspector-pane-toggle {
> +  background: var(--theme-pane-collapse-image) no-repeat center;

So the benefit of using .devtools-button is that it allows you to set the button image without having to worry about the position/size/...
This is why you should set the image on ::before (as I mentioned in the previous review).
#inspector-pane-toggle::before {
 background-image: var(--theme-pane-collapse-image);
}

@@ +35,4 @@
>  }
>  
>  #inspector-pane-toggle[pane-collapsed] {
> +  background-image: var(--theme-pane-expand-image);

Same thing here: #inspector-pane-toggle[pane-collapsed]::before

@@ +46,5 @@
>  
>  /* Add element toolbar button */
>  
>  #inspector-element-add-button {
> +  background: url("chrome://devtools/skin/images/add.svg") no-repeat center;

Same thing here:
#inspector-element-add-button::before {
  background-image: url("chrome://devtools/skin/images/add.svg");	
}

::: devtools/client/themes/toolbars.css
@@ -316,5 @@
>    background-color: rgba(0, 0, 0, .2); /* Splitter */
>  }
>  
>  /* Text-only button states */
> -.theme-dark .devtools-button:not(:empty):not([disabled]):hover,

Removing the .devtools-button styles isn't right (feel free to remove the #toolbox-buttons rules though). To make a proper icon button, the button must be empty (no whitespace or children as innerHTML).

@@ -322,4 @@
>  .theme-dark .devtools-toolbarbutton:not(:-moz-any([checked=true],[disabled],[text-as-image]))[label]:hover {
>    background: rgba(0, 0, 0, .3); /* Splitters */
>  }
> -.theme-light .devtools-button:not(:empty):not([disabled]):hover,

Same thing here.

@@ -327,5 @@
>  .theme-light .devtools-toolbarbutton:not(:-moz-any([checked=true],[disabled],[text-as-image]))[label]:hover {
>    background: rgba(170, 170, 170, .3); /* Splitters */
>  }
>  
> -.theme-dark .devtools-button:not(:empty):not([disabled]):hover:active,

Same thing here.

@@ -333,4 @@
>  .theme-dark .devtools-toolbarbutton:not(:-moz-any([checked=true],[disabled],[text-as-image]))[label]:hover:active {
>    background: rgba(0, 0, 0, .4); /* Splitters */
>  }
> -.theme-light .devtools-button:not(:empty):not([disabled]):hover:active,

Same thing here.

@@ -340,5 @@
>  }
>  
>  .theme-dark .devtools-toolbarbutton:not([disabled])[label][checked=true],
> -.theme-dark .devtools-toolbarbutton:not([disabled])[label][open],
> -.theme-dark .devtools-button:not(:empty)[checked=true],

Same thing here. Removing the .devtools-button styles breaks things.

@@ -347,5 @@
>    color: var(--theme-selection-color);
>  }
>  .theme-light .devtools-toolbarbutton:not([disabled])[label][checked=true],
> -.theme-light .devtools-toolbarbutton:not([disabled])[label][open],
> -.theme-light .devtools-button:not(:empty)[checked=true],

Same thing here.

@@ +528,3 @@
>    width: 16px;
>    height: 16px;
> +  background: var(--close-button-image) no-repeat center;

All the .devtools-closebutton rules can be deleted as we're now using .devtools-button.
This should suffice:
.devtools-closebutton::before {
  background-image: var(--close-button-image);
}

@@ +631,5 @@
>   * Rules that apply to the global toolbox like command buttons,
>   * devtools tabs, docking buttons, etc. */
>  
> +#toolbox-controls > button,
> +#toolbox-dock-buttons > button {

The toolbox controls can probably benefit from the devtools-button class too.

@@ +751,1 @@
>    filter: url(images/filters.svg#checked-icon-state) !important;

These filter rules should not be needed if you used ::before.

@@ +758,1 @@
>  }

Same thing here, please set the image on ::before, without setting other background properties.
Like this: 
#command-button-paintflashing::before {
  background-image: var(--command-paintflashing-image);	
}

The same pattern applies below too.

@@ +807,5 @@
> +  display: inline-block;
> +  vertical-align: middle;
> +  width: 8px;
> +  height: 8px;
> +  background: url("chrome://devtools/skin/images/dropmarker.svg") no-repeat center;

With ::devtools-button, it should boil down to this (I haven't tested):
#command-button-frames::before {
  background-image: var(--command-frames-image);
}

#command-button-frames {
  background: url("chrome://devtools/skin/images/dropmarker.svg") no-repeat right;
  padding-inline-end: 8px;
}

#command-button-frames:-moz-dir(rtl) {
  background-position: left;
}

@@ +991,5 @@
>  .theme-light .devtools-tab[icon-invertable] > image,
> +.theme-light #toolbox-dock-buttons > button,
> +.theme-light .command-button-invertable,
> +.theme-light .devtools-closebutton,
> +.theme-light button.devtools-button,

You can remove all the selectors you just added from the list if you use ::before to set the image.


You might need to override .theme-light .command-button:not(command-button-invertable) with filter: none; by adding the selector to the list later in the file.

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

(In reply to Tim Nguyen :ntim from comment #16)
> Comment on attachment 8750393 [details] [diff] [review]
> bug1266419.patch
> 
> Review of attachment 8750393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/toolbox.xul
> @@ +121,5 @@
> > +                     class="command-button command-button-invertable devtools-button"
> > +                     title="&toolboxFramesTooltip;"
> > +                     hidden="true">
> > +          <html:div class="image"></html:div>
> > +          <html:div class="drop-down"></html:div>
> 
> Since you're using the .devtools-button class, you can omit both of these
> elements.
> 
> You can just add the arrow on .devtools-button and set the image on
> .devtools-button::before.
Ah, I see, this clears things for me, done.

> 
> ::: devtools/client/themes/inspector.css
> @@ +30,5 @@
> >  
> >  /* Expand/collapse panel toolbar button */
> >  
> >  #inspector-pane-toggle {
> > +  background: var(--theme-pane-collapse-image) no-repeat center;
> 
> So the benefit of using .devtools-button is that it allows you to set the
> button image without having to worry about the position/size/...
> This is why you should set the image on ::before (as I mentioned in the
> previous review).
Yes

> #inspector-pane-toggle::before {
>  background-image: var(--theme-pane-collapse-image);
> }
Done

> 
> @@ +35,4 @@
> >  }
> >  
> >  #inspector-pane-toggle[pane-collapsed] {
> > +  background-image: var(--theme-pane-expand-image);
> 
> Same thing here: #inspector-pane-toggle[pane-collapsed]::before
Done

> 
> @@ +46,5 @@
> >  
> >  /* Add element toolbar button */
> >  
> >  #inspector-element-add-button {
> > +  background: url("chrome://devtools/skin/images/add.svg") no-repeat center;
> 
> Same thing here:
> #inspector-element-add-button::before {
>   background-image: url("chrome://devtools/skin/images/add.svg");	
> }
Done

> 
> ::: devtools/client/themes/toolbars.css
> @@ -316,5 @@
> >    background-color: rgba(0, 0, 0, .2); /* Splitter */
> >  }
> >  
> >  /* Text-only button states */
> > -.theme-dark .devtools-button:not(:empty):not([disabled]):hover,
> 
> Removing the .devtools-button styles isn't right (feel free to remove the
> #toolbox-buttons rules though). To make a proper icon button, the button
> must be empty (no whitespace or children as innerHTML).
Yeah, wasn't sure about that, fixed now, #toolbox-buttons rules removed (from all six places).

 
> @@ +528,3 @@
> >    width: 16px;
> >    height: 16px;
> > +  background: var(--close-button-image) no-repeat center;
> 
> All the .devtools-closebutton rules can be deleted as we're now using
> .devtools-button.
> This should suffice:
> .devtools-closebutton::before {
>   background-image: var(--close-button-image);
> }
> 
> @@ +631,5 @@
> >   * Rules that apply to the global toolbox like command buttons,
> >   * devtools tabs, docking buttons, etc. */
> >  
> > +#toolbox-controls > button,
> > +#toolbox-dock-buttons > button {
> 
> The toolbox controls can probably benefit from the devtools-button class too.
They do now (I removed some styles, more simplification possible perhaps)

> 
> @@ +751,1 @@
> >    filter: url(images/filters.svg#checked-icon-state) !important;
> 
> These filter rules should not be needed if you used ::before.
Removed

> 
> @@ +758,1 @@
> >  }
> 
> Same thing here, please set the image on ::before, without setting other
> background properties.
> Like this: 
> #command-button-paintflashing::before {
>   background-image: var(--command-paintflashing-image);	
> }
> 
> The same pattern applies below too.
Done

> 
> @@ +807,5 @@
> > +  display: inline-block;
> > +  vertical-align: middle;
> > +  width: 8px;
> > +  height: 8px;
> > +  background: url("chrome://devtools/skin/images/dropmarker.svg") no-repeat center;
> 
> With ::devtools-button, it should boil down to this (I haven't tested):
> #command-button-frames::before {
>   background-image: var(--command-frames-image);
> }
> 
> #command-button-frames {
>   background: url("chrome://devtools/skin/images/dropmarker.svg") no-repeat
> right;
>   padding-inline-end: 8px;
> }
> 
> #command-button-frames:-moz-dir(rtl) {
>   background-position: left;
> }
I had to override background-size for the dropdown image, otherwise if was too big.
(since the default size coming from command-button is 16x16)

> 
> @@ +991,5 @@
> >  .theme-light .devtools-tab[icon-invertable] > image,
> > +.theme-light #toolbox-dock-buttons > button,
> > +.theme-light .command-button-invertable,
> > +.theme-light .devtools-closebutton,
> > +.theme-light button.devtools-button,
> 
> You can remove all the selectors you just added from the list if you use
> ::before to set the image.
Done, except of .devtools-button::before (for light theme)

> 
> 
> You might need to override .theme-light
> .command-button:not(command-button-invertable) with filter: none; by adding
> the selector to the list later in the file.
Done

There is one problem though, 'toolbox-tabs' and 'toolbox-buttons' hbox elements are not overlapped if there isn't enough horizontal space. Any tips how to fix that? I'll attach a screenshot.

Honza

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: tabbox.png

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8750842 [details] [diff] [review]
bug1266419.patch

Tim, are you able to review this?

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: missing-option-labels.png

Missing option labels with the patch applied

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> There is one problem though, 'toolbox-tabs' and 'toolbox-buttons' hbox
> elements are not overlapped if there isn't enough horizontal space. Any tips
> how to fix that? I'll attach a screenshot.

Maybe setting display: flex on #toolbox-buttons?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8750862 [details]
> missing-option-labels.png
> 
> Missing option labels with the patch applied

I'm not sure why this broke - maybe the options panel is somehow reading the buttons tooltip text to populate the label?  Can you take a look at it?

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

(In reply to Brian Grinstead [:bgrins] from comment #21)
> (In reply to Jan Honza Odvarko [:Honza] from comment #17)
> Maybe setting display: flex on #toolbox-buttons?
Good call, fixed.

(In reply to Brian Grinstead [:bgrins] from comment #22)
> I'm not sure why this broke - maybe the options panel is somehow reading the
> buttons tooltip text to populate the label?  Can you take a look at it?
Fixed, yes, it's stored in the title (HTML) not tooltiptext (XUL)

Honza

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also, as mentioned in comment #3, what about the focus?

Honza

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8751684 [details] [diff] [review]
bug1266419.patch

Same as Comment 19

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Forgot to note, there is one known problem. The focus management works
> differently int HTML and XUL and I am not sure how to fix this. This test
> shows that (it fails).
> devtools/client/framework/test/browser_toolbox_keyboard_navigation.js 
> 
> I am going to file a follow up for it and we should discuss how important
> this is (could be rather harder to fix).

Ah, if you are testing on OSX then that test is meant to fail: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/browser.ini#46.  It only works if you have Full Keyboard access enabled (ctrl+F7 or Preferences -> Keyboard -> Shortcuts -> All controls).  But fails with the default setting which is textboxes and lists only so it's skipped in OSX in automation.

Comment 27

[Tim Nguyen :ntim]

Comment on attachment 8751684 [details] [diff] [review]
bug1266419.patch

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

The JS changes look good to me.
r=me with the CSS nits fixed.

::: devtools/client/themes/firebug-theme.css
@@ +13,5 @@
>  }
>  
>  /* Remove filters on firebug specific images */
>  
> +.theme-firebug .devtools-button::before,

This rule will make all devtools-button icons white on very light blue, which lowers significantly the contrast.

Can you instead use .theme-firebug .devtools-tabbar .devtools-button::before? (it will also take care of the 2 selectors below)

@@ +15,5 @@
>  /* Remove filters on firebug specific images */
>  
> +.theme-firebug .devtools-button::before,
> +.theme-firebug #toolbox-dock-buttons > button,
> +.theme-firebug .devtools-closebutton,

.theme-firebug #toolbox-dock-buttons > button,
.theme-firebug .devtools-closebutton,

Can both be removed from the list, since they now have no effect.

@@ +20,2 @@
>  .theme-firebug .devtools-option-toolbarbutton > image,
> +.theme-firebug .command-button-invertable,

This should be .theme-firebug .command-button-invertable::before

@@ +252,2 @@
>  /* Move the Inspector button a bit down (looks better) */
> +.theme-firebug #command-button-pick {

#command-button-pick::before

::: devtools/client/themes/inspector.css
@@ +9,5 @@
> +   panels (e.g. webconsole, debugger), these are not ready for HTML
> +   layout yet. */
> +#inspector-toolbar.devtools-toolbar {
> +  display: flex;
> +  flex-flow: row;

Isn't row the default value for flex-flow already ?

::: devtools/client/themes/toolbars.css
@@ -100,5 @@
>    border-width: 0;
>    border-bottom-width: 1px;
>    border-style: solid;
>    height: 24px;
> -  line-height: 24px;

Can you restore the line-height ?

@@ +703,5 @@
>  /* Command buttons */
>  
>  .command-button {
>    -moz-appearance: none;
>    border: none;

border: none; and -moz-appearance: none are not needed.

@@ +709,2 @@
>    margin: 0;
> +  width: 24px;

I think this width rule has no effect because of: 
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#164

I'm not sure why the width change to 24px is needed anyway.

@@ +714,5 @@
> +  /* Make sure the size overwrites the defaults  defined for
> +     each command button below. */
> +  background-size: 16px !important;
> +  background-position: 0 center;
> +  background-repeat: no-repeat;

All the background rules are obsolete.

@@ +715,5 @@
> +     each command button below. */
> +  background-size: 16px !important;
> +  background-position: 0 center;
> +  background-repeat: no-repeat;
> +  opacity: 0.7;

The opacity should be applied on ::before to get the same result as before.
Right now, the icon is shown with an 0.7*0.8 opacity (since ::before already has a 0.8 opacity by default). Setting the opacity to 0.7 on ::before will override the 0.8 opacity instead of adding up to it.

@@ +730,1 @@
>    opacity: 0.85;

Same comment here.

@@ +736,1 @@
>    opacity: 1;

Same comment here

@@ +758,5 @@
>    background-image: var(--command-pick-image);
>  }
>  
> +#command-button-pick {
> +  margin-inline-end: 4px;

Why is this needed ?

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

(In reply to Tim Nguyen :ntim from comment #27)
> Comment on attachment 8751684 [details] [diff] [review]
> bug1266419.patch
> 
> Review of attachment 8751684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The JS changes look good to me.
> r=me with the CSS nits fixed.
> 
> ::: devtools/client/themes/firebug-theme.css
> @@ +13,5 @@
> >  }
> >  
> >  /* Remove filters on firebug specific images */
> >  
> > +.theme-firebug .devtools-button::before,
> 
> This rule will make all devtools-button icons white on very light blue,
> which lowers significantly the contrast.
> 
> Can you instead use .theme-firebug .devtools-tabbar
> .devtools-button::before? (it will also take care of the 2 selectors below)
Done

> 
> @@ +15,5 @@
> >  /* Remove filters on firebug specific images */
> >  
> > +.theme-firebug .devtools-button::before,
> > +.theme-firebug #toolbox-dock-buttons > button,
> > +.theme-firebug .devtools-closebutton,
> 
> .theme-firebug #toolbox-dock-buttons > button,
> .theme-firebug .devtools-closebutton,
> 
> Can both be removed from the list, since they now have no effect.
Done

> 
> @@ +20,2 @@
> >  .theme-firebug .devtools-option-toolbarbutton > image,
> > +.theme-firebug .command-button-invertable,
> 
> This should be .theme-firebug .command-button-invertable::before
Done

> 
> @@ +252,2 @@
> >  /* Move the Inspector button a bit down (looks better) */
> > +.theme-firebug #command-button-pick {
> 
> #command-button-pick::before
Removed, the picker button position is now correct.

> 
> ::: devtools/client/themes/inspector.css
> @@ +9,5 @@
> > +   panels (e.g. webconsole, debugger), these are not ready for HTML
> > +   layout yet. */
> > +#inspector-toolbar.devtools-toolbar {
> > +  display: flex;
> > +  flex-flow: row;
> 
> Isn't row the default value for flex-flow already ?
Yes, removed.

> 
> ::: devtools/client/themes/toolbars.css
> @@ -100,5 @@
> >    border-width: 0;
> >    border-bottom-width: 1px;
> >    border-style: solid;
> >    height: 24px;
> > -  line-height: 24px;
> 
> Can you restore the line-height ?
Done

> 
> @@ +703,5 @@
> >  /* Command buttons */
> >  
> >  .command-button {
> >    -moz-appearance: none;
> >    border: none;
> 
> border: none; and -moz-appearance: none are not needed.
Removed

> 
> @@ +709,2 @@
> >    margin: 0;
> > +  width: 24px;
> 
> I think this width rule has no effect because of: 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> toolbars.css#164
Yes, removed.

> 
> I'm not sure why the width change to 24px is needed anyway.
One thing it affected was the #toolbox-picker-container. I set display: flex on the #toolbox-picker-container and since the picker is also .devtools-button now it allowed me to remove a few more CSS lines related to that button.

> 
> @@ +714,5 @@
> > +  /* Make sure the size overwrites the defaults  defined for
> > +     each command button below. */
> > +  background-size: 16px !important;
> > +  background-position: 0 center;
> > +  background-repeat: no-repeat;
> 
> All the background rules are obsolete.
Done

> 
> @@ +715,5 @@
> > +     each command button below. */
> > +  background-size: 16px !important;
> > +  background-position: 0 center;
> > +  background-repeat: no-repeat;
> > +  opacity: 0.7;
> 
> The opacity should be applied on ::before to get the same result as before.
> Right now, the icon is shown with an 0.7*0.8 opacity (since ::before already
> has a 0.8 opacity by default). Setting the opacity to 0.7 on ::before will
> override the 0.8 opacity instead of adding up to it.
Nice one, done.

> 
> @@ +730,1 @@
> >    opacity: 0.85;
> 
> Same comment here.
Done

> 
> @@ +736,1 @@
> >    opacity: 1;
> 
> Same comment here
Done

> 
> @@ +758,5 @@
> >    background-image: var(--command-pick-image);
> >  }
> >  
> > +#command-button-pick {
> > +  margin-inline-end: 4px;
> 
> Why is this needed ?
Yep, one of the things mentioned above, I just removed.

Honza

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tests.patch

Fixing existing tests for the iframe-picker. The internal structure of the menu should not be used in the tests (it'll change to HTML at some point) so, instead of clicking on individual menu-itemes I am using toolbox.onSelectFrame() to switch frame-context.

Honza

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try on Linux is green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c357def2288

Let's see other platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e4e09258c29

Honza

Comment 31

[Brian Grinstead [:bgrins]]

Honza, the two code patches need rebasing on top of fx-team

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-frames-menu.patch

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Comment 34

[Brian Grinstead [:bgrins]]

Honza, could you please make a try push and request Helen to do a UX review of the changes to make sure we aren't missing anything?

Comment 35

[Tim Nguyen :ntim]

Note that I still get conflicts with toolbox.js and toolbox.xul with the latest patches.

Comment 36

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #35)
> Note that I still get conflicts with toolbox.js and toolbox.xul with the
> latest patches.

Sorry, turns out I had to apply the frames menu patch first.

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tweaks.patch

Just a couple of CSS lines changed to fix the side bar toggle button and vertical centering of text in the Search box in Inspector panel.

Honza

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Patches should be applied in this order

1) bug1266419-frames-menu.patch
2) bug1266419.patch
3) bug1266419-tweaks.patch
4) bug1266419-tests.patch

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d859a26752d2

Honza

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8753344 [details] [diff] [review]
bug1266419-tweaks.patch

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

::: devtools/client/themes/inspector.css
@@ +39,5 @@
>  
> +/* Invert the toggle button in Firebug theme. */
> +.theme-firebug #inspector-pane-toggle {
> +  filter: invert(1);
> +}

By doing this, you're inverting twice the icon, once on ::before and once on the button.

The proper fix would be to add `.theme-firebug [id$="pane-toggle"]::before` to the list of selectors here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/firebug-theme.css#22

::: devtools/client/themes/toolbars.css
@@ +100,5 @@
>    border-width: 0;
>    border-bottom-width: 1px;
>    border-style: solid;
>    height: 24px;
> +  line-height: 20px;

Can you test if there are no regressions with this change ?
When you changed the line-height in the older patches at the same location, I've noticed the computed view toolbar had gone smaller on OSX.


Anyway, I'm not sure what this change fixes.

@@ +411,5 @@
>  }
>  
> +/* Make sure the text is v-centered in the Inspector panel for Firebug theme */
> +.theme-firebug .devtools-searchinput {
> +  line-height: 16px;

I suspect this breaks the alignment for the searchboxes in the other panels (or at least on some of them), although I haven't tested.

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tweaks.patch

(In reply to Tim Nguyen :ntim from comment #39)
> Comment on attachment 8753344 [details] [diff] [review]
> bug1266419-tweaks.patch
> 
> Review of attachment 8753344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/inspector.css
> @@ +39,5 @@
> >  
> > +/* Invert the toggle button in Firebug theme. */
> > +.theme-firebug #inspector-pane-toggle {
> > +  filter: invert(1);
> > +}
> 
> By doing this, you're inverting twice the icon, once on ::before and once on
> the button.
> 
> The proper fix would be to add `.theme-firebug [id$="pane-toggle"]::before`
> to the list of selectors here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> firebug-theme.css#22
Yes, this is much better, done.


> ::: devtools/client/themes/toolbars.css
> @@ +100,5 @@
> >    border-width: 0;
> >    border-bottom-width: 1px;
> >    border-style: solid;
> >    height: 24px;
> > +  line-height: 20px;
> 
> Can you test if there are no regressions with this change ?
> When you changed the line-height in the older patches at the same location,
> I've noticed the computed view toolbar had gone smaller on OSX.
> 
> 
> Anyway, I'm not sure what this change fixes.
Properly v-center text in Inspector's search box, but let me revert this, I don't like it either.

> 
> @@ +411,5 @@
> >  }
> >  
> > +/* Make sure the text is v-centered in the Inspector panel for Firebug theme */
> > +.theme-firebug .devtools-searchinput {
> > +  line-height: 16px;
> 
> I suspect this breaks the alignment for the searchboxes in the other panels
> (or at least on some of them), although I haven't tested.
Agreed, I changed the selector and used #inspector-searchbox. See my patch


Honza

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: inspector-search.png

Tim, see the screenshot, there are two problems:

1) The toggle button is inverted (this is now properly fixed)
2) The text in the search box isn't v-centered (fixed by new rule applied directly on #inspector-searchbox

Honza

Comment 42

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8753682 [details] [diff] [review]
bug1266419-tweaks.patch

I think bgrins and ntim did a good job going through this—everything to me seems to look exactly like it does in release, which I assume was the point.

+1

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8752248 [details] [diff] [review]
bug1266419-frames-menu.patch

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

Forwarding this review to Alex, who's worked on the frames menu

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8752249 [details] [diff] [review]
bug1266419.patch

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

Looks good, trusting my earlier reviews + ntim's comments + Helen's ui-r+

Comment 45

[Brian Grinstead [:bgrins]]

Comment on attachment 8753682 [details] [diff] [review]
bug1266419-tweaks.patch

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

Ditto.  Please update commit message to be clear that this is affecting just the Firebug theme.  Or better yet, fold this into the main patch

Comment 46

[Brian Grinstead [:bgrins]]

Comment on attachment 8752175 [details] [diff] [review]
bug1266419-tests.patch

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

Also forwarding to Alex

Comment 47

[Tim Nguyen :ntim]

Comment on attachment 8753682 [details] [diff] [review]
bug1266419-tweaks.patch

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

::: devtools/client/themes/firebug-theme.css
@@ +250,5 @@
> +
> +/* Make sure the text is vertically centered in Inspector's
> +   search box. */
> +.theme-firebug #inspector-searchbox {
> +  line-height: 17px;

The cause of the vertical alignment problem is that the XUL textbox has a different `display` value (-moz-box) than the HTML toolbar (flex). I tried to fix this in a less hacky way, but I couldn't find a viable solution that doesn't involve hardcoded height/line-heights/...

I'm guessing this would be fixed by switching the searchbox to HTML (as other HTML tools don't have this problem).

Anyway, can you move this rule inside inspector.css, and make it apply to all themes ? (the issue is in all themes I believe)
Also, I would mention in the comment above:
"This can be removed when the search box is switched to HTML"

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tweaks.patch

(In reply to Tim Nguyen :ntim from comment #47)
> Comment on attachment 8753682 [details] [diff] [review]
> bug1266419-tweaks.patch
> 
> Review of attachment 8753682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/firebug-theme.css
> @@ +250,5 @@
> > +
> > +/* Make sure the text is vertically centered in Inspector's
> > +   search box. */
> > +.theme-firebug #inspector-searchbox {
> > +  line-height: 17px;
> 
> The cause of the vertical alignment problem is that the XUL textbox has a
> different `display` value (-moz-box) than the HTML toolbar (flex). I tried
> to fix this in a less hacky way, but I couldn't find a viable solution that
> doesn't involve hardcoded height/line-heights/...
> 
> I'm guessing this would be fixed by switching the searchbox to HTML (as
> other HTML tools don't have this problem).
Yes, Bug 1265759 

> Anyway, can you move this rule inside inspector.css, and make it apply to
> all themes ? (the issue is in all themes I believe)
> Also, I would mention in the comment above:
> "This can be removed when the search box is switched to HTML"
Done

(In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from comment #45)
> Ditto.  Please update commit message to be clear that this is affecting just
> the Firebug theme.  Or better yet, fold this into the main patch
It's now affecting all themes so, I am keeping the message (I also kept this as separate patch since it could be useful for bug 1265759)

Thanks!
Honza

Comment 49

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-frames-menu.patch

Rebased

Honza

Comment 50

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-frames-menu.patch

Rebased

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Comment 52

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tests.patch

Rebased

Honza

Comment 53

[Alexandre Poirot [:ochameau]]

Comment on attachment 8754346 [details] [diff] [review]
bug1266419-frames-menu.patch

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

A few things to tweak, but looks good.

Are you planning to replace the xul:toolbarbutton by an html element?
Is there a bug for that?

::: devtools/client/framework/toolbox.js
@@ +419,5 @@
>  
>        gDevTools.on("pref-changed", this._prefChanged);
>  
>        let framesMenu = this.doc.getElementById("command-button-frames");
> +      framesMenu.addEventListener("click", this.selectFrame, true);

You should rename selectFrame to updateFramesMenu/createFramesMenu or something as you no longer select one here.

Also I'm not sure registering a capturing event is useful anymore?

@@ +1719,5 @@
> +    this.frameMap.forEach(frame => {
> +      // A frame is checked if it's the selected one. If there is no
> +      // selection the top level frame is checked by default.
> +      let checked = this.selectedFrameId ?
> +        frame.id == this.selectedFrameId : !frame.parentID;

On the browser toolbox you should have multiple top level frames (each being a top level window) with undefined parentID.
If selectedFrameId is null, only the first frame without parentID should be considered as selected.
It may be easier to consider that selectedFrameId is correctly set in this function and cover this in _updateFrames with something like this:
if (this.selectedFrameId) {
  ...
} else {
  // Select the first top level window by default
  this.selectedFrameId = [..this.frameMap.entries()].filter(_, frame => !frame.parentID)[0];
}

@@ +1723,5 @@
> +        frame.id == this.selectedFrameId : !frame.parentID;
> +
> +      // Create menu item.
> +      menu.append(new MenuItem({
> +        id: "frame-id-" + frame.id,

Do you really need this?

@@ +1726,5 @@
> +      menu.append(new MenuItem({
> +        id: "frame-id-" + frame.id,
> +        label: frame.url,
> +        type: "radio",
> +        checked: checked,

nit: checked: checked, => checked,

@@ +1734,5 @@
> +      }));
> +    });
> +
> +    // Show a drop down menu with frames.
> +    // XXX Missing menu API for specifying target (anchor)

Do you have a followup for this?

@@ +1794,5 @@
> +        } else {
> +          this.frameMap.set(frame.id, frame);
> +        }
> +      });
> +    }

Great abstraction between actor events and menuitems with the Map!

Comment 54

[Alexandre Poirot [:ochameau]]

Comment on attachment 8754348 [details] [diff] [review]
bug1266419-tests.patch

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

You are no longer testing the menu items.
Do you think you could fake a click on the toolbarbutton, wait for the menu and assert that clicking on items do update the frames?
Because here you are now mostly checking for the Map integrity.

Comment 55

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-frames-menu.patch

(In reply to Alexandre Poirot [:ochameau] from comment #53)
> Comment on attachment 8754346 [details] [diff] [review]
> bug1266419-frames-menu.patch
> 
> Review of attachment 8754346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few things to tweak, but looks good.
> 
> Are you planning to replace the xul:toolbarbutton by an html element?
> Is there a bug for that?
Yes, XUL -> HTML replacement happens in the other patch.

> 
> ::: devtools/client/framework/toolbox.js
> @@ +419,5 @@
> >  
> >        gDevTools.on("pref-changed", this._prefChanged);
> >  
> >        let framesMenu = this.doc.getElementById("command-button-frames");
> > +      framesMenu.addEventListener("click", this.selectFrame, true);
> 
> You should rename selectFrame to updateFramesMenu/createFramesMenu or
> something as you no longer select one here.
True, I renamed it to 'showFramesMenu'.

> 
> Also I'm not sure registering a capturing event is useful anymore?
Removed

> 
> @@ +1719,5 @@
> > +    this.frameMap.forEach(frame => {
> > +      // A frame is checked if it's the selected one. If there is no
> > +      // selection the top level frame is checked by default.
> > +      let checked = this.selectedFrameId ?
> > +        frame.id == this.selectedFrameId : !frame.parentID;
> 
> On the browser toolbox you should have multiple top level frames (each being
> a top level window) with undefined parentID.
> If selectedFrameId is null, only the first frame without parentID should be
> considered as selected.
> It may be easier to consider that selectedFrameId is correctly set in this
> function and cover this in _updateFrames with something like this:
> if (this.selectedFrameId) {
>   ...
> } else {
>   // Select the first top level window by default
>   this.selectedFrameId = [..this.frameMap.entries()].filter(_, frame =>
> !frame.parentID)[0];
> }
Good call, done

> 
> @@ +1723,5 @@
> > +        frame.id == this.selectedFrameId : !frame.parentID;
> > +
> > +      // Create menu item.
> > +      menu.append(new MenuItem({
> > +        id: "frame-id-" + frame.id,
> 
> Do you really need this?
Removed

> 
> @@ +1726,5 @@
> > +      menu.append(new MenuItem({
> > +        id: "frame-id-" + frame.id,
> > +        label: frame.url,
> > +        type: "radio",
> > +        checked: checked,
> 
> nit: checked: checked, => checked,
Done

> 
> @@ +1734,5 @@
> > +      }));
> > +    });
> > +
> > +    // Show a drop down menu with frames.
> > +    // XXX Missing menu API for specifying target (anchor)
> 
> Do you have a followup for this?
bug 1274551

> 
> @@ +1794,5 @@
> > +        } else {
> > +          this.frameMap.set(frame.id, frame);
> > +        }
> > +      });
> > +    }
> 
> Great abstraction between actor events and menuitems with the Map!
Thanks for the review!

Updated patch attached. 

Honza

Comment 56

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Rebased

Honza

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #54)
> Comment on attachment 8754348 [details] [diff] [review]
> bug1266419-tests.patch
> 
> Review of attachment 8754348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You are no longer testing the menu items.
> Do you think you could fake a click on the toolbarbutton, wait for the menu
> and assert that clicking on items do update the frames?
> Because here you are now mostly checking for the Map integrity.
Yes, I am aware of this. I didn't want to touch the internals of the Menu API. These are still using XUL internally, which is a workaround and it should change to HTML. But, I agree we should test this somehow. We could have test API that are wrapping the fact that XUL is still used inside. I filled a follow up for this and we can discuss how to do it properly: bug 1274558
Is that ok to do it as a follow up so, we can land this?

Honza

Comment 58

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Alex, see my comment above.

Honza

Comment 59

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #57)
> Yes, I am aware of this. I didn't want to touch the internals of the Menu
> API. These are still using XUL internally, which is a workaround and it
> should change to HTML. But, I agree we should test this somehow. We could
> have test API that are wrapping the fact that XUL is still used inside.

I don't see why the fact that we are using XUL behind the scene changes anything?
We just miss the tests being written and/or a naive API on menus to check their state.
Something like:
  menu.onshow = function () {
    is(menu.items.count, 10);
    is(menu.items[0].label, "foo");
  }

> I filled a follow up for this and we can discuss how to do it properly: bug
> 1274558
> Is that ok to do it as a follow up so, we can land this?

It is if bug 1274558 is a immediate followup where you are going to modify tests from attachment 8754348 [details] [diff] [review] to assert menus instead of the toolbox maps.
It is not if that ends up being triaged as a P4-enhancement that may not be done.
It is not if that's only going to add test against menu codebase only.
The point if about testing menu behavior on real world usages like toolbox tests.

Comment 60

[Brian Grinstead [:bgrins]]

In theory the fact that we are using XUL behind the scenes could make initial migration of the test easier since the XUL elements that are being checked will still exist (although only when the menu is opened)

Comment 61

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Rebase

Honza

Comment 62

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tests.patch

(In reply to Alexandre Poirot [:ochameau] from comment #59)
> The point if about testing menu behavior on real world usages like toolbox
> tests.
Alright, agreed, here is updated version. Tests are now doing somethings like as follows:

// Showing the context menu and wait till it's ready on the screen.
// The test isn't simulating clicking since it needs reference to
// the Menu object. This should be ok since `showFramesMenu` is the
// direct click handler.
let menu = toolbox.showFramesMenu();
yield once(menu, "open");

// Verifying that the menu is properly populated. 
let frames = menu.menuitems;
is(frames.length, 2, "We have both frames in the list");

The test is accessing the internal 'menuitems' structure, not the XUL structure so, when the XUL->HTML transformation happens the test won't break. Also, testing the internal XUL (or later HTML) structure should be done by tests related to the Menu itself.

Honza

Comment 63

[Alexandre Poirot [:ochameau]]

Comment on attachment 8756802 [details] [diff] [review]
bug1266419-tests.patch

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

Thanks for keeping test great!

See my comment about browser_inspector_highlighter-iframes_02.js.

::: devtools/client/inspector/test/browser_inspector_highlighter-iframes_02.js
@@ +50,5 @@
> +  // Verify that the menu is popuplated.
> +  let frames = menu.menuitems;
> +  ok(frames.length, 1, "There must be expected number of frames");
> +
> +  let frame = frames[frameIndex];

Isn't frame going to be null here? Or the previous assertion going to be false?

You assert that frames has only one item (ok(frames.length, 1))
be here you access `frames[1]` (switchToFrameContext(1, toolbox, inspector))

Comment 64

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419-tests.patch

(In reply to Alexandre Poirot [:ochameau] from comment #63)
> Comment on attachment 8756802 [details] [diff] [review]
> bug1266419-tests.patch
> You assert that frames has only one item (ok(frames.length, 1))
> be here you access `frames[1]` (switchToFrameContext(1, toolbox, inspector))
True, the assertion shouldn't be there, removed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e980cbf7f0d

Thanks!
Honza

Comment 65

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Fixing try failures (reset currentFrameId when the frame is destroyed)

New try push looks good (only clipboard failures):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6738382eaa58&selectedJob=21520623

Honza

Comment 66

[Carsten Book [:Tomcat]]

has problems to apply:

applying bug1266419-tweaks.patch
patching file devtools/client/themes/firebug-theme.css
Hunk #1 FAILED at 13
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/firebug-theme.css.rej
patching file devtools/client/themes/inspector.css
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/inspector.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1266419-tweaks.patch

can you take a look, thanks!

Comment 67

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266419.patch

Rebasing

Honza

Comment 68

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I updated bug1266419.patch, but bug1266419-tweaks.patch applies for me just fine.

Note that patches should be applied in this order

1) bug1266419-frames-menu.patch
2) bug1266419.patch
3) bug1266419-tweaks.patch
4) bug1266419-tests.patch

Does it work for you now?

Honza

Comment 69

[Carsten Book [:Tomcat]]

(In reply to Jan Honza Odvarko [:Honza] from comment #68)
> I updated bug1266419.patch, but bug1266419-tweaks.patch applies for me just
> fine.
> 
> Note that patches should be applied in this order
> 
> 1) bug1266419-frames-menu.patch
> 2) bug1266419.patch
> 3) bug1266419-tweaks.patch
> 4) bug1266419-tests.patch
> 
> Does it work for you now?
> 
> Honza

that works great ! Thanks for the Instructions Honza!

Comment 70

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/62ce2708b5be
https://hg.mozilla.org/integration/fx-team/rev/aaae402831aa
https://hg.mozilla.org/integration/fx-team/rev/703087e3d829
https://hg.mozilla.org/integration/fx-team/rev/f4769575334d

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62ce2708b5be
https://hg.mozilla.org/mozilla-central/rev/aaae402831aa
https://hg.mozilla.org/mozilla-central/rev/703087e3d829
https://hg.mozilla.org/mozilla-central/rev/f4769575334d

Comment 72

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I tested this bug on 
- 50.0a1 (2016-06-09) 
- 49.0a2 (2016-06-09), 
using 
- Windows 10 x64
- Ubuntu 14.04 x86
- Mac OS X 10.11.
The overall toolbars and buttons state seems to be the expected one and also, there is no misbehavior between the last and the present implementation.

There is one exception: on Ubuntu 14.04 x86 and on Mac OS X 10.11, Rules, Computed, Box Model and Animations side panel buttons have a "breadcrumbs" border when clicked. This issue is not reproducing on 48.0a1 (2016-04-21).
[Regression range]:
- Last good revision: 9f866b72af4a3c4520205d55c60aa74548682c4a
- First bad revision: 369a5ee3a2880a4a98df3a00bf3db8d8f36b181b
- Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9f866b72af4a3c4520205d55c60aa74548682c4a&tochange=369a5ee3a2880a4a98df3a00bf3db8d8f36b181b
  
Other than that, there is a weird behavior of ESC key that I don't identify in https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#toolbox: it enables/disables the "Toggle split console" function (button). I encountered this situation all the way to 48.0a1 (2016-04-21).

Comment 73

[Ada [:adalucinet]]

:Honza, any ideas about the issues mentioned by Iulia in comment 72? Thanks in advance!

Comment 74

[Tim Nguyen :ntim]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #72)
> I tested this bug on 
> - 50.0a1 (2016-06-09) 
> - 49.0a2 (2016-06-09), 
> using 
> - Windows 10 x64
> - Ubuntu 14.04 x86
> - Mac OS X 10.11.
> The overall toolbars and buttons state seems to be the expected one and
> also, there is no misbehavior between the last and the present
> implementation.
> 
> There is one exception: on Ubuntu 14.04 x86 and on Mac OS X 10.11, Rules,
> Computed, Box Model and Animations side panel buttons have a "breadcrumbs"
> border when clicked. This issue is not reproducing on 48.0a1 (2016-04-21).
> [Regression range]:
> - Last good revision: 9f866b72af4a3c4520205d55c60aa74548682c4a
> - First bad revision: 369a5ee3a2880a4a98df3a00bf3db8d8f36b181b
> - Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9f866b72af4a3c4520205d55c60aa74548682c4a&tochange=369a
> 5ee3a2880a4a98df3a00bf3db8d8f36b181b
If you're talking about the dotted focusring, it was intentionally added in bug 1242851.

> Other than that, there is a weird behavior of ESC key that I don't identify
> in
> https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#toolbox:
> it enables/disables the "Toggle split console" function (button). I
> encountered this situation all the way to 48.0a1 (2016-04-21).
This is intentional, I've added it to the MDN doc.

Comment 75

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Tim Nguyen :ntim from comment #74)
> (In reply to Iulia Cristescu, QA [:IuliaC] from comment #72)
> > I tested this bug on 
> > - 50.0a1 (2016-06-09) 
> > - 49.0a2 (2016-06-09), 
> > using 
> > - Windows 10 x64
> > - Ubuntu 14.04 x86
> > - Mac OS X 10.11.
> > The overall toolbars and buttons state seems to be the expected one and
> > also, there is no misbehavior between the last and the present
> > implementation.
> > 
> > There is one exception: on Ubuntu 14.04 x86 and on Mac OS X 10.11, Rules,
> > Computed, Box Model and Animations side panel buttons have a "breadcrumbs"
> > border when clicked. This issue is not reproducing on 48.0a1 (2016-04-21).
> > [Regression range]:
> > - Last good revision: 9f866b72af4a3c4520205d55c60aa74548682c4a
> > - First bad revision: 369a5ee3a2880a4a98df3a00bf3db8d8f36b181b
> > - Pushlog:
> > https://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=9f866b72af4a3c4520205d55c60aa74548682c4a&tochange=369a
> > 5ee3a2880a4a98df3a00bf3db8d8f36b181b
> If you're talking about the dotted focusring, it was intentionally added in
> bug 1242851.
> 
> > Other than that, there is a weird behavior of ESC key that I don't identify
> > in
> > https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#toolbox:
> > it enables/disables the "Toggle split console" function (button). I
> > encountered this situation all the way to 48.0a1 (2016-04-21).
> This is intentional, I've added it to the MDN doc.

Thank you for your clarification! These being said, I am marking this issue as Verified Fixed.
Note: I'm removing the :Honza's needinfo flag since :ntim answered.