Consolidate typography and sizing of panel subview items

RESOLVED FIXED in Firefox 29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: phlsa, Assigned: Gijs)

Tracking

28 Branch
Firefox 30
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

(Whiteboard: [Australis:P2])

Attachments

(7 attachments, 7 obsolete attachments)

282.38 KB, image/png
Details
357.84 KB, image/png
Details
307.61 KB, image/png
Details
266.29 KB, image/png
Details
95.17 KB, image/png
Details
11.41 KB, image/png
Details
15.37 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Created attachment 8372519 [details]
Explanation

We currently use different font sizes on the menu panel, the bookmarks panel, submenus of the bookmarks panel, the history panel and other panel subviews.

1) All panels should have the same font size
2) All panels should have the same spacing between list items
3) The main view of the menu panel is the exception, because it follows a different paradigm (icon grid)

Shorlanders mockup shows the intended font size, padding/line-height and styling for Windows. Specs for Mac and XP are on the way, but I believe they will be either identical or very close, at least as far as typography is concerned.
https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html
Since this falls into the same category:
The padding around the panels should also be the same as in the mockup.
Whiteboard: [Australis:P?]
Ah, forgot the Whiteboard.
P- because it blocks bug 963098 which is P2
OS: Mac OS X → All
Whiteboard: [Australis:P?] → [Australis:P-]
(Assignee)

Comment 3

5 years ago
(In reply to Philipp Sackl [:phlsa] from comment #0)
> Created attachment 8372519 [details]
> Explanation
> 
> We currently use different font sizes on the menu panel, the bookmarks
> panel, submenus of the bookmarks panel, the history panel and other panel
> subviews.
> 
> 1) All panels should have the same font size
> 2) All panels should have the same spacing between list items
> 3) The main view of the menu panel is the exception, because it follows a
> different paradigm (icon grid)
> 
> Shorlanders mockup shows the intended font size, padding/line-height and
> styling for Windows. Specs for Mac and XP are on the way, but I believe they
> will be either identical or very close, at least as far as typography is
> concerned.
> https://people.mozilla.org/~shorlander/mockups-interactive/australis-
> interactive-mockups/windows8.html

This is still confusing. Here are some attempts at specific questions:

The main menu panel fonts are sized based on the default OS font size (which they match - by default that's 11px on Windows and Mac, and 15.3 or whatever on Ubuntu Linux (but probably something else on other distros, and many users customize this value)). Should that change, that is, should we stop obeying the OS default font size?

The mockup uses a smaller font for the main panel than it does for subviews. I'm assuming based on point (3) above that that is intentional. However, if we want to honor OS styling for the main panel, are you sure that we want to make the subviews bigger? How much bigger (in em, where 1em is the main grid style)?

If we don't want to honor OS styling, are we sure we want to go back to 10px in the main panel (as per mockup), styling which was previously removed because it's illegible on Linux and still 'pretty bad' on OS X and Windows, depending on DPI and your eyes and so on? If not, and we don't want OS styling, what size should it be?


The ideal answer here would really be something that specifies, for each of the following:

1) main panel grid
2) subviews in main panel
3) standalone panels
4) submenus from the standalone bookmarks panel

what size font *and* line-height should be used. Keep in mind that if we're following OS style, all values should be in em, not px (like the mockup).

Personally, I think the simplest option is to just use the default panel font size everywhere, ie have the grid, subviews and standalone panel and its submenus all use the default font-size for panels.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 940452
(Assignee)

Updated

5 years ago
Duplicate of this bug: 966403
(Assignee)

Updated

5 years ago
Blocks: 969592
(Assignee)

Updated

5 years ago
Summary: Consolidate typography of panels → Consolidate typography and sizing of panel subview items
(In reply to :Gijs Kruitbosch from comment #3)
> This is still confusing. Here are some attempts at specific questions:
> 
> The main menu panel fonts are sized based on the default OS font size (which
> they match - by default that's 11px on Windows and Mac, and 15.3 or whatever
> on Ubuntu Linux (but probably something else on other distros, and many
> users customize this value)). Should that change, that is, should we stop
> obeying the OS default font size?
> 
> The mockup uses a smaller font for the main panel than it does for subviews.
> I'm assuming based on point (3) above that that is intentional. However, if
> we want to honor OS styling for the main panel, are you sure that we want to
> make the subviews bigger? How much bigger (in em, where 1em is the main grid
> style)?
> 
> If we don't want to honor OS styling, are we sure we want to go back to 10px
> in the main panel (as per mockup), styling which was previously removed
> because it's illegible on Linux and still 'pretty bad' on OS X and Windows,
> depending on DPI and your eyes and so on? If not, and we don't want OS
> styling, what size should it be?
> 
> 
> The ideal answer here would really be something that specifies, for each of
> the following:
> 
> 1) main panel grid
> 2) subviews in main panel
> 3) standalone panels
> 4) submenus from the standalone bookmarks panel
> 
> what size font *and* line-height should be used. Keep in mind that if we're
> following OS style, all values should be in em, not px (like the mockup).
> 
> Personally, I think the simplest option is to just use the default panel
> font size everywhere, ie have the grid, subviews and standalone panel and
> its submenus all use the default font-size for panels.

