1456849 - [Dev Tools] Adding Extra Toolbox buttons from settings will overlap the Dev Tool tabs

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Rares Doghi, Desktop QA]

Attachment: imgpsh_fullsize.png

[Affected versions]: 
Nightly 61.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Windows 7 x 32, Mac OS X 10.13 and Ubuntu 16.04 x64.

[Steps to reproduce]:

1. Open Firefox 
2. Open the Inspector by pressing F12 
3. Click on the Meatball Menu.
4. Resize to a smaller window.
5. Click the "Settings F1" button from the Meatball Menu.
6. Select all of the available Toolbox Buttons.

[Expected result]:
The Extra buttons should be displayed on the bar next to the (">>" Select another Tool) button.

[Actual result]:
The Extra buttons overlap the available tabs.

Comment 1

[Daisuke Akatsuka (:daisuke)]

I'll take a look at this.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1456849 - Part 1: Re-arrange the tool tabs if the visibility of command tools are changed.

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1456849 - Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed.

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

Comment 4

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a45e9c95e43e056ae571fc777926f1c078071e8d

Comment 5

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971481 [details]
Bug 1456849 - Part 1: Re-arrange the tool tabs if the visibility of command tools are changed.

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

Comment 6

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971482 [details]
Bug 1456849 - Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed.

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

Comment 7

[Daisuke Akatsuka (:daisuke)]

I discussed with Julian on Slack, I had missed some points.
try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a894e8cf5b95865dc2e74b38c606b6d96e193e9d

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8971482 [details]
Bug 1456849 - Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed.

https://reviewboard.mozilla.org/r/240252/#review246048

Test looks good, thanks Daisuke! 
A few suggestions in the comments, most importantly I would like to keep "overflow" in the test name.

::: commit-message-4a827:1
(Diff revision 1)
> +Bug 1456849 - Part 2: Add test whether the tool tabs are re-aranged when the visibility of toolbox button were changed. r?jdescottes

nit: re-aranged -> re-arranged

::: devtools/client/framework/test/browser.ini:116
(Diff revision 1)
>  [browser_toolbox_theme_registration.js]
>  [browser_toolbox_toggle.js]
>  [browser_toolbox_tool_ready.js]
>  [browser_toolbox_tool_remote_reopen.js]
>  [browser_toolbox_toolbar_overflow.js]
> +[browser_toolbox_toolbar_rearrange_by_buttons_visibility.js]

Since this test is a more specific "overflow" test it would be nice to keep `browser_toolbox_toolbar_overflow` as the beginning of the name. This way when you want to run tests for overflow issues you can just do `./mach test browser_toolbox_toolbar_overflow` and all the tests matching this string will be executed.

Maybe browser_toolbox_toolbar_overflow_button_visibility?

