1455443 - Improve usability of docking options in new meatball menu

Status: VERIFIED FIXED

(DevTools :: General, enhancement)

Description

[Victoria Wang [:victoria]]

Attachment: separatewindow.svg

As shown in this updated mockup:
https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens

- Show all three docking options in their own menu section with icons, with check mark on currently selected.
- The icons already exist in the project, but there's an updated separate window icon which I will attach to this bug.
- The menu items for Split Console and Settings can be moved into their own menu section

Comment 1

[Victoria Wang [:victoria]]

Attachment: dock-undock-expanded.svg

Oops, something was wrong with that previous icon. Attaching a new one

Comment 2

[Brian Birtles (:birtles)]

Thanks Victoria! I started working on this yesterday. Currently fiddling with showing both checkmark and icon (normally we do one or the other).

Also not sure about the light coloring of the icons if we continue using OS native menu styling.

Comment 3

[Victoria Wang [:victoria]]

Ah great, I just didn't want it to get lost :). Re: icon colors, makes sense to make them black or whatever the native color is. Can the svgs be filled to black in the code, or should I export new black icons?

Comment 4

[Brian Birtles (:birtles)]

(In reply to Victoria Wang [:victoria] from comment #3)
> Can the svgs be filled to black in the code, or should I export new black icons?

Yes, they can be filled in the code. I am filling them with the native menu text color (e.g. white for me on Ubuntu).

Comment 5

[Brian Birtles (:birtles)]

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

Comment 6

[Brian Birtles (:birtles)]

Attachment: Ubuntu screenshot

Comment 7

[Brian Birtles (:birtles)]

Updated attempt:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea8a8a4f9ac3914b25613d7dbeb6fd03a917e3f

What I discovered is that if we try to re-use the xul:image from iconic menu items we hit a whole raft of platform-specific issues. In particular, on Windows we need to make the image visible in the first place, then we discover that it uses an entirely different mapping of -moz-appearance values to Linux, and some of those -moz-appearance values trigger hard-coded padding values that we can't override from CSS.

After trying this for a while I gave up and switched to using a background-image which avoids all platform-specific code and dependencies on effectively reversing rules in other (platform-specific) stylesheets.

For posterity, below is the first parts of the platform-specific CSS I started writing:

> /*
>  * Normally we don't use *both* a checkbox and an icon but in this menu, we do
>  * so we need to fix each platform's behavior to allow both to be indented
>  * correctly.
>  *
>  * The logic below is intended to mirror the logic in browser/themes/moz.build.
>  */
> 
> %ifdef MOZ_WIDGET_COCOA
> /* TODO */
> %endif
> 
> %ifdef MOZ_WIDGET_GTK
> /*
>  * On Linux we want to match the margin-inline-start: 21px that is added to
>  * .menu-text when we _don't_ have a checkbox / radiobox.
>  */
> #toolbox-meatball-menu .menuitem-iconic[type="checkbox"] > .menu-iconic-left {
>   /* This is (18 + the size of end-padding on .menu-iconic-left)px */
>   padding-inline-start: 21px;
> }
> %endif
> 
> %ifndef MOZ_WIDGET_COCOA
> %ifndef MOZ_WIDGET_GTK
> #toolbox-meatball-menu .menuitem-iconic[type="checkbox"] > .menu-iconic-left {
>   /* This doesn't actually work since we'll end up using the padding from
>    * nsNativeThemeWin::GetWidgetPadding as triggered by -moz-appearance: menucheckbox;
>    * which is a Windows-specific rule... */
>   padding-inline-start: 1.45em !important;
> }
> 
> /* On Windows, .menu-icon-icon is set to display:none inside for checkbox/radio
>  * menu items to avoid alignment issues with (see bug 411064). We don't have
>  * that problem here so make sure to show the icon. */
> #toolbox-meatball-menu > .menu-iconic-left > .menu-iconic-icon {
>   display: -moz-box;
> }
> %endif
> %endif
> 
> #toolbox-meatball-menu-dock-bottom {
>   list-style-image: var(--dock-bottom-image);
> }
> #toolbox-meatball-menu-dock-side:-moz-locale-dir(ltr) {
>   list-style-image: var(--dock-side-right-image);
> }
> #toolbox-meatball-menu-dock-side:-moz-locale-dir(rtl) {
>   list-style-image: var(--dock-side-left-image);
> }
> #toolbox-meatball-menu-dock-window {
>   list-style-image: var(--dock-undock-image);
> }

Comment 8

[Brian Birtles (:birtles)]

Attachment: Windows screenshot

Comment 9

[Brian Birtles (:birtles)]

Attachment: Windows RTL

