Closed Bug 1266419 Opened 4 years ago Closed 4 years ago

Create an HTML replacement for Toolbars and Toolbar buttons

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

VERIFIED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

(Depends on 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [devtools-html])

Attachments

(10 files, 20 obsolete files)

142.54 KB, image/png
Details
452.63 KB, image/png
Details
61.20 KB, image/png
Details
17.51 KB, image/png
Details
125.86 KB, image/png
Details
4.59 KB, image/png
Details
1.94 KB, patch
Honza
: review+
Honza
: ui-review+
Honza
: feedback+
Details | Diff | Splinter Review
11.01 KB, patch
Honza
: review+
Honza
: ui-review+
Details | Diff | Splinter Review
8.92 KB, patch
Honza
: review+
Details | Diff | Splinter Review
37.62 KB, patch
Honza
: review+
Honza
: ui-review+
Details | Diff | Splinter Review
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
Severity: normal → enhancement
Whiteboard: [devtools-html]
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Attached patch bug1266419-frames-menu.patch (obsolete) — Splinter Review
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
Attachment #8749147 - Flags: review?(bgrinstead)
Attached patch bug1266419.patch (obsolete) — Splinter Review
Here the the second part.

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

Honza
Attachment #8749197 - Flags: review?(bgrinstead)
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
Attached image html-icon-sizing.png
some of the UI differences I see with the patches applied
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.
(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.
Attached patch bug1266419.patch (obsolete) — Splinter Review
(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
Attachment #8749197 - Attachment is obsolete: true
Attachment #8749197 - Flags: review?(bgrinstead)
Attachment #8749634 - Flags: review?(bgrinstead)
Comment on attachment 8749634 [details] [diff] [review]
bug1266419.patch

Tim, do you have time to do a review of this patch?
Attachment #8749634 - Flags: review?(ntim.bugs)
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
Attachment #8749634 - Flags: review?(ntim.bugs) → review-
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
Attachment #8749634 - Flags: review?(bgrinstead)
See the text buttons like "Import" in this screenshot on the light theme
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.
Attachment #8749147 - Flags: review?(bgrinstead)
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 ?
Blocks: 1271127
Attached patch bug1266419.patch (obsolete) — Splinter Review
(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
Attachment #8749634 - Attachment is obsolete: true
Attachment #8750393 - Flags: review?(bgrinstead)
(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 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.
Attached patch bug1266419.patch (obsolete) — Splinter Review
(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
Attachment #8750393 - Attachment is obsolete: true
Attachment #8750393 - Flags: review?(bgrinstead)
Attachment #8750842 - Flags: review?(bgrinstead)
Comment on attachment 8750842 [details] [diff] [review]
bug1266419.patch

Tim, are you able to review this?
Attachment #8750842 - Flags: review?(bgrinstead) → review?(ntim.bugs)
Missing option labels with the patch applied
(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?
(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?
Iteration: 49.1 - May 9 → 49.2 - May 23
Attached patch bug1266419.patch (obsolete) — Splinter Review
(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
Attachment #8750842 - Attachment is obsolete: true
Attachment #8750842 - Flags: review?(ntim.bugs)
Attachment #8751684 - Flags: review?(bgrinstead)
Also, as mentioned in comment #3, what about the focus?

Honza
Flags: needinfo?(bgrinstead)
Comment on attachment 8751684 [details] [diff] [review]
bug1266419.patch

Same as Comment 19
Attachment #8751684 - Flags: review?(bgrinstead) → review?(ntim.bugs)
(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.
Flags: needinfo?(bgrinstead)
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 ?
Attachment #8751684 - Flags: review?(ntim.bugs) → review+
Attached patch bug1266419.patch (obsolete) — Splinter Review
(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
Attachment #8751684 - Attachment is obsolete: true
Attachment #8752174 - Flags: review?(bgrinstead)
Attached patch bug1266419-tests.patch (obsolete) — Splinter Review
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
Attachment #8752175 - Flags: review?(bgrinstead)
Honza, the two code patches need rebasing on top of fx-team
Flags: needinfo?(odvarko)
Attached patch bug1266419-frames-menu.patch (obsolete) — Splinter Review
Attachment #8749147 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8752248 - Flags: review?(bgrinstead)
Attached patch bug1266419.patch (obsolete) — Splinter Review
Attachment #8752174 - Attachment is obsolete: true
Attachment #8752174 - Flags: review?(bgrinstead)
Attachment #8752249 - Flags: review?(bgrinstead)
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?
Flags: needinfo?(odvarko)
Note that I still get conflicts with toolbox.js and toolbox.xul with the latest patches.
(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.
Attached patch bug1266419-tweaks.patch (obsolete) — Splinter Review
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
Attachment #8753344 - Flags: review?(bgrinstead)
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
Flags: needinfo?(odvarko)
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.
Attached patch bug1266419-tweaks.patch (obsolete) — Splinter Review
(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
Attachment #8753344 - Attachment is obsolete: true
Attachment #8753344 - Flags: review?(bgrinstead)
Attachment #8753682 - Flags: review?(bgrinstead)
Attachment #8753682 - Flags: feedback?(ntim.bugs)
Attached image 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
Attachment #8752175 - Flags: ui-review?(hholmes)
Attachment #8752248 - Flags: ui-review?(hholmes)
Attachment #8752249 - Flags: ui-review?(hholmes)
Attachment #8753682 - Flags: ui-review?(hholmes)
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
Attachment #8753682 - Flags: ui-review?(hholmes) → ui-review+
Attachment #8752249 - Flags: ui-review?(hholmes) → ui-review+
Attachment #8752248 - Flags: ui-review?(hholmes) → ui-review+
Attachment #8752175 - Flags: ui-review?(hholmes) → ui-review+
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
Attachment #8752248 - Flags: review?(bgrinstead) → review?(poirot.alex)
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+
Attachment #8752249 - Flags: review?(bgrinstead) → review+
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
Attachment #8753682 - Flags: review?(bgrinstead) → review+
Comment on attachment 8752175 [details] [diff] [review]
bug1266419-tests.patch

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

Also forwarding to Alex
Attachment #8752175 - Flags: review?(bgrinstead) → review?(poirot.alex)
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"
Attachment #8753682 - Flags: feedback?(ntim.bugs) → feedback+
(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
Attachment #8753682 - Attachment is obsolete: true
Attachment #8754269 - Flags: ui-review+
Attachment #8754269 - Flags: review+
Attachment #8754269 - Flags: feedback+
Attached patch bug1266419-frames-menu.patch (obsolete) — Splinter Review
Rebased

Honza
Attachment #8752248 - Attachment is obsolete: true
Attachment #8752248 - Flags: review?(poirot.alex)
Attachment #8754345 - Flags: review?(poirot.alex)
Attached patch bug1266419-frames-menu.patch (obsolete) — Splinter Review
Rebased
Attachment #8754345 - Attachment is obsolete: true
Attachment #8754345 - Flags: review?(poirot.alex)
Attachment #8754346 - Flags: review?(poirot.alex)
Attached patch bug1266419.patch (obsolete) — Splinter Review
Attachment #8752249 - Attachment is obsolete: true
Attachment #8754347 - Flags: ui-review+
Attachment #8754347 - Flags: review?(poirot.alex)
Attachment #8754347 - Flags: review?(poirot.alex) → review+
Attached patch bug1266419-tests.patch (obsolete) — Splinter Review
Rebased

Honza
Attachment #8752175 - Attachment is obsolete: true
Attachment #8752175 - Flags: review?(poirot.alex)
Attachment #8754348 - Flags: ui-review+
Attachment #8754348 - Flags: review?(poirot.alex)
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!
Attachment #8754346 - Flags: review?(poirot.alex) → review+
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.
(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
Attachment #8754346 - Attachment is obsolete: true
Attachment #8754801 - Flags: ui-review+
Attachment #8754801 - Flags: review+
Attached patch bug1266419.patch (obsolete) — Splinter Review
Rebased

Honza
Attachment #8754347 - Attachment is obsolete: true
Attachment #8754804 - Flags: ui-review+
Attachment #8754804 - Flags: review+
(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
@Alex, see my comment above.

Honza
Flags: needinfo?(poirot.alex)
(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.
Flags: needinfo?(poirot.alex)
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)
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Attached patch bug1266419.patch (obsolete) — Splinter Review
Rebase

Honza
Attachment #8754804 - Attachment is obsolete: true
Attachment #8756798 - Flags: ui-review+
Attachment #8756798 - Flags: review+
Attached patch bug1266419-tests.patch (obsolete) — Splinter Review
(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
Attachment #8754348 - Attachment is obsolete: true
Attachment #8754348 - Flags: review?(poirot.alex)
Attachment #8756802 - Flags: review?(poirot.alex)
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))
Attachment #8756802 - Flags: review?(poirot.alex) → review+
(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
Attachment #8756802 - Attachment is obsolete: true
Attachment #8756832 - Flags: review+
Attached patch bug1266419.patch (obsolete) — Splinter Review
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
Attachment #8756798 - Attachment is obsolete: true
Attachment #8757155 - Flags: ui-review+
Attachment #8757155 - Flags: review+
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!
Flags: needinfo?(odvarko)
Keywords: checkin-needed
Attached patch bug1266419.patchSplinter Review
Rebasing

Honza
Attachment #8757155 - Attachment is obsolete: true
Attachment #8757878 - Flags: ui-review+
Attachment #8757878 - Flags: review+
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
Flags: needinfo?(odvarko) → needinfo?(cbook)
(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!
Flags: needinfo?(cbook)
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).
Depends on: 1278889
:Honza, any ideas about the issues mentioned by Iulia in comment 72? Thanks in advance!
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(odvarko)
(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.
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(odvarko)
Depends on: 1279649
Depends on: 1280791
Depends on: 1284245
Flags: qe-verify+
Depends on: 1291618
Depends on: 1303043
Depends on: 1327971
Depends on: 1327978
Depends on: 1328000
Depends on: 1328001
Depends on: 1328010
Depends on: 1328011
Blocks: 1328011
No longer depends on: 1328011
Product: Firefox → DevTools
No longer depends on: 1284245
Regressions: 1284245
You need to log in before you can comment on or make changes to this bug.