1456056 - Reduce devtools min-width more (and avoid to hide the chevron menu for devtools-tab)

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P3)

Description

[Daisuke Akatsuka (:daisuke)]

Although we will reduce devtools min-width to 250px when docked to side in bug 1453294, there are the opinions that we can reduce more. However if the width is narrow, the chevron will hide now. As Brian said in bug 1453294 comment 9, avoiding to hide the chevron is desirable.

In this bug, we need to modify following:
1. Re-define the min-width at devtools-browser.css[1].
2. Avoiding to hide the chevron menu[2]

[1] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/themes/devtools-browser.css#8-10
[2] https://searchfox.org/mozilla-central/source/devtools/client/framework/components/toolbox-tabs.js

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

I will steal this.

We can change Tooltabs to display only the chevron menu in the case of narrow width. However, If another tool button is enabled, I think that this definition can't reduce drastically. 

Following size is each button size:
----------------------------------------------------
 ToolboxStart      :  33px
  * Inspector      :  32px
  * Separator      :   1px
----------------------------------------------------
 ToolboxTabs       :  26px
  * Chevron        :  26px (Excluding the padding)
----------------------------------------------------
 ToolboxEnd        : 187px
  * Frames         :  32px
  * Paint Flashing :  24px
  * Scratchpad     :  24px
  * Screenshot     :  24px
  * Ruler          :  24px
  * Measure        :  24px
  * Separator      :  11px (Including the padding)
  * RDM            :  24px
----------------------------------------------------
 ToolboxControls   :  48px
  * Meatball       :  24px
  * Close          :  24px
----------------------------------------------------

Actually, We might need to define max-wdith is 294px if we want to display all buttons.
However, As mentioned by Julian in bug 1453294 comment 10, it might be acceptable since default setting is disabled these buttons.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

If we will re-define min-width of the toolbox more reduce, the close button will be hidden.

Furthermore, the 'WINDOW' host type is same to this as well. So we will need to add style to this XUL window. For now, we can resize the separated devtool window to thin width.

This is experimental implementation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7cfaed9045225e84a7f8c6688c0fa29a43be6fa

Comment 3

[Victoria Wang [:victoria]]

Hi Mantaroh, thanks for the handy ASCII chart :)

Related to this bug: During the workweek, there was discussion of a next-gen tab bar that would, at small widths, show all toolbox tabs as icons with no overflow behavior. https://mozilla.invisionapp.com/share/ADJ5EG8QBPV

I'm not clear on how far in the future the redesign task is, and if it would be good to complete this bug as a stop gap before the redesign to allow for smaller widths. I do agree we can start to hide ToolboxEnd icons (making them unusable) to allow for smaller widths, so the most minimal toolbar could be [ToolboxStart][Chevron][ToolboxControls].

We didn't really think through the tiny-widths problem for the new no-overflow concept - maybe we do need some overflow behavior, but we could similarly start with unusuably hiding the ToolboxEnd icons, then hide Toolbox tabs one by one, then things could disappear right to left except the close button, until DevTools is just big enough for the close button.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Narrow devtool width behavior.

Thanks Victoria,

I tried that toolbox will end up to be [ToolboxStart][Chevron][ToolboxControls]. In this case, min-width is 107px.

We need to consider the following cases:

 * Picker is disabled.                     : Toolbar doesn't have a [ToolboxStart] element.
 * Close button on WINDOW host type.       : Toolbar doesn't have a close button of [ToolboxControls].
 * All of 'ToolboxEnd' buttons is disabled.: Toolbar doesn't have a [ToolboxEnd] element.

I think that we can implement this behavior by using Grid-layout, however, we need to change grid-template-columns dynamically in ToolboxToolbar.js.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #4)
> Created attachment 8980177 [details]
> Narrow devtool width behavior.
> 
> Thanks Victoria,
> 
> I tried that toolbox will end up to be
> [ToolboxStart][Chevron][ToolboxControls]. In this case, min-width is 107px.
> 
> We need to consider the following cases:
> 
>  * Picker is disabled.                     : Toolbar doesn't have a
> [ToolboxStart] element.
>  * Close button on WINDOW host type.       : Toolbar doesn't have a close
> button of [ToolboxControls].
>  * All of 'ToolboxEnd' buttons is disabled.: Toolbar doesn't have a
> [ToolboxEnd] element.
> 
> I think that we can implement this behavior by using Grid-layout, however,
> we need to change grid-template-columns dynamically in ToolboxToolbar.js.