Comment 10

[Brian Birtles (:birtles)]

Looks like there is still some platform-specific styling going on here.

On OSX I get extra padding added. It's Friday afternoon here so I'm going to have to pick this up next week but I suspect I'll need to either:

1) Drop the menu-iconic class and introduce a faux-iconic (inline-iconic?) that just does the background styles and skips all the extra XBL baggage that comes with menu-iconic.
2) Add OSX-specific styling with %ifdef MOZ_WIDGET_COCOA to drop the (presumably) doubled padding.

Also, the other thing I need to fix here is that when the menu is first displayed the icons are missing. After hovering the menu they get read in an show up for the rest of the session. We need to preload those assets somehow. I'm not sure how we normally do that.

Comment 11

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #10)
> Looks like there is still some platform-specific styling going on here.
> 
> On OSX I get extra padding added. It's Friday afternoon here so I'm going to
> have to pick this up next week but I suspect I'll need to either:
> 
> 1) Drop the menu-iconic class and introduce a faux-iconic (inline-iconic?)
> that just does the background styles and skips all the extra XBL baggage
> that comes with menu-iconic.
> 2) Add OSX-specific styling with %ifdef MOZ_WIDGET_COCOA to drop the
> (presumably) doubled padding.

It turns out it's just that Mac sets the menu-left item to a fixed width of 16px so we should be able to override that without any platform-specific handling.

Regarding adding inline-iconic, it turns out that doesn't really help since once we introduce type="checked" we get menu-iconic anyway.

Comment 12

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #10)
> Also, the other thing I need to fix here is that when the menu is first
> displayed the icons are missing. After hovering the menu they get read in an
> show up for the rest of the session. We need to preload those assets
> somehow. I'm not sure how we normally do that.

I'm a bit stuck about what forum to ask this in so I'm just CC'ing a few people who know front-end stuff.

I've added a new menu to DevTools with SVG icons referenced by background-image. The problem is, when the menu is first opened the icons don't appear until you hover the menu. Presumably we need to preload them. I could try to reference them as offscreen background images from the button that triggers the menu. I see we do that elsehwere[1]. Or perhaps attach them to some other sprite sheet.[2] But is there some more general approach we normally use for this?

I tried devtools slack but didn't get any response. I probably should try a more Firefox FE channel of some sort--where do Firefox front end people hang out?

[1] e.g. https://reviewboard.mozilla.org/r/177990/diff/14#index_header
[2] Bug 1191437

Comment 13

[:Gijs (he/him)]

