979477 - Increased vertical spacing in australis menu makes it too tall

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Madhava Enros [:madhava]]

Previous bugs, trying to resolve issues with leaving enough room for menu item labels once localized, have resulted in a default menu like this: 

http://cl.ly/image/0V11193F1b2D

Which on smaller screens (768px tall or 800px tall is still the most common) can leave very little room to customize by adding things. For example, here is the menu shown in a Surface Pro:

http://cl.ly/image/0G3P050v1C3c

Before the vertical space adjustments, there was room for another row (or at least a mostly visible row).

Also - the new larger spacing looks odd when there's no visible need for it (i.e. in en-US by default).

Can we try to claw back some of that vertical space?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

For customize mode, we could choose not to increase the margin at the bottom of the content-deck - so we'd only be animating / increasing the margins on the left and right of the toolbars / content-deck.

Comment 2

[:Gijs (he/him)]

Isn't this just a dupe of bug 978925? If not, how isn't it?

Comment 4

[Justin Dolske [:Dolske]]

Maybe we could hide the Sync bar when in Customization mode? It's about a wash with the extra padding added to the window when customizing, so it wouldn't be too wonky (ie, not a concern of having something visible in customization but overflowed in normal mode). Does have the downsize of it making the panel appearance a bit mode-specific, but maybe that's no big deal.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: Menu Panel Button Spec - 01

- Make labels two lines instead of three with fade out
- Tighten up spacing between icon and label
- Slightly tighten up label line-height

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I had to keep the SVG mask to fade out any pixels that come from text on a potential 3rd line.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8387176 [details] [diff] [review]
Patch

Review of attachment 8387176 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem to actually work. I tried it on OS X and the lower padding is wrong, and attempting to make it higher still shows more text, instead of masking it out...

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This uses the `clip` property to limit the amount of text that we show to two lines (at 1.1 line height).

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8387801 [details] [diff] [review]
Patch v2

>             case "overflow":
>-              switch (aEvent.target.localName) {
>-                case "vbox":
>-                  // Resize the right view on the next tick.
>-                  if (this.showingSubView) {
>-                    setTimeout(this._syncContainerWithSubView.bind(this), 0);
>-                  } else if (!this.transitioning) {
>-                    setTimeout(this._syncContainerWithMainView.bind(this), 0);
>-                  }
>-                  break;
>-                case "toolbarbutton":
>-                  aEvent.target.setAttribute("fadelabel", "true");
>-                  break;
>+              // Resize the right view on the next tick.
>+              if (this.showingSubView) {
>+                setTimeout(this._syncContainerWithSubView.bind(this), 0);
>+              } else if (!this.transitioning) {
>+                setTimeout(this._syncContainerWithMainView.bind(this), 0);
>               }

You're still getting the overflow event for toolbarbuttons and potentially other elements here. You shouldn't call _syncContainerWithMainView / _syncContainerWithSubView for those.

You also may want to check the performance characteristics of the clip property and whether it would make sense to apply it only to toolbarbuttons that overflow.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

I restored the overflow code and am now only applying 'clip' under the same conditions that we applied the mask.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8387817 [details] [diff] [review]
Patch v3

Review of attachment 8387817 [details] [diff] [review]:
-----------------------------------------------------------------

It's a shame to lose the fade-out work you folks did, but well, that's life, I guess.

This looks fine to me. Tested on Windows 7.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8387817 [details] [diff] [review]
Patch v3

Review of attachment 8387817 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Mike!

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/50ed255bc181

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50ed255bc181

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8387817 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 897496
User impact if declined: the menu panel is too tall for some small screens
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bd7398f6152

Comment 17

[Francesco Lodolo [:flod]]

Thanks to Gijs for pointing me to this bug (coming from bug 897496).

I'll be honest: this looks awful. Thanks to this change, on OS X I have one of the main buttons saying 

Nuova fine-
stra anoni-

It would be like having

New pri-
vate win-

The fade out was a compromise, but at least gave a hint to users of what was going on. Now the menu looks just broken.

Is there a plan to get rid completely of the labels and leave only tooltips? Because I can assure you this does look terrible, and I think there are locales in even worse conditions.

Comment 18

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #17)
> Is there a plan to get rid completely of the labels and leave only tooltips?
> Because I can assure you this does look terrible, and I think there are
> locales in even worse conditions.

I'm worried about this. Stephen/Madhava, do we have alternative designs that we could go with so that l10n isn't so badly impacted?

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

I'm unable to reproduce the issue mention in comment 17, neither on Windows, Mac nor Ubuntu platforms using Firefox 29 beta 6.

"Finestra anonima" is correctly displayed in 2 rows. 

On two rows is displayed as it follows (which is correct):
nuova sche-
da

Francesco, do you still see the behaviour mentioned in your comment using Fx 29 beta 6 or latest Aurora?

Comment 20

[Francesco Lodolo [:flod]]

You can't reproduce because I changed the string, dropping "new" to fit the remaining text in two lines.