I generally agree. The bookmarks panel currently uses the same font-size as system menus (at least on Windows) and I think that's a sensible choice. It is minimally smaller than in the mockup (caps height is the same, but lower case characters appear smaller).

We can't use the same line height as native menus in the bookmarks panel though, because our highlight is too large. So we either need to change the highlight or the line height (I'm currently favoring the line height because the system default looks crammed when showing many long lines).

The main panel menu is also the only place where deviating from the system default sounds like a good idea, because the text is not the primary identifier for an item. Perhaps we could do something like »85% of the system default« there?

Stephen, what do you think?
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] from comment #6)
> The main panel menu is also the only place where deviating from the system
> default sounds like a good idea, because the text is not the primary
> identifier for an item. Perhaps we could do something like »85% of the
> system default« there?

We had something like that before. It tends to be too small and ends up being an accessibility problem. We shouldn't undercut the system default.
For subviews (at least on Windows), I think the font size is too big, the character encoding view barely fits. The bookmarks widget font size is perfect for Windows.
Created attachment 8386869 [details] [diff] [review]
consolidate-panel-subview-styling.patch

This patch:
- Changes font for subview list items to menu
- Removes margins on list items
- Reduces padding on list items
- Removes transition on list items

Haven't tested on Windows yet.
Flags: needinfo?(shorlander)
(Assignee)

Comment 10

5 years ago
Created attachment 8386923 [details] [diff] [review]
— Consolidate typography and sizing of panel subview items

Unconflicting
(Assignee)

Updated

5 years ago
Attachment #8386869 - Attachment is obsolete: true

Comment 11

5 years ago
Is this waiting on something else before going further ?
(Assignee)

Comment 12

5 years ago
(In reply to Guillaume C. [:ge3k0s] from comment #11)
> Is this waiting on something else before going further ?

Yes.
(Assignee)

Updated

5 years ago
No longer blocks: 969592
Depends on: 969592
(Assignee)

Updated

5 years ago
Whiteboard: [Australis:P-] → [Australis:P2]
(Assignee)

Comment 13

5 years ago
Created attachment 8390588 [details] [diff] [review]
Patch 1: Consolidate typography and sizing of panel subview items on Windows

So this is the windows part so far... I think the checkmarks should now be fine, the size of items should be OK (the ones in the panel are still smaller than the bookmarks ones, though... could switch to 3px top/bottom margin on the toolbarbutton-text if we wanted to change that), and I fixed the multiple feeds submenu to not be transparent, and I fixed the top/bottom margins of the submenus, and of course the original font issue. Mike, can you check this for code issues and on XP/Win8? If you have extra time, non-default fonts, but I *think* it should hold up (apart from the width issue that Jared is investigating). The one thing I didn't figure out how to fix is the submenu positioning. The box-shadow is already cut-off at the top on win7, and moving it up enough to make the items line up causes the border of the panel to be cut off, too. Not sure what to do about that, but I think I could live with shipping that.
Attachment #8390588 - Flags: review?(mdeboer)
Attachment #8386923 - Attachment is obsolete: true
Attachment #8390588 - Attachment description: — Consolidate typography and sizing of panel subview items, → Patch 1: Consolidate typography and sizing of panel subview items on Windows
(Assignee)

Comment 14

5 years ago
Created attachment 8390628 [details] [diff] [review]
Patch 2: Consolidate typography and sizing of panel subview items,

OS X was less work, surprisingly. This looks good to me. I tweaked the disabled text style for windows, which we were overriding up till now, and I ended up using last-of-type to make margin-bottoms work on 'normal' bookmark folders that have only 1 item (instead of the specific selectors for the bookmarks toolbar 'folder' and the recently bookmarked, which never have the footer), which means their footer gets removed. I couldn't see a better way because of how adding padding to the bottom of the container and negative margin-bottom on the footer doesn't seem to work, but we've been over that...
Attachment #8390628 - Flags: review?(mdeboer)
(Assignee)

Updated

5 years ago
Attachment #8390588 - Attachment is obsolete: true
Attachment #8390588 - Flags: review?(mdeboer)
(Assignee)

Comment 15

5 years ago
Created attachment 8390876 [details] [diff] [review]
Patch 3: Consolidate typography and sizing of panel subview items,

Now with linux support, and 22px height everywhere (ymmv on Linux depending on your system font sizes). I may have fixed another stray issue or two that I have forgotten about by now. If the comments don't clarify why I did things the way I did them, please ask.
Attachment #8390876 - Flags: review?(mdeboer)
(Assignee)

Updated

5 years ago
Attachment #8390628 - Attachment is obsolete: true
Attachment #8390628 - Flags: review?(mdeboer)
Comment on attachment 8390876 [details] [diff] [review]
Patch 3: Consolidate typography and sizing of panel subview items,

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

General points:

- The Feeds subview items have padding-start as if they'd contain an icon, but they don't. Can you remove that padding or put a generic feed icon in front of it?
- Same as above for the Developer subview.
- In the History subview, the 'Clear Recent History...' and 'Restore X' items also show padding at the start, event though they're not checkmark items or have an icon.

Note: In menu popups on Windows we also have this horizontal space for items without icons/ checkmark/ radio bullet, but this is looks better due to a vertical separator across the whole length of the popup. I'm not saying we should do this, but the fact that it's there punctuates the undesired effect these floating items have without a visual indication like a vertical separator.
I understand if you want to pass that to a follow-up, which I'd be happy to take.

- On Linux and OSX I can see cut-off buttons in the submenus without footers; the ones at the bottom. 
- On Linux, the footer block looks like it's swimming in vertical space...
- On Linux, in Menu Panel subviews, footer labels are not centered horizontally.
- On Linux, in Menu Panel subviews, icons and labels are too close together (not enough margin-end for icons).
- On OSX, the checkmarks are not properly aligned on the vertical axis.
- On OSX, the items with the reduced vertical padding look bad. I think OSX is the only platform that does not need these adjustments.
- On OSX, the horizontal alignment is off; there's enough space for two icons at the start of each button.

I think that lists them all for the moment.

::: browser/themes/linux/browser.css
@@ +86,5 @@
>    margin: 0;
>    padding: 2px 3px;
>  }
>  
> +toolbarbutton.bookmark-item:hover:active:not(.subviewbutton),