(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > Also, the other thing I need to fix here is that when the menu is first
> > displayed the icons are missing. After hovering the menu they get read in an
> > show up for the rest of the session. We need to preload those assets
> > somehow. I'm not sure how we normally do that.
> 
> I'm a bit stuck about what forum to ask this in so I'm just CC'ing a few
> people who know front-end stuff.
> 
> I've added a new menu to DevTools with SVG icons referenced by
> background-image. The problem is, when the menu is first opened the icons
> don't appear until you hover the menu. Presumably we need to preload them. I
> could try to reference them as offscreen background images from the button
> that triggers the menu. I see we do that elsehwere[1]. Or perhaps attach
> them to some other sprite sheet.[2] But is there some more general approach
> we normally use for this?

If the items only appear on hover then I'm pretty sure preloading isn't the issue. Something else is going wrong. If you check with the browser toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox - there's a button in there to not make menus/panels disappear as soon as focus changes), do the elements on which you're setting the background images have height and width before said images appear? Is there nothing overriding the style? Do you have webrender turned on, and if so do you see the same thing with it turned off?

> I tried devtools slack but didn't get any response. I probably should try a
> more Firefox FE channel of some sort--where do Firefox front end people hang
> out?

#devtools and #fx-team on IRC.


FWIW, platform convention for checked items differ and on some platforms the convention is that icons aren't shown (ie you either get an icon, or you can have a checked/non-checked item, but not both). I'm not convinced that these items are so special that they need to not abide with those platform conventions, in which case you could just use the native support stuff and should basically be fine without much other CSS - but wouldn't get (any) icons on some platforms.

Comment 14

[Brian Birtles (:birtles)]

Thank you!

(In reply to :Gijs (he/him) from comment #13)
> If the items only appear on hover then I'm pretty sure preloading isn't the
> issue. Something else is going wrong.

Right, that was my hunch too, but after the menu has been displayed once the icons show without a hitch from that point on. It's like the SVG graphic is not ready for the first paint and nothing triggers invalidation until the mouse is actually over the menu window.

> If you check with the browser toolbox
> (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox - there's a
> button in there to not make menus/panels disappear as soon as focus
> changes), do the elements on which you're setting the background images have
> height and width before said images appear?

Yes.

> Is there nothing overriding the style?

No.

> Do you have webrender turned on

No.

> > I tried devtools slack but didn't get any response. I probably should try a
> > more Firefox FE channel of some sort--where do Firefox front end people hang
> > out?
> 
> #devtools and #fx-team on IRC.

Thank you!

> FWIW, platform convention for checked items differ and on some platforms the
> convention is that icons aren't shown (ie you either get an icon, or you can
> have a checked/non-checked item, but not both). I'm not convinced that these
> items are so special that they need to not abide with those platform
> conventions, in which case you could just use the native support stuff and
> should basically be fine without much other CSS - but wouldn't get (any)
> icons on some platforms.

Right. Let me follow up with Victoria about this. Thanks again.

Comment 15

[Brian Birtles (:birtles)]

Victoria, do we need both checkmarks and icons? Gijs points out in comment 12 that on some platforms (most platforms as far as I can tell) don't generally show both a checkmark and icon for a menu item.

Would it make sense, for example, to show the icons and highlight the selected one with the highlight color?

Comment 16

[Victoria Wang [:victoria]]

It seems native enough for MacOS - it's in three places in Settings > General (e.g. Default Web Browser), and I also see it in the print dialog (printer disconnection icon). The pattern of a menu item with both check mark and icon seems pretty straightforward to me (I see the icon as part of the name of the menu item), and it would improve usability of the menu, so I would suggest that it still seems worth doing this in all the platforms, as long as it's not getting too hairy to implement. The highlight color seems less native to me (at least for MacOS) and would be a bit worse for accessibility.

Comment 17

[:Gijs (he/him)]

(In reply to Victoria Wang [:victoria] from comment #16)
> It seems native enough for MacOS

Sure, but not on Windows or Linux.

> The pattern of a menu item with both check
> mark and icon seems pretty straightforward to me (I see the icon as part of
> the name of the menu item), and it would improve usability of the menu, so I
> would suggest that it still seems worth doing this in all the platforms, as
> long as it's not getting too hairy to implement. The highlight color seems
> less native to me (at least for MacOS) and would be a bit worse for
> accessibility.

The "gutter" that on Windows (and apparently Linux) shares check state / icon state is native there, and from the comments so far, implementing checkmark + icon on Windows/Linux seems pretty hairy to me.

I can't think of anything else in Firefox that shows checkmarks as well as icons in a native menu on Windows / Linux. The only thing that breaks this rule is the sidebar dropdown, which is (a) new and (b) not a native menu (it's a panel instead of a menu).

Comment 18

[Brian Birtles (:birtles)]

(In reply to :Gijs (he/him) from comment #17)
> The "gutter" that on Windows (and apparently Linux) shares check state /
> icon state is native there, and from the comments so far, implementing
> checkmark + icon on Windows/Linux seems pretty hairy to me.

I'm not yet sure if the issues I'm seeing are actually a result of the extra work needed to support both checkmark + icon. I'm starting to wonder if it's related to the way we initialize the menu in devtools/client/framework/menu.js (and perhaps we've never noticed because so far I can't find anywhere else in devtools we use icons in popup menus?).

Comment 19

[Victoria Wang [:victoria]]

I checked this out on Windows 7 and I can see why this might be tricky. The gutter is outlined and one width only, so when there's an icon in there, checked state is shown as bolded text or a bonus tiny green check icon superimposed on the other icon.

In any case, if it seems like the difficulty of making this UI happen might block this issue from landing in 61 (or get in the way of other more important work), I think leaving the icons out for now and using just the checkboxes would be fine.

For the future, however, I think it could be worth implementing the custom UI based on the sidebar dropdown panel. It would match the main Firefox meatball menu which uses one of these custom panels too.

Comment 20

[Brian Birtles (:birtles)]

(In reply to Victoria Wang [:victoria] from comment #19)
> For the future, however, I think it could be worth implementing the custom
> UI based on the sidebar dropdown panel. It would match the main Firefox
> meatball menu which uses one of these custom panels too.

Doing something like that sidebar dropdown panel sounds like a good idea. It seems like it would fit better with the rest of Firefox UI and give us more flexibility too. If you have a chance to update the mockup then perhaps we can take that on in 62? Perhaps we could restore the split console icon too.

Comment 21

[Brian Birtles (:birtles)]

Attachment: Add icons to meatball menu

I'm adding the patches for the icons here for posterity since it seems like we won't need them in this bug. I spent quite a bit of time cleaning up the SVGs so hopefully we can use them in the follow-up bug.

Comment 22

[Brian Birtles (:birtles)]

Attachment: Make RTL version of dock to side icon

Comment 23

[Brian Birtles (:birtles)]

(Don't trust splinter for that last patch--you need to check the actual patch file to see how it uses `hg cp` to understand what is going on.)

Comment 24

[Brian Birtles (:birtles)]

Attachment: Bug 1455443 - Convert dock menu options to a checkbox list;

This also fixes the grouping so that the checkboxes appear before the separator
as per the mockup here:

  https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens

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

Comment 25

[Brian Birtles (:birtles)]

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

Comment 26

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #25)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8158da021838023934facfae34acb7928a97f4fe

Oops. I should have used an artifact build for that. Try again:

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

Comment 27

[Brian Birtles (:birtles)]

Attachment: dock-bottom.svg

Comment 28

[Brian Birtles (:birtles)]

Attachment: dock-side-left.svg

Comment 29

[Brian Birtles (:birtles)]

Attachment: dock-side-right.svg

Comment 30

[Brian Birtles (:birtles)]

Attachment: dock-undock.svg

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Birtles (:birtles) from comment #26)
> Oops. I should have used an artifact build for that. Try again:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=71414ac876c1bdb6be209988352607f3799f51a9

Victoria, please take a look at Brian's changes.

If you have a local checkout, you can use `./mach mozregression --launch 71414ac876c1bdb6be209988352607f3799f51a9 --repo try` to automate downloading and launching it.

Otherwise, you can also download a build from:

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

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8970418 [details]
Bug 1455443 - Convert dock menu options to a checkbox list;

https://reviewboard.mozilla.org/r/239194/#review245174

Thanks for working on this! :) The code changes look reasonable to me.

I added a need info for Victoria to review the UX.

::: devtools/client/framework/components/toolbox-controller.js:28
(Diff revision 1)
>        focusedButton: ELEMENT_PICKER_ID,
>        toolboxButtons: [],
>        currentToolId: null,
>        highlightedTools: new Set(),
>        panelDefinitions: [],
> +      hostType: undefined,

Maybe rename to `currentHostType` or `selectedHostType` throughout this patch?

I find it a bit jarring to see both `hostType` and `hostTypes` in the same module with different meanings for each.

::: devtools/client/framework/components/toolbox-toolbar.js:350
(Diff revision 1)
>    }
>  ) {
>    const menu = new Menu({ id: "toolbox-meatball-menu" });
>  
>    // Dock options
> -  for (const hostType of hostTypes) {
> +  for (const thisHostType of hostTypes) {

Change the name of the state as suggested would also allow reverting this name change.

::: devtools/client/locales/en-US/toolbox.properties:154
(Diff revision 1)
>  # LOCALIZATION NOTE (toolbox.meatballMenu.dock.*.label): These labels are shown
>  # in the "..." menu in the toolbox and represent the different arrangements for
>  # docking (or undocking) the developer tools toolbox.
>  toolbox.meatballMenu.dock.bottom.label=Dock to bottom
>  toolbox.meatballMenu.dock.side.label=Dock to side
> -toolbox.meatballMenu.dock.window.label=Undock
> +toolbox.meatballMenu.dock.separateWindow.label=Separate window

Mockup shows a capital "W" on window... I am never sure what's correct, but it seems like what you have matches other entries.

Comment 33

[Victoria Wang [:victoria]]

(In reply to Brian Birtles (:birtles) from comment #20)
> (In reply to Victoria Wang [:victoria] from comment #19)
> > For the future, however, I think it could be worth implementing the custom
> > UI based on the sidebar dropdown panel. It would match the main Firefox
> > meatball menu which uses one of these custom panels too.
> 
> Doing something like that sidebar dropdown panel sounds like a good idea. It
> seems like it would fit better with the rest of Firefox UI and give us more
> flexibility too. If you have a chance to update the mockup then perhaps we
> can take that on in 62? Perhaps we could restore the split console icon too.

Sounds good! Wrote myself a note to update the mockup with the custom panel with images for 62.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #31)
> (In reply to Brian Birtles (:birtles) from comment #26)
> > Oops. I should have used an artifact build for that. Try again:
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=71414ac876c1bdb6be209988352607f3799f51a9
> 
> Victoria, please take a look at Brian's changes.

Thanks, it looks good to me!

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8970418 [details]
Bug 1455443 - Convert dock menu options to a checkbox list;

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d95bd7ea668b
Convert dock menu options to a checkbox list; r=jryans

Comment 36

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d95bd7ea668b

Comment 37

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

I have reproduced this issue using Firefox 61.0a1 (2017.03.07) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b10 on Win 10 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.