Closed Bug 1352358 Opened 4 years ago Closed 4 years ago

Implement compact and touch theme modes for the navigation toolbar

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: johannh)

References

Details

(Whiteboard: [photon-visual][p1])

Attachments

(1 file)

Bug 1352358 - Implement compact and touch theme modes for the navigation toolbar.
59 bytes, text/x-review-board-request
dao
: review+
Details 
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.
Priority: -- → P2
Priority: P2 → P1
Whiteboard: [photon] → [photon-visual]
Depends on: 1352364
No longer blocks: photon-touch
Flags: qe-verify-
Priority: P1 → P2
Whiteboard: [photon-visual] → [photon-visual][p1]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
Iteration: 55.4 - May 1 → 55.5 - May 15
Summary: Implement compact and touch theme modes → Implement compact and touch theme modes for the navigation toolbar
Depends on: 1363840
Iteration: 55.5 - May 15 → 55.6 - May 29
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?
Attachment #8868962 - Flags: review?(dao+bmo) → review-
Blocks: 1365912
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?
Attachment #8868962 - Flags: review?(dao+bmo) → review-
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...
Attachment #8868962 - Flags: review?(dao+bmo) → review-
(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.
Depends on: 1365901
Depends on: 1366550
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?
Flags: needinfo?(jhofmann)
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.
Flags: needinfo?(jhofmann)
Blocks: 1367284
No longer blocks: 1367284
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.
Attachment #8868962 - Flags: review?(dao+bmo) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c2f5bb3e04cb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Depends on: 1369690
(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.
Depends on: 1372846
No longer depends on: 1372846
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.
Depends on: 1388457
You need to log in before you can comment on or make changes to this bug.