Closed Bug 1466007 Opened 6 years ago Closed 6 years ago

Add test of minimum devtools width.

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(1 file)

Bug 1466007 - Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
+++ This bug was initially created as a clone of Bug #1456056 +++

This is follow-up bug of bug 1456056 comment 11.

We need the devtool test that confirming to display only the chevron and the control buttons in toolbar when width is narrow.
Comment on attachment 8982401 [details]
Bug 1466007 - Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.

https://reviewboard.mozilla.org/r/248348/#review254716

Thanks for working on a new test Mantaroh. Is this coming from my comment on your second patch:
  "One small additional comment, maybe we should have a follow-up add a test for this behavior change?"
  
I should have been clearer but the behavior change I was referring to here was the fact that selected tool is not displayed when the width is too small.
Testing the width of the rendered buttons and button containers as we do here seems a bit fragile. 

Could we repurpose this test to check that when then toolbox is at minimum width, all the tools - including the selected tool - are in the chevron button?
I will leave some comments in the test to explain why I am not comfortable with going forward with this.

::: devtools/client/framework/test/browser_toolbox_toolbar_minimum_width.js:13
(Diff revision 1)
> +[{ picker: true,  commands: true  },
> + { picker: true,  commands: false },
> + { picker: false, commands: true  },
> + { picker: false, commands: false }].forEach(test => {

I'm not sure we should keep testing all those configurations. It will make the test hard to maintain if we add or remove buttons. 

The test should work regardless of which buttons have been added.

::: devtools/client/framework/test/browser_toolbox_toolbar_minimum_width.js:24
(Diff revision 1)
> +   });
> + });
> +
> +async function testFunc(pickerEnable, commandsEnable) {
> +  // 74px is Chevron(26px) + Meatball(24px) + Close(24px)
> +  setDevtoolWidth(74);

We can set the width to 0 from the preferences. Add a comment explaining that this will force the devtools to be as small as allowed by the CSS.

::: devtools/client/framework/test/browser_toolbox_toolbar_minimum_width.js:56
(Diff revision 1)
> +    "devtools.command-button-frames.enabled",
> +    "devtools.command-button-measure.enabled",
> +    "devtools.command-button-paintflashing.enabled",
> +    "devtools.command-button-responsive.enabled",
> +    "devtools.command-button-rulers.enabled",
> +    "devtools.command-button-scratchpad.enabled",

Related to my previous comment about not testing all those various configurations, this hardcoded list buttons that happen to be the in the "buttons-end" container will be hard to maintain.

::: devtools/client/framework/test/browser_toolbox_toolbar_minimum_width.js:77
(Diff revision 1)
> +  let chevronMenuButton = toolbox.doc.querySelector(".tools-chevron-menu");
> +  is(chevronMenuButton.getBoundingClientRect().width, 26,
> +     "The chevron button is displayed with correctly width ");
> +
> +  let meatballButton = toolbox.doc.getElementById("toolbox-meatball-menu-button");
> +  is(meatballButton.getBoundingClientRect().width, 24,
> +     "The meatball button is display with correctly width");
> +
> +  let closeButton = toolbox.doc.getElementById("toolbox-close");
> +  is(closeButton.getBoundingClientRect().width, 24,

Not really sure about this, we are testing the width, but that doesn't tell us if the buttons are visible (they could be overflowed if we change the way we show/hide things in the toolbar)
Attachment #8982401 - Flags: review?(jdescottes)
Comment on attachment 8982401 [details]
Bug 1466007 - Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.

https://reviewboard.mozilla.org/r/248348/#review254716

Thank you so much the review.

Sorry. I misunderstood this test purpose. I repurposed this test to check new chevron behavior.

> We can set the width to 0 from the preferences. Add a comment explaining that this will force the devtools to be as small as allowed by the CSS.

I left the comment here.
Comment on attachment 8982401 [details]
Bug 1466007 - Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.

https://reviewboard.mozilla.org/r/248348/#review255394

This test looks good, thanks!

::: commit-message-42880:1
(Diff revision 2)
> +Bug 1466007 - Add devtool test of minimum width. r?jdescottes

This commit message is a bit vague, could we mention "toolbar"?
Attachment #8982401 - Flags: review?(jdescottes) → review+
Comment on attachment 8982401 [details]
Bug 1466007 - Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.

https://reviewboard.mozilla.org/r/248348/#review255394

Thanks, Julian.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/10c4e77579ca
Add test which checking that the all tab will go into overflow menu if devtool's toolbar width is minimum width.r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/10c4e77579ca
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: