The styling for the panel UI is significantly different on OSX than on Windows. This is undesirable for a number of reasons: * UX and the front-end engineers often use OSX, leaving the Windows version significantly uglier. * Bugs like bug 1233869 ask for specific alignments, which means platform specific CSS will be necessary to get the same look on Windows. To demonstrate I've made a trivial panel UI with a toolbarbutton and label and no special styling. In the attached image, OSX is on the left and Windows is on the right. Of particular note: * The label uses a smaller font on OSX. * The image is right-aligned on Windows, but left aligned on OSX. These differences are also visible by looking at, eg, the "history" panel where: * On the left, there's significantly more whitespace on OSX than Windows. * The left of the "Restore Closed Tabs" button is aligned with the left of the favicon on OSX, whereas it is aligned with the left of the text on Windows. * The "Show all history" label at the bottom of the menu is small text on OSX but normal text on Windows. These differences should be obvious if you look at the history panel on both platforms, and are undesirable as it makes bugs like bug 1233869 get things looking pretty on OSX but leaves it ugly on Windows due to a lack of visibility of the UI on Windows by UX people who only use OSX.

UX would like them to be consistent. The desired style is present here: https://mozilla.invisionapp.com/d/main#/console/4656956/111461103/preview

I don't really remember the history of styling subviews. I assume we got them looking as-expected for Australis's use (bookmarks & history), but wouldn't be too surprised if they could be cleaned up, or the style issues you're running into are things Australis simply didn't specifically address. (In particular, sticking a <toolbarbutton> into a <panelview> -- pretty sure putting these buttons into things which are not toolbars or the main menupanel is going to look odd.) Gijs or Dao: you remember anything here? Is this just in need of some cleanup and polish?

