The buttons on the right of the DevTools tab bar are unclear

VERIFIED FIXED in Firefox 61

Status

enhancement
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

Attachments

(18 attachments, 3 obsolete attachments)

4.41 KB, image/png
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Harald
: review-
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
110.06 KB, image/png
Details
Assignee

Description

a year ago
Posted image Toolbar buttons
This is almost certainly a dupe but I've heard a lot of people comment that the buttons on the right-hand side of the toolbar are confusing.

See the attached screenshot.

I wonder if anyone can tell, at a glance, which icon is for split console vs changing the alignment of the panel? It gets even more confusing when you're in vertical split to distinguish between vertical split / RDM / iframe / console. I have to hover and read the tooltips.

Also, the cog is actually a tab (hence the different styling) while the others are action buttons or toggle buttons. Should the cog really be nested amongst buttons like this?

There is a vertical line on the right of the cog. What does this mean?

Safari solves this by putting buttons on a separate row so that all the "things" in the tab row are actually tabs.

Chrome solves this by having a separate "Dock side" entry in the drop-down menu.
Assignee

Updated

a year ago
Summary: The buttons on the right of the Developer toolbar are unclear → The buttons on the right of the DevTools tab bar are unclear
Victoria, I saw a prototype image that Brian showed me for this bug (I am not sure it's OK to publish here).  It seems to me that 'selecting iframe' icon has gone somewhere.  Does that mean the icon moved into a context menu?
Flags: needinfo?(victoria)
Hi Hiroyuki, great question. I don't use the optional toolbox buttons (besides Picker and RDM) and I forgot to account for them in my mockup (which can be published here - https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens/283871189 )

I have some questions about these - will post in Slack too to get more feedback.

- Do we have telemetry (or some good estimate) for how often these optional toolbox buttons are used?
- For any significantly-used buttons, which need the setting to be a top-level button? E.g. Could we move some of these into the meatball menu, into a new Tools submenu? (maybe Scratchpad, ruler toggle?)
Flags: needinfo?(victoria)
Some conclusions after conversations on slack about this:

It sounds like for the first version of this change, we shouldn't get rid of users' ability to add any optional toolbox buttons they want. It would be great to collect telemetry on those options to give us data for future reshuffling. For a later version, some features like selecting iframes could be improved and better-integrated (added into the picker?), and other features could be moved to the meatball menu. For v0, the meatball menu will remove a small amount of clutter by replacing only the settings icon + 2 docking icons, which is still a win - most users will see half as many icons on the right as they currently do.

Additional fix that would help the appearance of this area - don't show the selecting iframes icon if there are no iframes in the current page.

Updated the mockup to reflect this 'version 1' idea:
https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens/283871189
I filed bug 1446559 for telemetry about the toolbox buttons.
See Also: → 1446559
Assignee

Comment 5

a year ago
I started working on this just for fun, but I was curious about the "Split console" button. Should we move that to the menu too?
Assignee

Comment 6

a year ago
It looks like Chrome sticks the split console item in the meatball menu. I might include a patch to do that (and probably hide the menu item when the console tab is selected).
Assignee

Comment 7

a year ago
Posted image Right menu in progress (obsolete) —
I'm not sure when I'll get to this next. So far here's how it looks.

It still needs the "Split console" item to be moved into the menu and the iframes item to be conditionally hidden.

We really need to fix those focus styles too -- they look horrible. That's a separate bug though.
Assignee

Updated

a year ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee

Comment 8

a year ago
WIP try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a66fce0c2f70488eff4d0ecc1844fc05b10cfe4d

Still to do:

* Move the noautohide feature to the menu
* Lighten the icons as per mock-up (light theme only I expect, about #8d8d8d)
* Work out what the help menu items are supposed to link to (especially "Give Feedback")
* A few tweaks to naming in first patch
* Tests
(In reply to Brian Birtles (:birtles) from comment #8)
> Still to do:
> * Lighten the icons as per mock-up (light theme only I expect, about #8d8d8d)

Turns out we have exact colours specified for you! ;D
https://design.firefox.com/photon/visuals/iconography.html#color
("rgba(12, 12, 13, 0.8)" for light theme, "rgba(249, 249, 250, 0.8)" for dark theme, or better yet:
 "var(--grey-90-a80)" when we import https://github.com/FirefoxUX/design-tokens/blob/master/photon-colors/photon-colors.css )
(I just realized this was devtools! Would you mind adding those two colours to https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/variables.css as "--grey-90-a80" and "--grey-10-a80", and then using them in your code? :)
Thanks for working on this, Brian! RE: color — I realized that part of the mockup is outdated/not exact - sorry for the confusion! We can actually use existing color variables for this. 

This is kind of complicated, but I think the old-style, 1px line-width DevTools-only icons should be the existing var(--theme-toolbar-color), while the icons that come from Photon (and are twice as bold in visual weight) should be Gray-50. I updated the mockup with these specs to show how this should look: https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens/283871189

In dark mode, the difference between the icons are less pronounced - all the icons can be Gray-40.

Here are the photon icon files:
https://design.firefox.com/icons/viewer/#more
https://design.firefox.com/icons/viewer/#close

Give Feedback should probably link to the Discourse sub-forum: https://discourse.mozilla.org/c/devtools

The closest we have to "Getting Started" is probably this MDN page - https://developer.mozilla.org/en-US/docs/Tools - but with the big ad for DevEdition and the lack of beginner-friendly introductory info, it's not great as a landing page. I'm going to talk to Chris Mills about this.
Assignee

Updated

a year ago
Depends on: 1450624
Assignee

Comment 12

a year ago
Updated try:

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

Still to do:

* Fix devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js to test labels using the localized string
* Fix failures in devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split_persist.js
* Probably a few other test failures
* Possibly factor out the link opening function from the help items patch somewhere
* Apply color changes
Assignee

Comment 13

a year ago
Posted image Regular menu (obsolete) —
Attachment #8961667 - Attachment is obsolete: true
Assignee

Comment 14

a year ago
Posted image Browser console window menu (obsolete) —
Assignee

Comment 15

a year ago
A few notes/questions I have about the current setup:

* Split console alternates between "Split console" and "Hide console" as opposed to using a checkbox.
* "Disable popup auto-hide" has a checkmark.
* Is it ok to new the OS native menu styling or should I be trying to match the mockup?
* Do we want to show the accelerator keys F1/Esc like Chrome does? (They're not in the original mockup)
* When the settings panel is active none of the tabs are highlighted. Is that ok?
(I also made the keyboard shortcuts just cycle between the remaining tabs and not visit the settings tab.)
* As per comment 12, I've yet to lighten the colors of the icons.
* I'll probably tackle conditionally hiding the iframe icon in a separate bug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Blocks: 1451592
Assignee

Comment 33

a year ago
Posted image Screenshots
Updated the screenshots to show the current behavior (on Windows).
Attachment #8964495 - Attachment is obsolete: true
Attachment #8964497 - Attachment is obsolete: true
Assignee

Comment 34

a year ago
The remaining borders look horrible. I'll see if Hiro can look into bug 1449422.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

a year ago
mozreview-review
Comment on attachment 8965176 [details]
Bug 1444301 - Add missing PropTypes to ToolboxToolbar;

https://reviewboard.mozilla.org/r/233868/#review239680

Thanks! :) Someone should consider adding prop types to `ToolboxController` as well... it's easier to follow when each level is typed.
Attachment #8965176 - Flags: review?(jryans) → review+

Comment 41

a year ago
mozreview-review
Comment on attachment 8965177 [details]
Bug 1444301 - Use a consistent order when listing the props of the ToolboxToolbar component;

https://reviewboard.mozilla.org/r/233870/#review239802

Hmm, so you have reorder things, but it's not obvious to what's more consistent now...  Are they grouped in a logical way?  Would just sorting alphabetically be easier in terms of consistency with future changes?

I have no strong opinion myself, so this is fine if it seems better to you.

::: devtools/client/framework/components/toolbox-toolbar.js:174
(Diff revision 1)
> + *                                     panel.
> + * @property {Function} selectTool - Function to select a tool in the toolbox.
> + * @property {Function} focusButton - Keep a record of the currently focused
> + *                                    button.
>   */
>  function renderOptions({optionsPanel, currentToolId, selectTool, focusedButton,

Should you reorder the args to match the comment?

::: devtools/client/framework/components/toolbox-toolbar.js:202
(Diff revision 1)
> - * @property {Function} closeToolbox - Completely close the toolbox.
>   * @property {Array} hostTypes - Array of host type objects, containing:
>   *                   @property {String} position - Position name
> - *                   @property {Function} switchHost - Function to switch the host.
> - * @property {Function} focusButton - Keep a record of the currently focused button.
> + *                   @property {Function} switchHost - Function to switch the
> + *                                                     host.
> + * @property {Boolean} areDockButtonsEnabled - They are not enabled in certain

I would suggest maybe move the description text to the next line, so you don't end up with different wrapping limits for each property.  It looks a bit chaotic as it is now...
Attachment #8965177 - Flags: review?(jryans) → review+

Comment 42

a year ago
mozreview-review
Comment on attachment 8965177 [details]
Bug 1444301 - Use a consistent order when listing the props of the ToolboxToolbar component;

https://reviewboard.mozilla.org/r/233870/#review239814

::: devtools/client/framework/components/toolbox-toolbar.js:163
(Diff revision 1)
>  /**
> - * The options button is a ToolboxTab just like in the ToolboxTabs component. However
> - * it is separate from the normal tabs, so deal with it separately here.
> + * The options button is a ToolboxTab just like in the ToolboxTabs component.
> + * However it is separate from the normal tabs, so deal with it separately here.
> + * The following props are expected.
>   *
> - * @param {Object}   optionsPanel - A single panel definition for the options panel.
> + * @property {String}   focusedButton - The id of the focused button.

I think these @property should really be renamed to @param. It seems that there are some odd files in the toolbox that still uses @property when majority of the project uses @param

Comment 43

a year ago
mozreview-review
Comment on attachment 8965178 [details]
Bug 1444301 - Move dock functions to a new meatball menu;

https://reviewboard.mozilla.org/r/233872/#review239806

Thanks, this looks reasonable overall!

::: devtools/client/framework/components/toolbox-toolbar.js:196
(Diff revision 1)
>  function renderSeparator() {
>    return div({className: "devtools-separator"});
>  }
>  
>  /**
> - * Render the dock buttons, and handle all the cases for what type of host the toolbox
> + * Render the toolbox controls buttons. The following props are expected:

Nit: control buttons

::: devtools/client/framework/components/toolbox-toolbar.js:278
(Diff revision 1)
> + *      options.
> + *   @property {Object} L10N - Localization interface.
> + *   @property {Object} toolbox - The devtools toolbox. Used by the Menu
> + *                                component to determine which document to use.
> + */
> +function showToolboxMenu(menuButton, {hostTypes, L10N, toolbox}) {

Calling this "toolbox menu" feels a bit too generic.  Could it be "toolbox overflow menu"?  Not sure of a great name...

::: devtools/client/framework/components/toolbox-toolbar.js:286
(Diff revision 1)
> +  for (const hostType of hostTypes) {
> +    menu.append(new MenuItem({
> +      id: `toolbox-menu-dock-${hostType.position}`,
> +      label: L10N.getStr(`toolbox.menu.dock.${hostType.position}.label`),
> +      click: () => {
> +        hostType.switchHost();

Could go with one line and drop the braces, if you prefer.

::: devtools/client/jar.mn:278
(Diff revision 1)
>      skin/images/firebug/twisty-closed-firebug.svg (themes/images/firebug/twisty-closed-firebug.svg)
>      skin/images/firebug/twisty-open-firebug.svg (themes/images/firebug/twisty-open-firebug.svg)
>      skin/images/firebug/arrow-down.svg (themes/images/firebug/arrow-down.svg)
>      skin/images/firebug/arrow-up.svg (themes/images/firebug/arrow-up.svg)
>      skin/images/firebug/close.svg (themes/images/firebug/close.svg)
> +    skin/images/firebug/more.svg (themes/images/firebug/more.svg)

As :gl said elsewhere, you'll need to rebase across Firebug theme removal.

::: devtools/client/themes/images/firebug/more.svg:4
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"

In case you are unaware, there are some SVG file guidelines[1], most of which can be handled by "run svgo".  If you haven't done so, might be worth a try.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines
Attachment #8965178 - Flags: review?(jryans) → review+
I'll continue reviewing code, but Victoria should also review the UX.

I triggered a macOS opt build from your try run, and here's the output:

https://queue.taskcluster.net/v1/task/d3jq7vwjRUWB7X8zG2EFfg/runs/0/artifacts/public/build/target.dmg

Victoria, could you give this a try and provide any feedback?
Flags: needinfo?(victoria)

Comment 45

a year ago
mozreview-review
Comment on attachment 8965179 [details]
Bug 1444301 - Move Options button into meatball menu;

https://reviewboard.mozilla.org/r/233874/#review239832

Great, this seems reasonable overall. :)

::: devtools/client/framework/components/toolbox-toolbar.js:273
(Diff revision 1)
> +
> +  // Settings
> +  menu.append(new MenuItem({
> +    id: "toolbox-menu-settings",
> +    label: L10N.getStr("toolbox.menu.settings.label"),
> +    // TODO: Show "F1" acceltext once MenuItem supports it.

Seems best to file a bug and include the number here.

::: devtools/client/framework/toolbox.js:1108
(Diff revision 1)
>      this.panelDefinitions = definitions.filter(definition =>
>        definition.isTargetSupported(this._target) && definition.id !== "options");
>  
> -    this.optionsDefinition = definitions.find(({id}) => id === "options");
>      // The options tool is treated slightly differently, and is in a different area.
>      this.component.setOptionsPanel(definitions.find(({id}) => id === "options"));

Can `setOptionsPanel` also be removed here?

::: devtools/client/locales/en-US/toolbox.properties:170
(Diff revision 1)
>  
> +# LOCALIZATION NOTE (toolbox.menu.settings.label): This is the label for the
> +# item in the "..." menu in the toolbox that brings up the Settings (Options)
> +# panel.
> +# The keyboard shortcut will be shown to the side of the label.
> +toolbox.menu.settings.label=Settings

Is it worth using this opportunity to choose a canoncial name that's used in both strings and code?  We have "Settings" in strings and "Options" in code, which is a bit confusing when searching code, etc.

If we like Settings, can we rename code to this as well?  (Feel free to defer to a new commit or bug...)
Attachment #8965179 - Flags: review?(jryans) → review+

Comment 46

a year ago
mozreview-review-reply
Comment on attachment 8965179 [details]
Bug 1444301 - Move Options button into meatball menu;

https://reviewboard.mozilla.org/r/233874/#review239832

> Seems best to file a bug and include the number here.

Oh, it's in the next commit... :D Sorry!

Comment 47

a year ago
mozreview-review
Comment on attachment 8965180 [details]
Bug 1444301 - Add accelerator support to MenuItem component;

https://reviewboard.mozilla.org/r/233876/#review239846

::: devtools/client/framework/components/toolbox-toolbar.js:273
(Diff revision 1)
>  
>    // Settings
>    menu.append(new MenuItem({
>      id: "toolbox-menu-settings",
>      label: L10N.getStr("toolbox.menu.settings.label"),
> -    // TODO: Show "F1" acceltext once MenuItem supports it.
> +    accelerator: L10N.getStr("toolbox.help.key"),

It's curious that we seem have bound both F1 *and* Cmd/Ctrl-Shift-O to this...
Attachment #8965180 - Flags: review?(jryans) → review+

Comment 48

a year ago
mozreview-review
Comment on attachment 8965181 [details]
Bug 1444301 - Move split console function to meatball menu;

https://reviewboard.mozilla.org/r/233878/#review239850

::: devtools/client/framework/components/toolbox-toolbar.js:264
(Diff revision 1)
> + *                                             console on / off.
>   *   @property {Object} L10N - Localization interface.
>   *   @property {Object} toolbox - The devtools toolbox. Used by the Menu
>   *                                component to determine which document to use.
>   */
> -function showToolboxMenu(menuButton, {hostTypes, selectTool, L10N, toolbox}) {
> +function showToolboxMenu(

Haha, I've suggested this "Rust like" args style in other reviews...  Glad to see you're trying it here. :)
Attachment #8965181 - Flags: review?(jryans) → review+

Comment 49

a year ago
mozreview-review
Comment on attachment 8965182 [details]
Bug 1444301 - Move disable pop-up autohide feature to toolbox menu;

https://reviewboard.mozilla.org/r/233880/#review239852

::: devtools/client/framework/components/toolbox-controller.js:32
(Diff revision 1)
>        panelDefinitions: [],
>        hostTypes: [],
>        areDockOptionsEnabled: true,
>        canCloseToolbox: true,
>        isSplitConsoleActive: false,
> +      disableAutohide: undefined,

Nit: "auto" and "hide" feel like separate words, so maybe `disableAutoHide` (capital H), etc.?

::: devtools/client/framework/components/toolbox-toolbar.js:327
(Diff revision 1)
>        click: toggleSplitConsole,
>      }));
>    }
>  
> +  // Disable pop-up autohide
> +  if (typeof disableAutohide !== "undefined") {

Might be nice to repeat here the detail about what `undefined` means in this case.

::: devtools/client/framework/toolbox.js:1110
(Diff revision 1)
>      // The options tool is treated slightly differently, and is in a different area.
>      this.component.setOptionsPanel(definitions.find(({id}) => id === "options"));
> +
> +    // Do async lookup of disable pop-up auto-hide state.
> +    if (this.disableAutohideAvailable) {
> +      this._isDisableAutohideEnabled().then(disable => {

How about `let disable = await ...`?  Seems okay to mark the function async for this.
Attachment #8965182 - Flags: review?(jryans) → review+

Comment 50

a year ago
mozreview-review
Comment on attachment 8965183 [details]
Bug 1444301 - Add help items to menu;

https://reviewboard.mozilla.org/r/233882/#review239858

::: devtools/client/framework/components/toolbox-toolbar.js:356
(Diff revision 1)
> +  }
> +
> +  // REVIEWER: Should this logic be factored out somewhere common? Where?
> +  // Should we prefer the implementation in dom-panel.js::openLink?
> +  // https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/devtools/client/dom/dom-panel.js#181-186
> +  const openLink = url => {

Seems like a new module in `devtools/client/shared` might make sense for this.  Maybe `link` module?

If you prefer, you could also remove the dependency on the `toolbox` by using an approach like MDNLink[1].

[1]: https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/client/netmonitor/src/components/MdnLink.js#37

::: devtools/client/framework/components/toolbox-toolbar.js:379
(Diff revision 1)
> +
> +  // Getting started
> +  menu.append(new MenuItem({
> +    id: "toolbox-menu-gettingstarted",
> +    label: L10N.getStr("toolbox.menu.gettingStarted.label"),
> +    click: evt => {

`evt` unused?

::: devtools/client/locales/en-US/toolbox.properties:179
(Diff revision 1)
>  # item in the "..." menu in the toolbox that brings up the Settings (Options)
>  # panel.
>  # The keyboard shortcut will be shown to the side of the label.
>  toolbox.menu.settings.label=Settings
>  
> +# LOCALIZATION NOTE (toolbox.menu.gettingStarted.label): This is the label for the

Some words appear to be missing in this note and the next.
Attachment #8965183 - Flags: review?(jryans) → review+

Comment 51

a year ago
mozreview-review
Comment on attachment 8965184 [details]
Bug 1444301 - Introduce lighter shading for photon-style icons in the toolbox;

https://reviewboard.mozilla.org/r/233884/#review239864
Attachment #8965184 - Flags: review?(jryans) → review+

Comment 52

a year ago
mozreview-review
Comment on attachment 8965185 [details]
Bug 1444301 - Update close icon to use Photon icon;

https://reviewboard.mozilla.org/r/233886/#review239866
Attachment #8965185 - Flags: review?(jryans) → review+

Comment 53

a year ago
mozreview-review
Comment on attachment 8965186 [details]
Bug 1444301 - Move the separator before the RDM icon;

https://reviewboard.mozilla.org/r/233888/#review239872

Marking r- for now, as it seems to miss a detail of the mockup.

::: devtools/client/framework/components/toolbox-toolbar.js:176
(Diff revision 1)
>            onKeyDown(event);
>          }
>        });
> -    }),
> -    isStart ? div({className: "devtools-separator"}) : null
> +    });
> +
> +  // Add the appropriate separator, if needed.

Looking at the mockup, it seems that if there's nothing to the left of the separator, it shouldn't be drawn.

It seems like this patch will always draw the separator if RDM were the only button, for example.
Attachment #8965186 - Flags: review?(jryans) → review-

Comment 54

a year ago
mozreview-review
Comment on attachment 8965187 [details]
Bug 1444301 - Adjust spacing of iframe button;

https://reviewboard.mozilla.org/r/233890/#review239874
Attachment #8965187 - Flags: review?(jryans) → review+

Comment 55

a year ago
mozreview-review
Comment on attachment 8965188 [details]
Bug 1444301 - Tweak RDM button;

https://reviewboard.mozilla.org/r/233892/#review239876
Attachment #8965188 - Flags: review?(jryans) → review+

Comment 56

a year ago
mozreview-review
Comment on attachment 8965189 [details]
Bug 1444301 - Make the meatball button a little less meaty;

https://reviewboard.mozilla.org/r/233894/#review239878
Attachment #8965189 - Flags: review?(jryans) → review+

Comment 57

a year ago
mozreview-review
Comment on attachment 8965190 [details]
Bug 1444301 - Fix badly rounded corner on frames graphic;

https://reviewboard.mozilla.org/r/233896/#review239880
Attachment #8965190 - Flags: review?(jryans) → review+

Comment 58

a year ago
mozreview-review
Comment on attachment 8965191 [details]
Bug 1444301 - Add spacing around separator at end of toolbox;

https://reviewboard.mozilla.org/r/233898/#review239882
Attachment #8965191 - Flags: review?(jryans) → review+
Assignee

Comment 59

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #53)
> Comment on attachment 8965186 [details]
> Bug 1444301 - Move the separator before the RDM icon;
> ...
> // Add the appropriate separator, if needed.
> 
> Looking at the mockup, it seems that if there's nothing to the left of the
> separator, it shouldn't be drawn.
> 
> It seems like this patch will always draw the separator if RDM were the only
> button, for example.

Right, that's bug 1451592 however. In this patch series we will still always draw the iframe button so there will always be something left of the separator.
(In reply to Brian Birtles (:birtles) from comment #59)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #53)
> > Comment on attachment 8965186 [details]
> > Bug 1444301 - Move the separator before the RDM icon;
> > ...
> > // Add the appropriate separator, if needed.
> > 
> > Looking at the mockup, it seems that if there's nothing to the left of the
> > separator, it shouldn't be drawn.
> > 
> > It seems like this patch will always draw the separator if RDM were the only
> > button, for example.
> 
> Right, that's bug 1451592 however. In this patch series we will still always
> draw the iframe button so there will always be something left of the
> separator.

Ah I see, fair enough!  I'll mark r+ here then.

Comment 61

a year ago
mozreview-review
Comment on attachment 8965186 [details]
Bug 1444301 - Move the separator before the RDM icon;

https://reviewboard.mozilla.org/r/233888/#review239900
Attachment #8965186 - Flags: review- → review+
Hi Brian, I'm looking at the try build now (thanks Ryan!) and this is looking great!! 

I really like the size of the meatball icon. Originally I was worried that a small meatball icon would clash with the regular-sized meatball icon in the URL bar, but now that I'm seeing this it seems totally fine given that all the DevTools icons are small. In fact, I think it would look better now if we change the color to be darker using the photon Gray 90 + .8 opacity color - sorry for the switching back and forth on this. It's exciting to see this in action!

I think the split/hide console behavior seems fine, though maybe the default state needs to be reworded to "Show split console" to make it more of a verb? I'll defer to you on this.

One tiny spacing tweak - I think the close button needs about 3px right margin.
Flags: needinfo?(victoria)
Assignee

Comment 63

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> Comment on attachment 8965176 [details]
> Bug 1444301 - Add missing PropTypes to ToolboxToolbar;
> 
> https://reviewboard.mozilla.org/r/233868/#review239680
> 
> Thanks! :) Someone should consider adding prop types to `ToolboxController`
> as well... it's easier to follow when each level is typed.

Definitely. I'll do it next time I'm touching this file (I want to try and land these patches soon because I'm travelling next week so I won't do it just now.)

(And if some day we could switch to Flow, or, better still, TypeScript, that would be amazing!)
Assignee

Comment 64

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #41)
> Comment on attachment 8965177 [details]
> Bug 1444301 - Use a consistent order when listing the props of the
> ToolboxToolbar component;
> 
> https://reviewboard.mozilla.org/r/233870/#review239802
> 
> Hmm, so you have reorder things, but it's not obvious to what's more
> consistent now...  Are they grouped in a logical way?  Would just sorting
> alphabetically be easier in terms of consistency with future changes?
> 
> I have no strong opinion myself, so this is fine if it seems better to you.

Yeah, I was mostly just aiming for consistency without changing things too much but I did try to apply some logical grouping where I did change the order. e.g.:

* optionsPanel goes with panelDefinitions since they're the same sort of thing
* similar bools are grouped at the end

...actually that's about all that changed.

> ::: devtools/client/framework/components/toolbox-toolbar.js:174
> (Diff revision 1)
> > + *                                     panel.
> > + * @property {Function} selectTool - Function to select a tool in the toolbox.
> > + * @property {Function} focusButton - Keep a record of the currently focused
> > + *                                    button.
> >   */
> >  function renderOptions({optionsPanel, currentToolId, selectTool, focusedButton,
> 
> Should you reorder the args to match the comment?

Absolutely. That was an oversight.

> ::: devtools/client/framework/components/toolbox-toolbar.js:202
> (Diff revision 1)
> > - * @property {Function} closeToolbox - Completely close the toolbox.
> >   * @property {Array} hostTypes - Array of host type objects, containing:
> >   *                   @property {String} position - Position name
> > - *                   @property {Function} switchHost - Function to switch the host.
> > - * @property {Function} focusButton - Keep a record of the currently focused button.
> > + *                   @property {Function} switchHost - Function to switch the
> > + *                                                     host.
> > + * @property {Boolean} areDockButtonsEnabled - They are not enabled in certain
> 
> I would suggest maybe move the description text to the next line, so you
> don't end up with different wrapping limits for each property.  It looks a
> bit chaotic as it is now...

Fixed. That looks a lot better now.
Assignee

Comment 65

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> Comment on attachment 8965178 [details]
> ::: devtools/client/framework/components/toolbox-toolbar.js:278
> (Diff revision 1)
> > + *      options.
> > + *   @property {Object} L10N - Localization interface.
> > + *   @property {Object} toolbox - The devtools toolbox. Used by the Menu
> > + *                                component to determine which document to use.
> > + */
> > +function showToolboxMenu(menuButton, {hostTypes, L10N, toolbox}) {
> 
> Calling this "toolbox menu" feels a bit too generic.  Could it be "toolbox
> overflow menu"?  Not sure of a great name...

Yeah, the trouble is we'll have another overflow menu in bug 1442531.

I originally called it the meatball menu. Perhaps I should switch back to that.

> ::: devtools/client/framework/components/toolbox-toolbar.js:286
> (Diff revision 1)
> > +  for (const hostType of hostTypes) {
> > +    menu.append(new MenuItem({
> > +      id: `toolbox-menu-dock-${hostType.position}`,
> > +      label: L10N.getStr(`toolbox.menu.dock.${hostType.position}.label`),
> > +      click: () => {
> > +        hostType.switchHost();
> 
> Could go with one line and drop the braces, if you prefer.

Fixed.

> ::: devtools/client/jar.mn:278
> (Diff revision 1)
> >      skin/images/firebug/twisty-closed-firebug.svg (themes/images/firebug/twisty-closed-firebug.svg)
> >      skin/images/firebug/twisty-open-firebug.svg (themes/images/firebug/twisty-open-firebug.svg)
> >      skin/images/firebug/arrow-down.svg (themes/images/firebug/arrow-down.svg)
> >      skin/images/firebug/arrow-up.svg (themes/images/firebug/arrow-up.svg)
> >      skin/images/firebug/close.svg (themes/images/firebug/close.svg)
> > +    skin/images/firebug/more.svg (themes/images/firebug/more.svg)
> 
> As :gl said elsewhere, you'll need to rebase across Firebug theme removal.

Fixed.

> ::: devtools/client/themes/images/firebug/more.svg:4
> (Diff revision 1)
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"
> 
> In case you are unaware, there are some SVG file guidelines[1], most of
> which can be handled by "run svgo".  If you haven't done so, might be worth
> a try.
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines

Ah, I hadn't seen that so that's good to know! This particular file is taken straight from the Photon library which, I believe, has already been through svgo.

I double-checked it and simplified it a little later in this patch series--fortunately SVG happens to be my specialty!
Assignee

Comment 66

a year ago
(In reply to Brian Birtles (:birtles) from comment #65)
> > ::: devtools/client/themes/images/firebug/more.svg:4
> > (Diff revision 1)
> > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"
> > 
> > In case you are unaware, there are some SVG file guidelines[1], most of
> > which can be handled by "run svgo".  If you haven't done so, might be worth
> > a try.
> > 
> > [1]:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> > SVG_Guidelines
> 
> Ah, I hadn't seen that so that's good to know! This particular file is taken
> straight from the Photon library which, I believe, has already been through
> svgo.
> 
> I double-checked it and simplified it a little later in this patch
> series--fortunately SVG happens to be my specialty!

Oh, I just noticed this comment was actually about the Firebug icon. Now that the firebug theme has been removed, I've dropped this icon from the patch.
Assignee

Comment 67

a year ago
(In reply to Brian Birtles (:birtles) from comment #65)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> > Comment on attachment 8965178 [details]
> > > +function showToolboxMenu(menuButton, {hostTypes, L10N, toolbox}) {
> > 
> > Calling this "toolbox menu" feels a bit too generic.  Could it be "toolbox
> > overflow menu"?  Not sure of a great name...
> 
> Yeah, the trouble is we'll have another overflow menu in bug 1442531.
> 
> I originally called it the meatball menu. Perhaps I should switch back to
> that.

Probably tabbar-menu or toolbar-menu are more accurate but even those could potentially be confused with the tabs overflow menu we're looking to introduce in bug 1442531.

Let me try toolbox-meatball-menu for the object ID and "meatball menu" elsewhere.
Assignee

Comment 68

a year ago
mozreview-review-reply
Comment on attachment 8965179 [details]
Bug 1444301 - Move Options button into meatball menu;

https://reviewboard.mozilla.org/r/233874/#review239832

> Can `setOptionsPanel` also be removed here?

It turns out it can. Originally I was concerned we'd fail to set the focusButton correct and break keyboard navigation but it seems to work fine (perhaps better) without this.

That said, keyboard navigation is badly broken we we might need to reinstate this when we come to fix keyboard navigation properly.
Assignee

Comment 69

a year ago
mozreview-review-reply
Comment on attachment 8965179 [details]
Bug 1444301 - Move Options button into meatball menu;

https://reviewboard.mozilla.org/r/233874/#review239832

> Is it worth using this opportunity to choose a canoncial name that's used in both strings and code?  We have "Settings" in strings and "Options" in code, which is a bit confusing when searching code, etc.
> 
> If we like Settings, can we rename code to this as well?  (Feel free to defer to a new commit or bug...)

Filed bug 1452007 for this.
Assignee

Comment 70

a year ago
mozreview-review-reply
Comment on attachment 8965180 [details]
Bug 1444301 - Add accelerator support to MenuItem component;

https://reviewboard.mozilla.org/r/233876/#review239846

> It's curious that we seem have bound both F1 *and* Cmd/Ctrl-Shift-O to this...

Yeah, I don't know where that came from. I've preferred F1 here since that's what Chrome uses and since Cmd/Ctrl-Shift-O is more likely to vary with locale (and its probably better to teach users the shortcuts they can rely on more often).
Assignee

Comment 71

a year ago
mozreview-review-reply
Comment on attachment 8965181 [details]
Bug 1444301 - Move split console function to meatball menu;

https://reviewboard.mozilla.org/r/233878/#review239850

> Haha, I've suggested this "Rust like" args style in other reviews...  Glad to see you're trying it here. :)

Yeah, I've been using prettier to help me with some cases of indenting where I'm having trouble making it look readable.
Assignee

Comment 72

a year ago
mozreview-review-reply
Comment on attachment 8965182 [details]
Bug 1444301 - Move disable pop-up autohide feature to toolbox menu;

https://reviewboard.mozilla.org/r/233880/#review239852

> How about `let disable = await ...`?  Seems okay to mark the function async for this.

Did you mean we should also then await _buildTabs?

I was assuming we shouldn't block on that so I've just made this use await but not block on _buildTabs. That could be a bit odd if anyone adds anything after the setDisableAutohide however.
Assignee

Comment 73

a year ago
mozreview-review-reply
Comment on attachment 8965182 [details]
Bug 1444301 - Move disable pop-up autohide feature to toolbox menu;

https://reviewboard.mozilla.org/r/233880/#review239852

> Nit: "auto" and "hide" feel like separate words, so maybe `disableAutoHide` (capital H), etc.?

Yes, I agree that's better. However, we seemed to use Autohide pretty consistently in this part of the code so I decided to just stick with the local convention. Perhaps next time we touch this feature we should do a global search and replace for this.
Assignee

Comment 74

a year ago
mozreview-review-reply
Comment on attachment 8965183 [details]
Bug 1444301 - Add help items to menu;

https://reviewboard.mozilla.org/r/233882/#review239858

> Seems like a new module in `devtools/client/shared` might make sense for this.  Maybe `link` module?
> 
> If you prefer, you could also remove the dependency on the `toolbox` by using an approach like MDNLink[1].
> 
> [1]: https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/client/netmonitor/src/components/MdnLink.js#37

> If you prefer, you could also remove the dependency on the toolbox by using an approach like MDNLink.

I tried this but couldn't seem to get it to work.

Specifically, something like:

  const openLink = url => {
    const win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
    if (!win || !win.openUILinkIn !== "function") {
      return;
    }

    win.openUILinkIn(url, "tab");
  };

(And the necessary require for gDevTools)

However, when I trigger the function nothing appears to happen. Unfortunately I don't really have time to debug it right now so I might just add the module with the current dependency on toolbox.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 91

a year ago
I've rebased and addressed all the review / UX feedback and now I'm just waiting on try:

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

I'm not sure when I'll be able to land, however, since I'll be traveling from early tomorrow with a less capable laptop so if there are any problems to fix it might take me some time to get to them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 106

a year ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b46dad596df4
Add missing PropTypes to ToolboxToolbar; r=jryans
https://hg.mozilla.org/integration/autoland/rev/b34330320b8c
Use a consistent order when listing the props of the ToolboxToolbar component; r=jryans
https://hg.mozilla.org/integration/autoland/rev/0f07755ee54f
Move dock functions to a new meatball menu; r=jryans
https://hg.mozilla.org/integration/autoland/rev/a6afc78b217e
Move Options button into meatball menu; r=jryans
https://hg.mozilla.org/integration/autoland/rev/220140d1c28e
Add accelerator support to MenuItem component; r=jryans
https://hg.mozilla.org/integration/autoland/rev/f00ccb9d81b7
Move split console function to meatball menu; r=jryans
https://hg.mozilla.org/integration/autoland/rev/9140b553db46
Move disable pop-up autohide feature to toolbox menu; r=jryans
https://hg.mozilla.org/integration/autoland/rev/4abc98b60291
Add help items to menu; r=jryans
https://hg.mozilla.org/integration/autoland/rev/154ca4e5a0d2
Introduce lighter shading for photon-style icons in the toolbox; r=jryans
https://hg.mozilla.org/integration/autoland/rev/5d0acb7cf921
Update close icon to use Photon icon; r=jryans
https://hg.mozilla.org/integration/autoland/rev/abf995f8c9b3
Move the separator before the RDM icon; r=jryans
https://hg.mozilla.org/integration/autoland/rev/2b22e8b9c851
Adjust spacing of iframe button; r=jryans
https://hg.mozilla.org/integration/autoland/rev/dfde9a8145ce
Tweak RDM button; r=jryans
https://hg.mozilla.org/integration/autoland/rev/396a9455b0bd
Make the meatball button a little less meaty; r=jryans
https://hg.mozilla.org/integration/autoland/rev/305860d08511
Fix badly rounded corner on frames graphic; r=jryans
https://hg.mozilla.org/integration/autoland/rev/c5e2ad71fce5
Add spacing around separator at end of toolbox; r=jryans

Comment 107

a year ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7855107fe8e6
Fix lint error in toolbox-toolbar.js on CLOSED TREE; r=bustage

Comment 108

a year ago
mozreview-review
Comment on attachment 8965183 [details]
Bug 1444301 - Add help items to menu;

https://reviewboard.mozilla.org/r/233882/#review240368

::: devtools/client/locales/en-US/toolbox.properties:182
(Diff revision 3)
>  # The keyboard shortcut will be shown to the side of the label.
>  toolbox.meatballMenu.settings.label=Settings
>  
> +# LOCALIZATION NOTE (toolbox.meatballMenu.gettingStarted.label): This is the
> +# label for the Getting Started menu item.
> +toolbox.meatballMenu.gettingStarted.label=Getting started

As the page isn't about Getting Started but just documentation the naming does not make full sense. Reviewing other tools, "Documentation" and "Release Notes" are typical Help entries.

::: devtools/client/locales/en-US/toolbox.properties:186
(Diff revision 3)
> +# label for the Getting Started menu item.
> +toolbox.meatballMenu.gettingStarted.label=Getting started
> +
> +# LOCALIZATION NOTE (toolbox.meatballMenu.giveFeedback.label): This is the label
> +# for the Give feedback menu item.
> +toolbox.meatballMenu.giveFeedback.label=Give feedback

"Community" might be more appropiate here, as all other feedback forms in Firefox lead to survey forms and are not about starting discussions.
Attachment #8965183 - Flags: review-
Since we've already landed patches here, I filed bug 1452290 to capture Harald's feedback.
I have reproduced this bug with Nightly 60.0a1 (2018-03-08) on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Bulid ID   -  20180415220108
User Agent -  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [bugday-20180411]
Assignee

Updated

a year ago
Depends on: 1455462

Updated

a year ago
Depends on: 1455589
Depends on: 1454123
I have reproduced this issue using Firefox  60.0a1 (2018.03.08) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b9 on Win 8.1 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Assignee

Updated

a year ago
Depends on: 1467431

Updated

11 months ago
Product: Firefox → DevTools
Assignee

Updated

11 months ago
No longer depends on: 1467431
Marking dev-doc-needed as this bug changed the toolbox button appearance, so some screenshots are now outdated.

For example, in bug 1468896, a user was confused by the page https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Debugging#Debugging_popups because the popups control is now in the meatball menu instead of a standalone button.
Keywords: dev-doc-needed
Assignee

Comment 114

11 months ago
I believe somewhere I suggested holding off updating the screenshots until bug 1461522 lands (and perhaps also the follow up to re-arrange the docking icons) since they will probably need to be redone after that anyway.

Comment 115

11 months ago
As a first step to updating images, I have added a closeup of the right-hand side of the developers tools and a table explaining each of the icons. They seem much more self-explanatory, but I've added the descriptions to the overview.

New information added here: https://developer.mozilla.org/en-US/docs/Tools Under the heading "Core Tools"
Flags: needinfo?(bbirtles)
Irene, where are the changes again? https://developer.mozilla.org/en-US/docs/Tools#The_Core_Tools was updated last Jun 28.
Flags: needinfo?(ismith)

Comment 117

11 months ago
Harald, Darn! I forgot that our changes are temporarily not showing up. I will let you know when the edits will be visible.
Flags: needinfo?(ismith)
Assignee

Comment 118

11 months ago
Looks good Irene!

A few notes:

* The screenshot button is disabled by default so perhaps we shouldn't show it here?
* If the page being viewed has iframes (e.g. yahoo.com) there will be an iframe selector button that is probably worth showing
* The icons for the docking will change in bug 1474224 and then again in bug 1474223 so we'll need to update the screenshot again then. Sorry!
Flags: needinfo?(bbirtles)

Comment 119

11 months ago
Brian,

Thanks for the feedback! Can you tell me which toolbox buttons are available by default? I've been using the tools for so long I don't remember which ones are on when you make a new install.

I added the iframe button but I think all possible buttons should be included here as well as information about which are on by default and which are not.

Also, where do the screenshot go if you don't have "Screenshot to clipboard" selected? I think all of that information should go into the table.

We can remake the button images when the UI is finalized.
Flags: needinfo?(bbirtles)
Assignee

Comment 120

11 months ago
Hi Irene,

I'm afraid I'm not so familiar with DevTools. I *think* only the following are on by default:

* Pick an element from the page
* Select an iframe as the currently targeted document
* Responsive Design Mode

However, it would be best to try starting Firefox with a new profile to confirm. (No need to do a new install, just close Firefox and start it with the -ProfileManager flag to create a new profile.)

I believe the screenshots are downloaded if you don't select the Screenshot to Clipboard option.
Flags: needinfo?(bbirtles)
Remainder of the requests listed here, to be done in a future task.
https://github.com/mdn/sprints/issues/312
You need to log in before you can comment on or make changes to this bug.