The idea is that besides the default mode, there would be a compact mode with reduced spacing around UI elements (much like current compact themes) and a touch mode with extra spacing.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review144022 ::: browser/themes/shared/browser.inc.css:10 (Diff revision 1) > +#main-window[density=compact] #urlbar, > +#main-window[density=compact] .searchbar-textbox { > + min-height: 26px; > + margin-top: 3px; > + margin-bottom: 3px; > +} This should be in urlbar-searchbar.inc.css (location-search-bar.inc.css prior to bug 1365683) ::: browser/themes/shared/toolbarbuttons.inc.css:47 (Diff revision 1) > --toolbarbutton-checkedhover-backgroundcolor: rgba(85%,85%,85%,.25); > } > > +/* Larger buttons in touch mode */ > + > +#main-window[density=touch] { nit 1: uidensity nit 2: use :root ::: browser/themes/shared/toolbarbuttons.inc.css:52 (Diff revision 1) > +#main-window[density=touch] { > + --toolbarbutton-width: 36px; > + --toolbarbutton-inner-padding: 9px; > +} > + > +#main-window[density=touch] #back-button { please move this to the other back button styling ::: browser/themes/shared/toolbarbuttons.inc.css:57 (Diff revision 1) > +#main-window[density=touch] #back-button { > + padding-top: 1px !important; > + padding-bottom: 1px !important; > +} > + > +#main-window[density=touch] #back-button > .toolbarbutton-icon { ditto ::: browser/themes/shared/toolbarbuttons.inc.css:199 (Diff revision 1) > } > > #nav-bar .toolbarbutton-1 > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > #nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { > - /* horizontal padding + border + actual icon width */ > + max-width: var(--toolbarbutton-width); Can this just use calc with var(--toolbarbutton-inner-padding)? ::: browser/themes/shared/toolbarbuttons.inc.css:229 (Diff revision 1) > } > > #nav-bar .toolbarbutton-1[type=panel] > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1[type=menu]:not(#PanelUI-menu-button):not(#back-button):not(#forward-button):not(#new-tab-button) > .toolbarbutton-icon { > /* horizontal padding + border + icon width */ > max-width: 43px; need to update this as well ::: browser/themes/shared/toolbarbuttons.inc.css:291 (Diff revision 1) > toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):not([open]), > #nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, > #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, > #nav-bar .toolbarbutton-1:not([disabled=true]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-icon, > +#nav-bar #back-button:not([disabled=true]):not([open]):not(:active):hover > .toolbarbutton-icon, What's this change about? ::: browser/themes/shared/toolbarbuttons.inc.css:385 (Diff revision 1) > > -#back-button:-moz-locale-dir(rtl) { > - border-radius: 10000px 0 0 10000px !important; > +#main-window:not([density=compact]) #back-button:-moz-locale-dir(rtl) { > + border-radius: 10000px 0 0 10000px; > } > > #back-button > menupopup { Is this needed for compact mode?

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review144022 > What's this change about? Yeah, I'm a bit unhappy about that. I needed to increase the specificity of that selector because adding #main-window[uidensity=compact] to the #back-button rules made it override the hover styles and adding !important here wouldn't work either because it conflicts with other toolbarbutton's background styles. I'm all ears if you have a better suggestion :) > Is this needed for compact mode? Yup, I'm not actually sure why the margin is e.g. -3px px on OSX. Taking this away would look weird.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review144570 ::: browser/themes/shared/location-search-bar.inc.css:51 (Diff revision 2) > +} > + > +#main-window[uidensity=touch] #urlbar, > +#main-window[uidensity=touch] .searchbar-textbox { > + min-height: 32px; > +} Please use :root rather than #main-window where possible. I believe it should work here. ::: browser/themes/shared/toolbarbuttons.inc.css:190 (Diff revision 2) > /* horizontal padding + border + actual icon width */ > - max-width: 32px; > +%ifdef MOZ_PHOTON_THEME > + max-width: calc(2 * var(--toolbarbutton-inner-padding) + 2px + 16px); > +%else > + /* Before Photon horizontal padding is 7px, but --toolbarbutton-inner-padding is set to 3px */ > + max-width: calc(32px); Why calc? ::: browser/themes/shared/toolbarbuttons.inc.css:224 (Diff revision 2) > #nav-bar .toolbarbutton-1[type=panel] > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1[type=menu]:not(#PanelUI-menu-button):not(#back-button):not(#forward-button):not(#new-tab-button) > .toolbarbutton-icon { > /* horizontal padding + border + icon width */ > max-width: 43px; > } > +%endif Don't do this. ::: browser/themes/shared/toolbarbuttons.inc.css:305 (Diff revision 2) > toolbarbutton.bookmark-item:not(.subviewbutton):hover:active:not([disabled="true"]), > toolbarbutton.bookmark-item[open="true"], > #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled=true]):-moz-any(:hover:active, [open]) > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1[open] > .toolbarbutton-menubutton-dropmarker:not([disabled=true]) > .dropmarker-icon, > #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon, > +#nav-bar #back-button:not([disabled=true]):-moz-any([open],:hover:active) > .toolbarbutton-icon, Can you drop this if you use :root further down? (see next comment) ::: browser/themes/shared/toolbarbuttons.inc.css:365 (Diff revision 2) > /* when not hovered anymore, trigger a new transition to hide the forward button immediately */ > margin-left: calc(-0.01px - var(--forwardbutton-width) - var(--backbutton-urlbar-overlap)); > } > %endif > > -#back-button { > +#main-window:not([uidensity=compact]) #back-button { Does :root work here? May have to restore some !important to make that work. ::: browser/themes/shared/toolbarbuttons.inc.css:395 (Diff revision 2) > + padding: 7px; > +} > + > +#main-window[uidensity=touch] #back-button { > + padding-top: 1px; > + padding-bottom: 1px; Why is this needed? Should the button automatically use the .toolbarbutton-1 padding in this case?

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review144570 > Why calc? Leftover. I wasn't sure if I should calculate all the components for clarity(calc(16px + 2px ...) but that seemed wasteful to me. > Can you drop this if you use :root further down? (see next comment) Neat, that actually worked! > Why is this needed? Should the button automatically use the .toolbarbutton-1 padding in this case? Default back-button padding is 3px, the interactive mockup says it's 1px in touch mode.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review144580 ::: browser/themes/shared/location-search-bar.inc.css:41 (Diff revision 3) > > #urlbar-container { > -moz-box-align: center; > } > + > +:root[uidensity=compact] #urlbar, Need to rebase. This is now browser/theme/shared/urlbar-searchbar.inc.css with photon-specific and non-photon-specific bits. ::: browser/themes/shared/toolbarbuttons.inc.css:44 (Diff revision 3) > --toolbarbutton-checkedhover-backgroundcolor: rgba(85%,85%,85%,.25); > } > > +/* Larger buttons in touch mode */ > + > +:root[uidensity=touch] { nit: move this up after the first rule for :root ::: browser/themes/shared/toolbarbuttons.inc.css:222 (Diff revision 3) > > #nav-bar .toolbarbutton-1[type=panel] > .toolbarbutton-icon, > #nav-bar .toolbarbutton-1[type=menu]:not(#PanelUI-menu-button):not(#back-button):not(#forward-button):not(#new-tab-button) > .toolbarbutton-icon { > - /* horizontal padding + border + icon width */ > - max-width: 43px; > +%ifdef MOZ_PHOTON_THEME > + /* horizontal padding + border + icon width + inner margin + dropmarker width */ > + max-width: calc(2 * var(--toolbarbutton-inner-padding) + 2px + 16px + 2px + 15px); I believe this is incorrect. Should be: /* start padding + borders + icon width + end padding */ max-width: calc(var(--toolbarbutton-inner-padding) + 2px + 16px + 17px); Bug 1365901 is getting rid of the border, maybe we should land that before this patch...

