Closed Bug 1455443 Opened 6 years ago Closed 6 years ago

Improve usability of docking options in new meatball menu

Categories

(DevTools :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified

People

(Reporter: victoria, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

Attached image separatewindow.svg (obsolete) —
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
Attached image dock-undock-expanded.svg (obsolete) —
Oops, something was wrong with that previous icon. Attaching a new one
Attachment #8969428 - Attachment is obsolete: true
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.
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?
(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).
Assignee: nobody → bbirtles
Blocks: 1444299
Status: NEW → ASSIGNED
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);
> }
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.
(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.
(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
(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.
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(victoria)
(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).
(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?).
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.
(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.
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.
(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.)
(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
Attached image dock-undock.svg
Attachment #8969432 - Attachment is obsolete: true
(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
Flags: needinfo?(victoria)
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.
Attachment #8970418 - Flags: review?(jryans) → review+
(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!
Flags: needinfo?(victoria)
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d95bd7ea668b
Convert dock menu options to a checkbox list; r=jryans
https://hg.mozilla.org/mozilla-central/rev/d95bd7ea668b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.