Oh, Sorry.
The minimum case is [Chevron][ToolboxControls] since we can disable style picker. So min-width is 74px or 50px. (close button is based on host type. i.e. Window type or side type or bottom type)

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

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1456056 - Part 1. Improve the performance of toolbar tabs overflow.

This patch will two impprove performances:

 * Remove unnecesarry reflow by using the DOMWindowUtils.getBoundsWithoutFlushing().
   This is a tiny performance improvement. Previous code will reflow on each
   tab width caclculation.

 * Change requestIdleCallback's timeout to 100ms.
   If user resize the devtool's width over time, overflow calculation will occur
   every 300ms. This patch will reduce this delay.

Review commit: https://reviewboard.mozilla.org/r/246644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246644/

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

This patch will allow that all buttons which accessing the tool panel is
overflowed. i.e. toolbar will display chevron button only.

Review commit: https://reviewboard.mozilla.org/r/246646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246646/

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

This patch will apply the grid layout to the toolbar.
If devtools's width is narrow, we expected that devtool display chevron and the
controls elements only(i.e. chevron and meatball and close button).
In order to display these button, a wrapper of toolbar will use grid layout.

Basically, this patch define grid columns as follow:

--------------------------------------------------
| Picker   |  tooltabs  |  commands  | controls  |
| 0 ~ 33px | 26px ~ 1fr | 0px ~ auto | 24 or 48px|
--------------------------------------------------

We can disable the picker and command buttons, in this case, a toolbar will
stretch the tooltabs width by using grid-column-start/end.

Furthermore, WINDOW host type will not display close button, in this case,
ToolboxToolbar will change the grid-template-column.

Review commit: https://reviewboard.mozilla.org/r/246670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246670/

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8980500 [details]
Bug 1456056 - Part 1. Improve the performance of toolbar tabs overflow.

https://reviewboard.mozilla.org/r/246644/#review253256

Some nits, but this looks good, thanks for the patch Mantaroh.

