Closed Bug 979477 Opened 6 years ago Closed 6 years ago

Increased vertical spacing in australis menu makes it too tall

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: madhava, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 2 obsolete files)

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?
Assignee: nobody → shorlander
Whiteboard: [Australis:P3]
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.
Isn't this just a dupe of bug 978925? If not, how isn't it?
Duplicate of this bug: 978925
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.
- Make labels two lines instead of three with fade out
- Tighten up spacing between icon and label
- Slightly tighten up label line-height
Assignee: shorlander → jaws
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch Patch (obsolete) — Splinter Review
I had to keep the SVG mask to fade out any pixels that come from text on a potential 3rd line.
Attachment #8387176 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8387176 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v2 (obsolete) — Splinter Review
This uses the `clip` property to limit the amount of text that we show to two lines (at 1.1 line height).
Attachment #8387176 - Attachment is obsolete: true
Attachment #8387801 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8387801 - Flags: feedback-
Attachment #8387801 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v3Splinter Review
I restored the overflow code and am now only applying 'clip' under the same conditions that we applied the mask.
Attachment #8387801 - Attachment is obsolete: true
Attachment #8387817 - Flags: review?(gijskruitbosch+bugs)
Attachment #8387817 - Flags: review?(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.
Attachment #8387817 - Flags: review?(mconley) → review+
Comment on attachment 8387817 [details] [diff] [review]
Patch v3

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

Thanks Mike!
Attachment #8387817 - Flags: review?(gijskruitbosch+bugs)
https://hg.mozilla.org/integration/fx-team/rev/50ed255bc181
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/50ed255bc181
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
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
Attachment #8387817 - Flags: approval-mozilla-aurora?
Attachment #8387817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
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.
(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?
Flags: needinfo?(shorlander)
Flags: needinfo?(madhava)
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?
Flags: needinfo?(francesco.lodolo)
You can't reproduce because I changed the string, dropping "new" to fit the remaining text in two lines.
Flags: needinfo?(francesco.lodolo)
QA Whiteboard: [qa-]
Depends on: 1095742
Flags: needinfo?(shorlander)
Flags: needinfo?(madhava)
You need to log in before you can comment on or make changes to this bug.