nit: I believe we generally put pseudo-classes at the end of selectors, to improve legibility wrt other states.

::: browser/themes/linux/customizableui/panelUIOverlay.css
@@ +16,5 @@
> +  margin-top: -6px;
> +  padding-top: 2px;
> +}
> +
> +/* Add some space at the top because there's no headers: */

nit: 'there are no...'

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +912,5 @@
>    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
>  }
>  
>  .subviewbutton[checked="true"] {
> +  background: url("chrome://global/skin/menu/shared-menu-check.png") center left 7px / 11px 11px no-repeat transparent;

In hindsight, I don't really like the notion of 'shared' in the code/ URL. I wish it to just be 'menu-check.png'.
Attachment #8390876 - Flags: review?(mdeboer) → feedback+
Created attachment 8391263 [details]
bug-969584-osx1.png
Created attachment 8391264 [details]
bug-969584-osx2.png
Created attachment 8391265 [details]
bug-969584-osx3.png
Created attachment 8391266 [details]
bug-969584-osx4.png
Created attachment 8391267 [details]
bug-969584-osx5.png
Created attachment 8391274 [details]
bug-969584-linux1.png
Created attachment 8391276 [details]
bug-969584-linux2.png
Comment on attachment 8391264 [details]
bug-969584-osx2.png

With 10.8 SDK this looks fine!
Attachment #8391264 - Attachment is obsolete: true
Comment on attachment 8391265 [details]
bug-969584-osx3.png

With 10.8 SDK this looks fine!
Attachment #8391265 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> Comment on attachment 8390876 [details] [diff] [review]
> Patch 3: Consolidate typography and sizing of panel subview items,
> 
> Review of attachment 8390876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General points:
> 
> - The Feeds subview items have padding-start as if they'd contain an icon,
> but they don't. Can you remove that padding or put a generic feed icon in
> front of it?

I'd like to followup the feeds subview.

> - Same as above for the Developer subview.

Several of the items in there have checkmarks. It was decided in the various panel checkmark bugs that that is how it should work:

https://bugzilla.mozilla.org/show_bug.cgi?id=979378#c9

> - In the History subview, the 'Clear Recent History...' and 'Restore X'
> items also show padding at the start, event though they're not checkmark
> items or have an icon.

See above. Note also that I didn't change anything about the horizontal alignment of the checkmarks or the items, so all of that seems quite orthogonal to this bug. If you want to have a discussion about this with Stephen and followup, that's fine, but I'd really like to keep it out of this bug. Please. :-)

> - On Linux and OSX I can see cut-off buttons in the submenus without
> footers; the ones at the bottom. 

I can't reproduce on either OS X or Linux.

> - On Linux, the footer block looks like it's swimming in vertical space...

I don't see a screenshot for this, did I miss it? Looks like this for me: http://i.imgur.com/b3cwokX.png (apologies for the retina-induced weird resolution). Looks about the same as OS X... so I wasn't planning on touching them.

> - On Linux, in Menu Panel subviews, footer labels are not centered
> horizontally.

bug 979987, also not this bug.

> - On Linux, in Menu Panel subviews, icons and labels are too close together
> (not enough margin-end for icons).

I can't reproduce this either, and I don't think I touched the margin-end for icons...

(the other bits you already obsoleted, so I assume they're no longer an issue)
(Assignee)

Comment 27

5 years ago
Created attachment 8391403 [details] [diff] [review]
Patch 3: Consolidate typography and sizing of panel subview items,

This fixes the vertical cut offs, the styling of the random charset element, and the right margin on icons on Linux (oh, Linux...)
Attachment #8391403 - Flags: review?(mdeboer)
(Assignee)

Updated

5 years ago
Attachment #8390876 - Attachment is obsolete: true
I will review this today, after chores.
Comment on attachment 8391403 [details] [diff] [review]
Patch 3: Consolidate typography and sizing of panel subview items,

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

Superbe! Awesome work, thanks! With small nits fixed, r=me.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +622,5 @@
>  .PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .menu-iconic-text {
> +  font: menu;
> +}
> +
> +/* Let me tell you about this one crazy element we use: */

nit: what kind of information does this comment provide us?

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +62,5 @@
> +  margin-bottom: 2px;
> +}
> +
> +/* Disabled empty item looks too small otherwise, because
> + * it has no icon. */

nit: I think this comment will fit on one line.

@@ +65,5 @@
> +/* Disabled empty item looks too small otherwise, because
> + * it has no icon. */
> +menuitem.subviewbutton[disabled]:not(.menuitem-iconic),
> +/* Same for checkbox menu items, whose icons lose size due to
> + * -moz-appearance: none: */

nit: I think this comment will fit on one line.

@@ +67,5 @@
> +menuitem.subviewbutton[disabled]:not(.menuitem-iconic),
> +/* Same for checkbox menu items, whose icons lose size due to
> + * -moz-appearance: none: */
> +menuitem[type="checkbox"].subviewbutton {
> +  /* This is 16px for an icon + 3px for its margins + 2px for its padding + 2px for its border, see above */

nit: whereas here you should change this comment to two lines.
Attachment #8391403 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 30

5 years ago
Fixed all the nits (also the previous ones, oops...) and landed as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/7103e5ec9b11
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
If this updates the footer's padding, it should also update the downloads footer padding (at least for Windows 8).
(Assignee)

Comment 32

5 years ago
(In reply to Tim Nguyen [:ntim] from comment #31)
> If this updates the footer's padding, it should also update the downloads
> footer padding (at least for Windows 8).

It did not, as noted in comment #26 . It's strictly limited to the bookmark button's menus and the items inside the other view-widget subviews.
(Assignee)

Comment 33

5 years ago
Comment on attachment 8391403 [details] [diff] [review]
Patch 3: Consolidate typography and sizing of panel subview items,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bookmarks panel is much larger than it needs to be, showing fewer bookmarks, making it less usable for people with many bookmarks
Testing completed (on m-c, etc.): soon to be on m-c! :-)
Risk to taking this patch (and alternatives if risky): pretty low, almost only CSS changes
String or IDL/UUID changes made by this patch: none
Attachment #8391403 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7103e5ec9b11
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
status-firefox29: --- → affected
status-firefox30: --- → fixed
Attachment #8391403 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 36

5 years ago
In the latest Nightly on Windows, the highlight for the Open "Site" item in bookmarked RSS feeds is the system default instead of the customized Australis one. This should be corrected to match the rest of the items.

The new changes are looking awesome! Everything seems to be styled consistently with good spacing and size. The visible difference between system separators and user separators is a nice touch as well.
(In reply to Clever from comment #36)
> In the latest Nightly on Windows, the highlight for the Open "Site" item in
> bookmarked RSS feeds is the system default instead of the customized
> Australis one. This should be corrected to match the rest of the items.

For reference, this has been already filed as Bug 985659.
In the future, don't hesitate to file a report if you see a issue since bugs - even as dupes - are cheap ;-)

Updated

5 years ago
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.