::: devtools/client/framework/components/ToolboxTabs.js:113
(Diff revision 1)
>    /**
>     * Update the Map of tool id and tool tab width.
>     */
>    updateCachedToolTabsWidthMap() {
>      let thisNode = findDOMNode(this);
> +    let win = thisNode.ownerDocument.defaultView;

You can use the "window" global here.

::: devtools/client/framework/components/ToolboxTabs.js:114
(Diff revision 1)
>     * Update the Map of tool id and tool tab width.
>     */
>    updateCachedToolTabsWidthMap() {
>      let thisNode = findDOMNode(this);
> +    let win = thisNode.ownerDocument.defaultView;
> +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

eslint: line is too long

::: devtools/client/framework/components/ToolboxTabs.js:115
(Diff revision 1)
>     */
>    updateCachedToolTabsWidthMap() {
>      let thisNode = findDOMNode(this);
> +    let win = thisNode.ownerDocument.defaultView;
> +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    // For performance, bring the reflow first time.

Can we make the comment more explicit, for instance:

"Force a reflow before calling getBoundsWithoutFlushing on each tab".

::: devtools/client/framework/components/ToolboxTabs.js:116
(Diff revision 1)
>    updateCachedToolTabsWidthMap() {
>      let thisNode = findDOMNode(this);
> +    let win = thisNode.ownerDocument.defaultView;
> +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    // For performance, bring the reflow first time.
> +    getComputedStyle(thisNode).width;

If we just need to force a reflow, maybe reading "thisNode.clientWidth" would be enough? There a few spots in the codebase that already do this to force a reflow.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8980501 [details]
Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

https://reviewboard.mozilla.org/r/246646/#review253266

Great, thanks Mantarhoh.

::: devtools/client/framework/components/ToolboxTabs.js:169
(Diff revision 1)
>          let removingToolId  = visibleTabs.pop();
>          let removingToolWidth = this._cachedToolTabsWidthMap.get(removingToolId);
>          sumWidth -= removingToolWidth;
>        }
> +
> +      if ((sumWidth + selectedToolWidth) <= toolboxWidth) {

Can we add a small comment here to explain when this check is important. Which is only when all other tools have already been removed but we still don't have enough space for the selected tool, right?

Otherwise since the condition is very similar to the one used in the while above, it can be a bit confusing.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8980501 [details]
Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

https://reviewboard.mozilla.org/r/246646/#review253266

Sorry about the typo in your name! 
One small additional comment, maybe we should have a follow-up add a test for this behavior change?

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253278

Thanks for the patch, this works really well and the toolbar looks nice even when resized to very small width! 
I have some comments and I would like somebody else to check the pattern used with CSS grid here.

::: devtools/client/framework/components/ToolboxTabs.js:272
(Diff revision 1)
> +    let gridColumnStyle = "toolbox-tabs-wrapper-grid";
> +    if (!hasStartSibling) {
> +      gridColumnStyle += "-start";
> +    }
> +    if (!hasEndSibling) {
> +      gridColumnStyle += "-end";
> +    }

In general, when referencing strings from CSS or localization files, I found it better to avoid concatenations. In this case explicitly have "toolbox-tabs-wrapper-grid-start" rather than `something + "-start"`. When someone spots "toolbox-tabs-wrapper-grid-start" in the CSS they will be able to quickly find where it is set.

Now here we have 4 classes in total:
- toolbox-tabs-wrapper-grid
- toolbox-tabs-wrapper-grid-start
- toolbox-tabs-wrapper-grid-end
- toolbox-tabs-wrapper-grid-start-end

Plus the default class of the element "toolbox-tabs-wrapper". 

Maybe we could just have two extra classes? "toolbox-tabs-wrapper-has-start", "toolbox-tabs-wrapper-has-end"?

::: devtools/client/framework/components/ToolboxToolbar.js:95
(Diff revision 1)
> +                            id: (this.props.canCloseToolbox
> +                                 ? ""
> +                                 : "devtools-tabbar-without-close-button")};

Use a class instead of an id here.

::: devtools/client/framework/components/ToolboxToolbar.js:107
(Diff revision 1)
> -          renderToolboxButtonsEnd(this.props),
> +            {},
> +            this.props,
> +            { hasStartSibling: !!startButtons,
> +              hasEndSibling: !!endButtons})),

nit: could we create the props outside of this return statement? let toolboxTabsProps = Object.assign(...) right before the return for instance.

But rather than passing new props to ToolboxTabs here, since the startButtons and endButtons are created inside ToolboxToolbar, why not add the new css classes here? "devtools-tabbar-has-start" "devtools-tabbar-has-end". I don't think there is much added value in doing this in ToolboxTabs.

::: devtools/client/themes/toolbox.css:30
(Diff revision 1)
>    -moz-appearance: none;
> -  display: flex;
> +  /* For narrow devtool width, we define the each column width of tabbar.
> +    This value will be changed by target element. i.e. Picker element will 33px.
> +    This grid-template-columns will generate in ToolboxToolbar.js. */
> +  display: grid;
> +  grid-template-columns: minmax(0px, 33px) minmax(26px, 1fr) minmax(0px, auto) 48px;

We should add a comment to explain:
- what is inside each column (for instance what you have in the commit message)
- where are the hardcoded sizes coming from

::: devtools/client/themes/toolbox.css:46
(Diff revision 1)
> +.toolbox-tabs-wrapper-grid {
> +  grid-column: 2 / 3;
> +}
> +.toolbox-tabs-wrapper-grid-start {
> +  grid-column : 1 / 3;
> +}
> +.toolbox-tabs-wrapper-grid-end {
> +  grid-column : 2 / 4;
> +}
> +.toolbox-tabs-wrapper-grid-start-end {
> +  grid-column : 1 / 4;
> +}

I will ask Nicolas Chevobbe to review this, I am not familiar enough with grid here.

::: devtools/client/themes/toolbox.css:87
(Diff revision 1)
> +#toolbox-button-start {
> +  grid-column: 1 / 2;
> +}
> +#toolbox-button-end {
> +  grid-column: 3 / 4;
> +}
> +#toolbox-button-controls {
> +  grid-column: 4 / 5;

Those classes do not match any element

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

Nicolas, could you have a look at the patch here, especially the parts related to the usage of grid? Depending on the configuration, we display a different number of columns and I am not sure this should be done with grid-column or with grid-template-columns

Thanks!

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253290

::: devtools/client/framework/components/ToolboxTabs.js:272
(Diff revision 1)
>        return null;
>      });
>  
> +    // Stretch the tabs wrapper width if toolbox doesn have the previous or
> +    // after sibling element.
> +    let gridColumnStyle = "toolbox-tabs-wrapper-grid";