(In reply to Justin Dolske [:Dolske] from comment #3) > Is this just in need of some cleanup and polish? I think the simple answer here is "yes", but that likely involves making an actual unified spec, too. The specifics there might be tricky (also in terms of e.g. the icon vs. text alignment from comment #0, which is expected to be different in menus when comparing OSX and Windows, so maybe we want the same here? Unsure!).

"Things should look the same across platforms" should be taken with a grain of salt. For instance, default font sizes are different (and customizable on Windows and Linux, I guess they aren't on OS X), so you shouldn't expect identical results. E.g. if a mockup uses 13px font size, this likely doesn't mean you should hardcode a 13px font size. (There are scenarios where hardcoding absolute font size might make sense, but they're rare exceptions.) As Gijs mentioned, another example is that standard menus on OS X look different than on Windows / Linux. This may or may not be relevant here. That said, the spacing around the icon in attachment 8702472 [details] [diff] [review] looks weird on both Windows and OS X. Clearly it shouldn't look weird regardless of whether it's the same across platforms. Did we expect <menuitem/> to be used instead of <toolbarbutton/> in subviews? I don't know. Likewise, it's possible that we didn't expect <label/> to be used in this context.

(In reply to Dão Gottwald [:dao] from comment #5) > "Things should look the same across platforms" should be taken with a grain > of salt. For instance, default font sizes are different (and customizable on > Windows and Linux, I guess they aren't on OS X), so you shouldn't expect > identical results. E.g. if a mockup uses 13px font size, this likely doesn't > mean you should hardcode a 13px font size. (There are scenarios where > hardcoding absolute font size might make sense, but they're rare exceptions.) To me "Things should look the same across platforms" means that unless there is a platform specific reason for it, then we should be consistent everywhere as much as possible. In other words internal consistency should be the rule and platform specific differences should be the exception. Some things, like fonts, are relative. Generally when we reference a font-size on Windows in design it should be based on a known default value. For instance we know in a default install of Windows XP or Windows 7 what the default font-sizes are. Some other things are probably not relative. For example most margins, padding and spacing between things like icon and text can (usually) be explicitly stated. There is a fairly significant difference in menus between Linux, Windows and OS X: In OS X checkmarks appear in a gutter next to the menu items but on Windows the checkmark and any icons occupy the same space and in Linux there usually aren't icons in menu items. Other menus like Bookmarks and History look nicely aligned on all platforms. Maybe those were special cased?

(In reply to Ryan Feeley [:rfeeley] from comment #2) > UX would like them to be consistent. The desired style is present here: > https://mozilla.invisionapp.com/d/main#/console/4656956/111461103/preview https://mozilla.invisionapp.com/share/QP4SOTTK3#/screens is a version that doesn't require a login

I discussed this with Mark, we expect regressions but at least it's a starting point in order to erase the differences between platforms. (Screenshots in the next comment)

Comment on attachment 8738308 [details] [diff] [review] bug-1235501.patch Review of attachment 8738308 [details] [diff] [review]: ----------------------------------------------------------------- This massively increases the padding in the bookmarks menu and the history subview on Windows, at least, so this won't do as-is. Can you maybe explain specifically what you're trying to address? Some of the changes seem arbitrary at first sight. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +1406,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") top 5px left 4px / 11px 11px no-repeat transparent; Why did this change? centering it vertically seems to make more sense than using top 5px. It's also not clear why you're moving the checkmark further left. ::: browser/themes/windows/syncedtabs/sidebar.css @@ +82,5 @@ > +.item-title-container { > + box-sizing: border-box; > + align-items: center; > + height: 24px; > + font-size: 12px; px-based font-sizes are not OK, please use em instead. Also, this seems like it is completely unrelated to this patch? ::: toolkit/themes/windows/global/toolbarbutton.css @@ -22,5 @@ > } > > -.toolbarbutton-icon[label]:not([label=""]), > -.toolbarbutton-icon[type="menu"] { > - -moz-margin-end: 5px; This affects other apps, and the rule seems to make sense to me. Why remove it?

Comment on attachment 8738308 [details] [diff] [review] bug-1235501.patch Review of attachment 8738308 [details] [diff] [review]: ----------------------------------------------------------------- > This massively increases the padding in the bookmarks menu and the history subview on Windows, at least, so this won't do as-is. This is intended, see comment 7 UX document, it seems like they want the padding to be consistent across platforms. > Can you maybe explain specifically what you're trying to address? Some of the changes seem arbitrary at first sight . Of course, I apologize this patch is not very straightforward to understand. I commented directly on the review rule per rule to describe what I tried to do. ::: browser/themes/linux/customizableui/panelUIOverlay.css @@ -25,5 @@ > -.subviewbutton > .toolbarbutton-text { > - padding-top: 3px; > - padding-bottom: 3px; > -} > - (1): Removed for the sake of consistency (no such padding on OSX), I don't mind restoring it if this is intended. @@ -39,5 @@ > > -.subviewbutton > .toolbarbutton-text { > - -moz-padding-start: 16px; > -} > - (2bis): We want a shared rule for the toolbarbutton paddings, see panelUIOverlay.inc.css rule (2). @@ -79,5 @@ > > -.subviewbutton > .toolbarbutton-icon { > - -moz-margin-end: 5px !important; > -} > - (4bis): This rule is unified across platforms in panelUIOverlay.inc.css rule (4) ::: browser/themes/osx/customizableui/panelUIOverlay.css @@ -36,5 @@ > - > -.subviewbutton[checked="true"]:-moz-locale-dir(rtl) { > - background-position: top 5px right 4px; > -} > - See panelUIOverlay.inc.css rules (2), (4) and (3) @@ -69,5 @@ > -.PanelUI-subView menuseparator, > -.cui-widget-panelview menuseparator { > - padding: 0 !important; > -} > - (5): There's already a rule line 1140 in panelUIOverlay.inc.css that sets the padding to 0. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +963,5 @@ > } > > +.subviewbutton { > + -moz-padding-start: 18px; > +} (2): unified padding rule for subview buttons @@ +971,5 @@ > +} > + > +.subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) > .toolbarbutton-text { > + margin: 2px 6px !important; /* !important for overriding toolbarbutton.css */ > +} (4): These 2 rules unify the padding after an icon, see other 4bis rules @@ +976,5 @@ > + > +.restoreallitem > .toolbarbutton-icon { > + display: none; > +} > + (3): Consistency across platforms @@ +1406,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") top 5px left 4px / 11px 11px no-repeat transparent; Center: OK, corrected. Left alignment: In order for this to work with rules (2)/(2bis), we need to move to the left. (The checkbox is still at the same position in the screen) ::: browser/themes/windows/customizableui/panelUIOverlay.css @@ -70,5 @@ > -.subviewbutton > .toolbarbutton-text { > - padding-top: 3px; > - padding-bottom: 3px; > -} > - (1): Removed for the sake of consistency (no such padding on OSX), I don't mind restoring it if this is intended. @@ -90,5 @@ > > -.subviewbutton > .toolbarbutton-text { > - -moz-padding-start: 16px; > -} > - (2bis): We want a shared rule for the toolbarbutton paddings, see panelUIOverlay.inc.css rule (2). ::: browser/themes/windows/syncedtabs/sidebar.css @@ +82,5 @@ > +.item-title-container { > + box-sizing: border-box; > + align-items: center; > + height: 24px; > + font-size: 12px; It is in fact unrelated, the goal was to unify the sync sidebar look on windows too, but this goes beyond the scope of this bug. I’m gonna remove this, we can make another bug for this one later. ::: toolkit/themes/windows/global/toolbarbutton.css @@ -22,5 @@ > } > > -.toolbarbutton-icon[label]:not([label=""]), > -.toolbarbutton-icon[type="menu"] { > - -moz-margin-end: 5px; (4bis): This rule is unified across platforms in panelUIOverlay.inc.css rule (4)