(In reply to Dão Gottwald [::dao] from comment #8) > > #nav-bar .toolbarbutton-1[type=panel] > .toolbarbutton-icon, > > #nav-bar .toolbarbutton-1[type=menu]:not(#PanelUI-menu-button):not(#back-button):not(#forward-button):not(#new-tab-button) > .toolbarbutton-icon { > > - /* horizontal padding + border + icon width */ > > - max-width: 43px; > > +%ifdef MOZ_PHOTON_THEME > > + /* horizontal padding + border + icon width + inner margin + dropmarker width */ > > + max-width: calc(2 * var(--toolbarbutton-inner-padding) + 2px + 16px + 2px + 15px); > > I believe this is incorrect. Should be: > > /* start padding + borders + icon width + end padding */ > max-width: calc(var(--toolbarbutton-inner-padding) + 2px + 16px + 17px); So, I think you were actually on the right track, except that the end padding doesn't currently depend on var(--toolbarbutton-inner-padding). It probably should.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review145462 ::: browser/themes/shared/toolbarbuttons.inc.css (Diff revision 4) > } > > -#nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > - /* horizontal padding + border + actual icon width */ > - max-width: 31px; > -} Why is this not needed anymore? ::: browser/themes/shared/toolbarbuttons.inc.css:228 (Diff revision 4) > padding-top: var(--toolbarbutton-vertical-text-padding); > padding-bottom: 0; > +%ifdef MOZ_PHOTON_THEME > + /* To make the hover feedback line up with sibling buttons, it needs the same > + * height (16px) + padding (2 * 6px), but as a minimum > + * because otherwise an increase in text sizes would break things. nit: "[...] it needs the same height as the icons and the same vertical padding, [...]"

(In reply to Dão Gottwald [::dao] from comment #11) > Comment on attachment 8868962 [details] > Bug 1352358 - Implement compact and touch theme modes for the navigation > toolbar. > > https://reviewboard.mozilla.org/r/140634/#review145462 > > ::: browser/themes/shared/toolbarbuttons.inc.css > (Diff revision 4) > > } > > > > -#nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > > - /* horizontal padding + border + actual icon width */ > > - max-width: 31px; > > -} > > Why is this not needed anymore?

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review145462 > Why is this not needed anymore? I'm pretty sure this is just a leftover we didn't clean up properly. It has no effect in neither Photon nor normal on any platform afaik.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review145936 ::: browser/themes/shared/toolbarbuttons.inc.css (Diff revision 6) > } > > -#nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > - /* horizontal padding + border + actual icon width */ > - max-width: 31px; > -} I think this might still be needed for non-photon, as this button has no end-border, hence a 1px smaller max-width...? ::: browser/themes/shared/urlbar-searchbar.inc.css:15 (Diff revision 6) > +} > + > +:root[uidensity=touch] #urlbar, > +:root[uidensity=touch] .searchbar-textbox { > + min-height: 32px; > +} These override the following #urlbar, .searchbar-textbox rule, which is a bit confusing. Please move this after the MOZ_PHOTON_THEME block.

Comment on attachment 8868962 [details] Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. https://reviewboard.mozilla.org/r/140634/#review145936 > I think this might still be needed for non-photon, as this button has no end-border, hence a 1px smaller max-width...? Ok, doesn't hurt to keep it.

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fff82bf54547 Implement compact and touch theme modes for the navigation toolbar. r=dao

Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7f77aec0e29a Backed out changeset fff82bf54547 for packaging bustage.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s fb19c4ed05d5 -d a6c98d7ddc3d: rebasing 398141:fb19c4ed05d5 "Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar. r=dao" (tip) merging browser/extensions/pocket/jar.mn merging browser/extensions/pocket/skin/linux/pocket.css merging browser/extensions/pocket/skin/osx/pocket.css merging browser/extensions/pocket/skin/windows/pocket.css merging browser/themes/osx/browser.css merging browser/themes/shared/toolbarbuttons.inc.css merging browser/themes/shared/urlbar-searchbar.inc.css warning: conflicts while merging browser/themes/shared/toolbarbuttons.inc.css! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c2f5bb3e04cb Implement compact and touch theme modes for the navigation toolbar. r=dao

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=c575ecde53030c41c12ba228d8fe78c3ed896e76&newProject=mozilla-central&newRev=d10f5ccd882b965fcad39914f7c3c930d1301a41 This seems to have caused the toolbarbuttons to have a tiny bit less horizontal spacing, not sure how. Inspecting the buttons yields the correct sizes so that's fine for me.

(In reply to Johann Hofmann [:johannh] from comment #26) > Screenshots: > > https://screenshots.mattn.ca/compare/?oldProject=mozilla- > central&oldRev=c575ecde53030c41c12ba228d8fe78c3ed896e76&newProject=mozilla- > central&newRev=d10f5ccd882b965fcad39914f7c3c930d1301a41 > > This seems to have caused the toolbarbuttons to have a tiny bit less > horizontal spacing, not sure how. Inspecting the buttons yields the correct > sizes so that's fine for me. It's only the pocket button that changed. It has the correct padding now.

(In reply to Dão Gottwald [::dao] from comment #27) > (In reply to Johann Hofmann [:johannh] from comment #26) > > Screenshots: > > > > https://screenshots.mattn.ca/compare/?oldProject=mozilla- > > central&oldRev=c575ecde53030c41c12ba228d8fe78c3ed896e76&newProject=mozilla- > > central&newRev=d10f5ccd882b965fcad39914f7c3c930d1301a41 > > > > This seems to have caused the toolbarbuttons to have a tiny bit less > > horizontal spacing, not sure how. Inspecting the buttons yields the correct > > sizes so that's fine for me. > > It's only the pocket button that changed. It has the correct padding now. Ah, true.

Sorry for asking, but are the compact theme buttons fully implemented now as per specs? Mockup is showing buttons with a bit higher density and I'm not seeing any open bugs for this. https://s2.postimg.org/50r63wnqx/compact_buttons.png

(In reply to Eddward from comment #29) > Sorry for asking, but are the compact theme buttons fully implemented now as > per specs? > Mockup is showing buttons with a bit higher density and I'm not seeing any > open bugs for this. > https://s2.postimg.org/50r63wnqx/compact_buttons.png Please file a new bug.