could we remove the "-grid" suffix ? I find it better if we don't describe what we are going to do in CSS in classnames (because we might change it)

::: devtools/client/framework/components/ToolboxTabs.js:272
(Diff revision 1)
> +    let gridColumnStyle = "toolbox-tabs-wrapper-grid";
> +    if (!hasStartSibling) {
> +      gridColumnStyle += "-start";
> +    }
> +    if (!hasEndSibling) {
> +      gridColumnStyle += "-end";
> +    }

Could we rename gridColumnStyle to classnames ? it would better conveys what it will be used for.
And then, we could avoid to do string concatenation on the classnames but have multiple ones: 

```js
const classnames = ["toolbox-tabs-wrapper"];
if (!hasStartSibling) {
  classnames.push("toolbox-tabs-wrapper-first")
}
if (!hasEndSibling) {
  classnames.push("toolbox-tabs-wrapper-last")
}

return div({
  className: classnames.join(" "),
  ...
```

or something similar

::: devtools/client/framework/components/ToolboxToolbar.js:92
(Diff revision 1)
>     * The render function is kept fairly short for maintainability. See the individual
>     * render functions for how each of the sections is rendered.
>     */
>    render() {
> -    const containerProps = {className: "devtools-tabbar"};
> +    // If toolbox doesn't have a close button, we need to change the wrapper's
> +    // grid template column style since 'toolbox-contols' element width might be

nit: s/toolbox-contols/toolbox-controls

::: devtools/client/themes/toolbox.css:30
(Diff revision 1)
>    -moz-appearance: none;
> -  display: flex;
> +  /* For narrow devtool width, we define the each column width of tabbar.
> +    This value will be changed by target element. i.e. Picker element will 33px.
> +    This grid-template-columns will generate in ToolboxToolbar.js. */
> +  display: grid;
> +  grid-template-columns: minmax(0px, 33px) minmax(26px, 1fr) minmax(0px, auto) 48px;

Could we put a wireframe of the layout in a comment (like in the commit message), so it's easier to understand what the grid is doing ?

A few questions/comments: 

- First column is minmax(0,33px), meaning it can takes any values between those, which can be weird. I think what we want here is to have the column to be auto , and let the inspect element defines its width (33px).

- I don't really understand what we are trying to do on the third column. minmax(0px, auto) means it's max size will be what we actually want, and that it can shrink down to 0. I think having it as auto is what we want (i.e., having no controls would make the column 0) ?

- Last column is either 48 or 24px (if no close button). I guess we could have it as auto, and let the children declare its width, instead of re-declaring a grid template. Having 2 templates to maintain can lead to errors I think, so it's better to have one, flexible, and handle widths in children where we can.

::: devtools/client/themes/toolbox.css:46
(Diff revision 1)
> +.toolbox-tabs-wrapper-grid {
> +  grid-column: 2 / 3;
> +}
> +.toolbox-tabs-wrapper-grid-start {
> +  grid-column : 1 / 3;
> +}
> +.toolbox-tabs-wrapper-grid-end {
> +  grid-column : 2 / 4;
> +}
> +.toolbox-tabs-wrapper-grid-start-end {

If we have different classnames for "first" and "last", then we can only override grid-column-start and grid-column-end , which would be a bit simpler to read.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980500 [details]
Bug 1456056 - Part 1. Improve the performance of toolbar tabs overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246644/diff/1-2/

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980501 [details]
Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246646/diff/1-2/

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246670/diff/1-2/

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980500 [details]
Bug 1456056 - Part 1. Improve the performance of toolbar tabs overflow.

https://reviewboard.mozilla.org/r/246644/#review253256

> If we just need to force a reflow, maybe reading "thisNode.clientWidth" would be enough? There a few spots in the codebase that already do this to force a reflow.

Yes, The reflow is occured if I tried to access the clientWidth property. So I addressed it.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980501 [details]
Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

https://reviewboard.mozilla.org/r/246646/#review253266

NP : )

> Can we add a small comment here to explain when this check is important. Which is only when all other tools have already been removed but we still don't have enough space for the selected tool, right?
> 
> Otherwise since the condition is very similar to the one used in the while above, it can be a bit confusing.

I addressed it.

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253278

> In general, when referencing strings from CSS or localization files, I found it better to avoid concatenations. In this case explicitly have "toolbox-tabs-wrapper-grid-start" rather than `something + "-start"`. When someone spots "toolbox-tabs-wrapper-grid-start" in the CSS they will be able to quickly find where it is set.
> 
> Now here we have 4 classes in total:
> - toolbox-tabs-wrapper-grid
> - toolbox-tabs-wrapper-grid-start
> - toolbox-tabs-wrapper-grid-end
> - toolbox-tabs-wrapper-grid-start-end
> 
> Plus the default class of the element "toolbox-tabs-wrapper". 
> 
> Maybe we could just have two extra classes? "toolbox-tabs-wrapper-has-start", "toolbox-tabs-wrapper-has-end"?

OK. As commented by Nicolas, I used the '-first' and '-last' suffix, then I separted this concated classname.

> Use a class instead of an id here.

If I use the "grid-template-columns: ... auto" style, this id is no longer needed.

> nit: could we create the props outside of this return statement? let toolboxTabsProps = Object.assign(...) right before the return for instance.
> 
> But rather than passing new props to ToolboxTabs here, since the startButtons and endButtons are created inside ToolboxToolbar, why not add the new css classes here? "devtools-tabbar-has-start" "devtools-tabbar-has-end". I don't think there is much added value in doing this in ToolboxTabs.

OK. I added the style in ToolboxToolbar componet.

> We should add a comment to explain:
> - what is inside each column (for instance what you have in the commit message)
> - where are the hardcoded sizes coming from

As commented by Nicolas, I fixed this grid columns.

> I will ask Nicolas Chevobbe to review this, I am not familiar enough with grid here.

Thanks forwarding to Nicolas, I fixed this grid column style.

> Those classes do not match any element

Ah, sorry.
This is my mistake that I forget removing experimental code.

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253290

Thanks, Nicolas.

> could we remove the "-grid" suffix ? I find it better if we don't describe what we are going to do in CSS in classnames (because we might change it)

OK. I removed this suffix.

> Could we rename gridColumnStyle to classnames ? it would better conveys what it will be used for.
> And then, we could avoid to do string concatenation on the classnames but have multiple ones: 
> 
> ```js
> const classnames = ["toolbox-tabs-wrapper"];
> if (!hasStartSibling) {
>   classnames.push("toolbox-tabs-wrapper-first")
> }
> if (!hasEndSibling) {
>   classnames.push("toolbox-tabs-wrapper-last")
> }
> 
> return div({
>   className: classnames.join(" "),
>   ...
> ```
> 
> or something similar

OK. I separated this classes. As mentioned Julian, I moved this code to the ToolboxToolbar. As result of it, I used the descendant combinator of devtools-tabbar and toolbox-tabs-wrapper.

> nit: s/toolbox-contols/toolbox-controls

Thanks.

> Could we put a wireframe of the layout in a comment (like in the commit message), so it's easier to understand what the grid is doing ?
> 
> A few questions/comments: 
> 
> - First column is minmax(0,33px), meaning it can takes any values between those, which can be weird. I think what we want here is to have the column to be auto , and let the inspect element defines its width (33px).
> 
> - I don't really understand what we are trying to do on the third column. minmax(0px, auto) means it's max size will be what we actually want, and that it can shrink down to 0. I think having it as auto is what we want (i.e., having no controls would make the column 0) ?
> 
> - Last column is either 48 or 24px (if no close button). I guess we could have it as auto, and let the children declare its width, instead of re-declaring a grid template. Having 2 templates to maintain can lead to errors I think, so it's better to have one, flexible, and handle widths in children where we can.

I can use "auto minmax(26px, 1fr) auto max-content". As you pointed, first and third column should be 'auto'. Then we can use 'max-content' keyword to last column. If We use 'auto' to last column, close and meatball button will shrink.

> If we have different classnames for "first" and "last", then we can only override grid-column-start and grid-column-end , which would be a bit simpler to read.

OK. I used grid-column-start and grid-column-end property. Thanks.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Screenshot at May 29 17-58-01.png

I tested the last patch and I can get at a point where some icons are half-shown. Is this wanted ?

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253708

Thanks for the update! 
r+ on my side, but I'm looking forward to see if the problem detected by Nicolas can be reproduced and fixed.

::: devtools/client/framework/components/ToolboxToolbar.js:91
(Diff revision 2)
> -    const containerProps = {className: "devtools-tabbar"};
> +    // If toolbox doesn't have a close button, we need to change the wrapper's
> +    // grid template column style since 'toolbox-controls' element width might be
> +    // changed.

This comment is no longer relevant I think?

::: devtools/client/framework/components/ToolboxToolbar.js:97
(Diff revision 2)
> +    // grid template column style since 'toolbox-controls' element width might be
> +    // changed.
> +    let classnames = ["devtools-tabbar"];
> +    let startButtons = renderToolboxButtonsStart(this.props);
> +    let endButtons = renderToolboxButtonsEnd(this.props);
> +    let toolboxTabs = ToolboxTabs(this.props);

extracting this is no longer necessary since we don't have the huge Object.assign() call :)

::: devtools/client/framework/components/ToolboxToolbar.js:99
(Diff revision 2)
> +    // Stretch the tool tabs wrapper width if toolbox doesn have the previous or
> +    // before element. (i.e. start buttons or end buttons element)

This comment would probably be better in the CSS next to the styles that actually do this.

Some nits:
- "if toolbox doesn" "if the toolbox doesn't"
- "previous of before" did you mean "previous or next"? Maybe only refer to those elements as start buttons and end buttons?

::: devtools/client/framework/components/ToolboxToolbar.js:102
(Diff revision 2)
> +    let toolboxTabs = ToolboxTabs(this.props);
> +
> +    // Stretch the tool tabs wrapper width if toolbox doesn have the previous or
> +    // before element. (i.e. start buttons or end buttons element)
> +    if (!startButtons) {
> +      classnames.push("devtools-tabbar-first");

Not sure about the names used here (-first, -last).

This was ~ok when applied to the ToolboxTabs element, because having no start-buttons meant the element was the "first" child. (incorrect for "last" though, since there might be a close button as the last child)

(my previous suggestion would give "devtools-tabbar-has-start" or "devtools-tabbar-has-startbuttons" here, but feel free to use something else)

::: devtools/client/themes/toolbox.css:26
(Diff revision 2)
>  
>  /* Toolbox tabbar */
>  
>  .devtools-tabbar {
>    -moz-appearance: none;
> -  display: flex;
> +  /* For narrow devtool width, we define the each column width of tabbar. */

Could we add the ASCII based layout you have in your commit message here? This would help understand which column contains what.

::: devtools/client/themes/toolbox.css:45
(Diff revision 2)
>  
> +.devtools-tabbar .toolbox-tabs-wrapper {
> +  grid-column-start: 2;
> +  grid-column-end: 3;
> +}
> +.devtools-tabbar-first .toolbox-tabs-wrapper {

add a new line between the two blocks

::: devtools/client/themes/toolbox.css:48
(Diff revision 2)
> +  grid-column-end: 3;
> +}
> +.devtools-tabbar-first .toolbox-tabs-wrapper {
> +  grid-column-start: 1;
> +}
> +.devtools-tabbar-last .toolbox-tabs-wrapper {

ditto: new line

Comment 24

[Mantaroh Yoshinaga[:mantaroh]]

Thanks, Nicolas

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #22)
> Created attachment 8981491 [details]
> Screenshot at May 29 17-58-01.png
> 
> I tested the last patch and I can get at a point where some icons are
> half-shown. Is this wanted ?

Toolbox will end up '[Tool Tabs][Controls]'. So other wrapper element will be overflowed now. This style is aim preventing overflow the close button of toolbox. Should we hide if picker or commands element is overflowed? What do you think of this behavior?

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

I'm really not sure what should happen. Ideally, each time a "toolbar entry" (tab, icon, ...) start to be clipped, it should be hidden and put under the meatball menu IMO. Is this something achievable in this patch or should we create another bug to handle this ?

Comment 26

[Julian Descottes [:jdescottes]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #25)
> I'm really not sure what should happen. Ideally, each time a "toolbar entry"
> (tab, icon, ...) start to be clipped, it should be hidden and put under the
> meatball menu IMO. Is this something achievable in this patch or should we
> create another bug to handle this ?

IMO definitely out of scope for this bug. But that is something we should consider for the improved meatball menu Brian is working on in Bug 1461522.

I assumed the screenshot was with the minimum width, and I thought it would be nice to make sure the tabbar looks good when reaching min-width (ie, no half icons displayed). But if it's just an intermediary stage I think it's fine as is. Below a certain width, I feel like users will mostly just want to make devtools as small as possible (because they need to reduce them temporarily to test something) rather than trying to use them at 110px, 90px, 74px etc...

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review254014

One small comment, otherwise I agree with Julian's ones. 
Let's see how we can handle the "clipped" icons in a follow-up.

::: commit-message-ebda9:18
(Diff revision 2)
> +Furthermore, WINDOW host type will not display close button, in this case,
> +ToolboxToolbar will change the grid-template-column.

it's not true anymore right ?

Comment 28

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980500 [details]
Bug 1456056 - Part 1. Improve the performance of toolbar tabs overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246644/diff/2-3/

Comment 29

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980501 [details]
Bug 1456056 - Part 2. Display chevron button only if the devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246646/diff/2-3/

Comment 30

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246670/diff/2-3/

Comment 31

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review253708

Thank you for the review!

> This comment is no longer relevant I think?

Ah, Yes. This is unnecessary comment. I removed it and I checked the comment of this changes again.

> This comment would probably be better in the CSS next to the styles that actually do this.
> 
> Some nits:
> - "if toolbox doesn" "if the toolbox doesn't"
> - "previous of before" did you mean "previous or next"? Maybe only refer to those elements as start buttons and end buttons?

Thanks.
I removed this comment to the toolbox.css

> Not sure about the names used here (-first, -last).
> 
> This was ~ok when applied to the ToolboxTabs element, because having no start-buttons meant the element was the "first" child. (incorrect for "last" though, since there might be a close button as the last child)
> 
> (my previous suggestion would give "devtools-tabbar-has-start" or "devtools-tabbar-has-startbuttons" here, but feel free to use something else)

OK.
I renamed this classnames to 'devtools-tabbar-has-...'.

Comment 32

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8980502 [details]
Bug 1456056 - Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow.

https://reviewboard.mozilla.org/r/246670/#review254014

Thank you so much for review!

> it's not true anymore right ?

No. This is no longer needed. So I removed it. thanks.

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

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

Comment 34

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #33)
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9cae741fba301e7d2c7b07e7f5e3ea15e585b133

Maybe, This test failure is the problem of taskcluster. As far as I can see, RPK and l10n task will search mar files, however, artifact builds doesn't have mar update files.

If I use the --no-artifact option, test will success:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a493ddcdb7df64201c31fd35fe6a02eff2e165d

Comment 35

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #34)
> Maybe, This test failure is the problem of taskcluster. As far as I can see,
> RPK and l10n task will search mar files, however, artifact builds doesn't
> have mar update files.
> 
This is the bug 1465836. I’ll land it since this failure is not related with this bug.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2995e423a7b2
Part 1. Improve the performance of toolbar tabs overflow. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a2fe3122cc91
Part 2. Display chevron button only if the devtool width is narrow.r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/996ab33915e7
Part 3. Use the grid layout to a toolbar in order to display only the chevron button and the controls element if devtool's width is narrow. r=jdescottes,nchevobbe

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2995e423a7b2
https://hg.mozilla.org/mozilla-central/rev/a2fe3122cc91
https://hg.mozilla.org/mozilla-central/rev/996ab33915e7

Comment 38

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

Attachment: close-button-hide.gif

Comment 39

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

The issue is still reproducible for me on 63.0a1 (2018.08.30) and 62.0. What I observed, if zoom in the inspector window to the highest possible zoom level and then change the inspector window width the close button disappear as I attached in close-button-hide.gif.

Comment 40

[Julian Descottes [:jdescottes]]

The initial issue was about reducing the min-width for DevTools. The issue highlighted in comment 38 and comment 39 is a follow up in my opinion and should be moved to its own bug?

Comment 41

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #40)
> The initial issue was about reducing the min-width for DevTools. The issue
> highlighted in comment 38 and comment 39 is a follow up in my opinion and
> should be moved to its own bug?

Thanks, Timea, Julian.

I think we should to this width multiply by zoom value. I can fix this bug by using the patch which I wrote previously.  I'll file the new bug for this. (bug 1468177 comment 1)

Comment 42

[Rares Doghi, Desktop QA]

Hi, I can confirm this issue as fixed in Firefox 64.0, Nightly 66 and Beta 65.0b6.