(In reply to Edouard Oger [:eoger] from comment #11) > Comment on attachment 8738308 [details] [diff] [review] > bug-1235501.patch > > Review of attachment 8738308 [details] [diff] [review]: > ----------------------------------------------------------------- > > > This massively increases the padding in the bookmarks menu and the history subview on Windows, at least, so this won't do as-is. > This is intended, see comment 7 UX document, it seems like they want the > padding to be consistent across platforms. Which "UX document" ? comment 6 here explicitly calls out this difference (gutter / icon / checkbox stuff) as intentional. I don't think we should diverge from platform convention.

(In reply to :Gijs Kruitbosch from comment #12) > (In reply to Edouard Oger [:eoger] from comment #11) > > Comment on attachment 8738308 [details] [diff] [review] > > bug-1235501.patch > > > > Review of attachment 8738308 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > > This massively increases the padding in the bookmarks menu and the history subview on Windows, at least, so this won't do as-is. > > This is intended, see comment 7 UX document, it seems like they want the > > padding to be consistent across platforms. > > Which "UX document" ? comment 6 here explicitly calls out this difference > (gutter / icon / checkbox stuff) as intentional. I don't think we should > diverge from platform convention. If you mean the invision app thing linked from comment 7, that explicitly says "Mac OS X" above the panels.

(In reply to Edouard Oger [:eoger] from comment #11) > ::: toolkit/themes/windows/global/toolbarbutton.css > @@ -22,5 @@ > > } > > > > -.toolbarbutton-icon[label]:not([label=""]), > > -.toolbarbutton-icon[type="menu"] { > > - -moz-margin-end: 5px; > > (4bis): This rule is unified across platforms in panelUIOverlay.inc.css rule > (4) We can't just remove rules from toolkit where they apply to all toolbarbuttons and then only apply them to some things in subviews in panels.

Okay here's another shot at this, I reduced the scope of the patch and tried to work mainly on the left padding of toolbarbutton/icons inside PanelUI. I'm afraid must insist about the left padding in the history and bookmarks panel: as you can see in the invision app, if you scroll down these are Windows prototypes and they have the huge padding too.

Comment on attachment 8738665 [details] [diff] [review] bug-1235501.patch (In reply to Edouard Oger [:eoger] from comment #15) > Created attachment 8738665 [details] [diff] [review] > bug-1235501.patch > > Okay here's another shot at this, I reduced the scope of the patch and tried > to work mainly on the left padding of toolbarbutton/icons inside PanelUI. > > I'm afraid must insist about the left padding in the history and bookmarks > panel: as you can see in the invision app, if you scroll down these are > Windows prototypes and they have the huge padding too. I think this is a mistake. It doesn't fit with in with the styling of the main menus or the rest of the OS or Firefox UI. Please can we reconsider? Stephen? In the meantime, the patch attached does not match the spec. The padding on the top few items of the bookmarks menu is very obviously wrong compared to spec, but the remaining items are also 1-2px out from where the spec says they should be. The spec itself is inconsistent because some items are 23px out and others 24. Otherwise, this patch increases the vertical padding on subview items significantly, so that looks to be wrong too.

Could I get some clarification on what the problem is? I don't see any usually huge spacing issues.

(In reply to :Gijs Kruitbosch from comment #18) > > I'm afraid must insist about the left padding in the history and bookmarks > > panel: as you can see in the invision app, if you scroll down these are > > Windows prototypes and they have the huge padding too. > > I think this is a mistake. It doesn't fit with in with the styling of the > main menus or the rest of the OS or Firefox UI. Please can we reconsider? Gijs, to be clear, you are talking about the space between the left of the panel and the start of the elements - ie, the space shown in the following attachment (which is taken from Stephen's mockup of the History panel), right? It's roughly a 19 pixel gap, which isn't really huge, and IMO is quite an improvement over the status-quo, but I want to make sure that is what we are asking Stephen to reconsider.

(In reply to Mark Hammond [:markh] from comment #20) > Created attachment 8738790 [details] > history-mockup.png > > (In reply to :Gijs Kruitbosch from comment #18) > > > I'm afraid must insist about the left padding in the history and bookmarks > > > panel: as you can see in the invision app, if you scroll down these are > > > Windows prototypes and they have the huge padding too. > > > > I think this is a mistake. It doesn't fit with in with the styling of the > > main menus or the rest of the OS or Firefox UI. Please can we reconsider? > > Gijs, to be clear, you are talking about the space between the left of the > panel and the start of the elements - ie, the space shown in the following > attachment (which is taken from Stephen's mockup of the History panel), > right? > > It's roughly a 19 pixel gap, which isn't really huge, and IMO is quite an > improvement over the status-quo, but I want to make sure that is what we are > asking Stephen to reconsider. The spacing in the spec for the bookmarks menu is bigger, for some reason (?) - 23-24px, by my measurement. The spacing caused by the patch is bigger still. In any case, the issue I'm asking to reconsider is that currently, on Windows, the spacing is more like 13-14px, because there isn't space for a checkbox *and* an icon. OS X has a "gutter" for such items which extends all the way down, next to the icons. On Windows, the gutter is shared between the icons and checkboxes. This is very visible in main menus on e.g. Windows 8. Stephen said as much in comment 6: (In reply to Stephen Horlander [:shorlander] from comment #6) > In OS X checkmarks appear in a gutter next to the menu items but on > Windows the checkmark and any icons occupy the same space It seems surprising that we would try to erase this difference with this patch. That's what I was asking him about.

(In reply to :Gijs Kruitbosch from comment #21) > (In reply to Stephen Horlander [:shorlander] from comment #6) > > In OS X checkmarks appear in a gutter next to the menu items but on > > Windows the checkmark and any icons occupy the same space > > It seems surprising that we would try to erase this difference with this > patch. That's what I was asking him about. Oh, I wasn't suggesting we change that particular platform difference. But I didn't think the margin difference was the huge :) I am ok with tightening it up for Windows. I will update the spec. Will also look into the spacing difference…

(In reply to Stephen Horlander [:shorlander] from comment #22) > I am ok with tightening it up for Windows. I will update the spec. Will also > look into the spacing difference… Adding ni? for Stephen so it doesn't fall off the radar...

Is this still relevant with photon ?

(In reply to Tim Nguyen :ntim from comment #24) > Is this still relevant with photon ? No, it's not - thanks!