::: devtools/client/framework/test/browser_toolbox_toolbar_rearrange_by_buttons_visibility.js:14
(Diff revision 1)
> +
> +const { Toolbox } = require("devtools/client/framework/toolbox");
> +
> +add_task(async function() {
> +  const tab = await addTab("about:blank");
> +  const toolbox = await openToolboxForTab(tab, "webconsole", Toolbox.HostType.BOTTOM);

nit: here you just want to use the "options" panel so you could directly start devtools with: 

  const toolbox = await openToolboxForTab(tab, "options", Toolbox.HostType.BOTTOM);

And then you can get the panel with:

  const optionsTool = toolbox.getCurrentPanel();
  
This way you don't need to switch panels.

::: devtools/client/framework/test/browser_toolbox_toolbar_rearrange_by_buttons_visibility.js:38
(Diff revision 1)
> +               .querySelectorAll("#enabled-toolbox-buttons-box input[type=checkbox]");
> +
> +  info("Test the count of shown devtools tab after making all buttons to be visible");
> +  await resizeWindow(toolbox, 800);
> +  // Once, make all toolbox button to be invisible.
> +  await setToolboxButtonsVisibility(checkButtons, false);

nit: this is not asynchronous, we should remove the `await` keyword.

::: devtools/client/framework/test/browser_toolbox_toolbar_rearrange_by_buttons_visibility.js:40
(Diff revision 1)
> +  info("Test the count of shown devtools tab after making all buttons to be visible");
> +  await resizeWindow(toolbox, 800);
> +  // Once, make all toolbox button to be invisible.
> +  await setToolboxButtonsVisibility(checkButtons, false);
> +  // Get count of shown devtools tab elements.
> +  const tabCountOnInvisible = toolbox.doc.querySelectorAll(".devtools-tab").length;

nit: the variable name is not clear for me. Maybe use "initialTabCount" ?

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8971481 [details]
Bug 1456849 - Part 1: Re-arrange the tool tabs if the visibility of command tools are changed.

https://reviewboard.mozilla.org/r/240250/#review246054

Looks good, thanks for fixing this Daisuke!

::: devtools/client/framework/components/toolbox-toolbar.js:79
(Diff revision 2)
>        L10N: PropTypes.object,
>        // The devtools toolbox
>        toolbox: PropTypes.object,
>        // Call back function to detect tabs order updated.
>        onTabsOrderUpdated: PropTypes.func.isRequired,
> +      // Count of visible toolbox buttons.

We should mention "(Used by ToolboxTabs)" here to explain why this is not directly used in this class.

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971481 [details]
Bug 1456849 - Part 1: Re-arrange the tool tabs if the visibility of command tools are changed.

https://reviewboard.mozilla.org/r/240250/#review246054

> We should mention "(Used by ToolboxTabs)" here to explain why this is not directly used in this class.

Okay, thanks!

Comment 11

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971482 [details]
Bug 1456849 - Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed.

https://reviewboard.mozilla.org/r/240252/#review246048

> Since this test is a more specific "overflow" test it would be nice to keep `browser_toolbox_toolbar_overflow` as the beginning of the name. This way when you want to run tests for overflow issues you can just do `./mach test browser_toolbox_toolbar_overflow` and all the tests matching this string will be executed.
> 
> Maybe browser_toolbox_toolbar_overflow_button_visibility?

I agree.

> nit: here you just want to use the "options" panel so you could directly start devtools with: 
> 
>   const toolbox = await openToolboxForTab(tab, "options", Toolbox.HostType.BOTTOM);
> 
> And then you can get the panel with:
> 
>   const optionsTool = toolbox.getCurrentPanel();
>   
> This way you don't need to switch panels.

Oh, this is nice! Thanks!

Comment 12

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971481 [details]
Bug 1456849 - Part 1: Re-arrange the tool tabs if the visibility of command tools are changed.

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

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8971482 [details]
Bug 1456849 - Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed.

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

Comment 14

[Daisuke Akatsuka (:daisuke)]

I'll make the patches to land after checking the try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de0019991e1896577beaf64e7d27a645ba1d04de

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95f8c87c1d97
Part 1: Re-arrange the tool tabs if the visibility of command tools are changed. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/37463107d758
Part 2: Add test whether the tool tabs are re-arranged when the visibility of toolbox button were changed. r=jdescottes

Comment 16

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/95f8c87c1d97
https://hg.mozilla.org/mozilla-central/rev/37463107d758

Comment 17

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

Attachment: test.jpg

Comment 18

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

If I select all of the Available Toolbox Buttons and minimize the window width this issue is still reproduced for me, on Ubuntu 14.04 x64, the toolbox buttons will overlap the dev tool tabs. Please see attached file: test.jpg.

Comment 19

[Julian Descottes [:jdescottes]]

This is an edge case when only one tool can be displayed. We decided to prioritize showing the selected tool rather than showing the "chevron" menu in this case. This can be further discussed in a followup bug but in my opinion this works as intended.

Comment 20

[Hani Yacoub, Desktop QA]

The issue mentioned by Timea is also reproducible on Windows and Mac OS X, but based on comment 19 I'll mark this as verified as fixed on Firefox Nightly 62.0a1 and Firefox 61.0b3 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.