969584 - Consolidate typography and sizing of panel subview items

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: 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

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Since this falls into the same category:
The padding around the panels should also be the same as in the mockup.

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Ah, forgot the Whiteboard.
P- because it blocks bug 963098 which is P2

Comment 3

[:Gijs (he/him)]

(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.

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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?

Comment 7

[Dão Gottwald [:dao]]

(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.

Comment 8

[Tim Nguyen :ntim]

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.

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: 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.

Comment 10

[:Gijs (he/him)]

Attachment: — Consolidate typography and sizing of panel subview items

Unconflicting

Comment 11

[u428464]

Is this waiting on something else before going further ?

Comment 12

[:Gijs (he/him)]

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

Yes.

Comment 13

[:Gijs (he/him)]

Attachment: 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.

Comment 14

[:Gijs (he/him)]

Attachment: 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...

Comment 15

[:Gijs (he/him)]

Attachment: 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.

Comment 16

[Mike de Boer [:mikedeboer]]

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'.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-osx1.png

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-osx2.png

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-osx3.png

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-osx4.png

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-osx5.png

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-linux1.png

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: bug-969584-linux2.png

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8391264 [details]
bug-969584-osx2.png

With 10.8 SDK this looks fine!

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8391265 [details]
bug-969584-osx3.png

With 10.8 SDK this looks fine!

Comment 26

[:Gijs (he/him)]

(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)

Comment 27

[:Gijs (he/him)]

Attachment: 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...)

Comment 28

[Mike de Boer [:mikedeboer]]

I will review this today, after chores.

Comment 29

[Mike de Boer [:mikedeboer]]

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.

Comment 30

[:Gijs (he/him)]

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/7103e5ec9b11

Comment 31

[Tim Nguyen :ntim]

If this updates the footer's padding, it should also update the downloads footer padding (at least for Windows 8).

Comment 32

[:Gijs (he/him)]

(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.

Comment 33

[:Gijs (he/him)]

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

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7103e5ec9b11

Comment 35

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f230ea9749c4

Comment 36

[Clever]

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.

Comment 37

[(mostly gone) XtC4UaLL [:xtc4uall]]

(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 ;-)