1375335 - Menu bar is too high

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Oriol Brufau [:Oriol]]

Attachment: bad.png

Press Alt key to show the menu bar.

After bug 1185482, it's too high.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: good.png

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

The regressing change is behind MOZ_PHOTON_THEME, so it will only affect 57, right? Can I mark it fix-optional for 56?

Comment 3

[Johann Hofmann [:johannh]]

Yup, makes sense.

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1375335 - Fix window control height calculation on Windows 10.

Resetting this height after calculating dimensions causes the
menu items to end up too high and makes the window controls move
upward when showing the menu bar. Keeping it around is probably
the simplest solution.

Review commit: https://reviewboard.mozilla.org/r/160488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160488/

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review166182

::: browser/base/content/browser-tabsintitlebar.js
(Diff revision 1)
>            AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
>          if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
>            titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight);
>            $("titlebar-buttonbox").style.height = titlebarContentHeight + "px";
> -        } else {
> -          $("titlebar-buttonbox").style.removeProperty("height");

Won't this leave the title bar buttons use too much height when going from maximized to restored, as opposed to a window being restored without having been maximized first?

Comment 6

[Johann Hofmann [:johannh]]

Yeah, it actually slightly squeezed the window controls vertically. I think we need to re-flush and calculate the dimensions again then.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/1-2/

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review166314

::: browser/base/content/browser-tabsintitlebar.js:186
(Diff revision 2)
>          if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
>            titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight);
>            $("titlebar-buttonbox").style.height = titlebarContentHeight + "px";
>          } else {
>            $("titlebar-buttonbox").style.removeProperty("height");
> +          titlebarContentHeight = rect(titlebarContent).height;

How about we remove the height property before we calculate all the dimensions?

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/2-3/

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8889430 [details]
> Bug 1375335 - Re-calculate titlebar dimensions after removing custom height.
> 
> https://reviewboard.mozilla.org/r/160488/#review166314
> 
> ::: browser/base/content/browser-tabsintitlebar.js:186
> (Diff revision 2)
> >          if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
> >            titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight);
> >            $("titlebar-buttonbox").style.height = titlebarContentHeight + "px";
> >          } else {
> >            $("titlebar-buttonbox").style.removeProperty("height");
> > +          titlebarContentHeight = rect(titlebarContent).height;
> 
> How about we remove the height property before we calculate all the
> dimensions?

Yes, I considered that, but we need the menuHeight before we know if we can remove it. In the latest patch I only re-calculate if we actually changed something, I hope that's better.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/3-4/

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/4-5/

Comment 13

[Johann Hofmann [:johannh]]

Ok, another approach:

This one always sets the height, even in non-maximized mode and adjusts the padding to avoid distorting the icons. This has the great advantage that the window controls have a consistent size and no longer "jump" when showing the menubar in maximized mode.

What do you think?

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review167280

::: browser/base/content/browser-tabsintitlebar.js:183
(Diff revision 5)
>  
> +      // On Windows 10, adjust the window controls to span the entire
> +      // tab strip height if we're not showing a menu bar.
>        if (AppConstants.MOZ_PHOTON_THEME &&
>            AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> -        if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
> +        if (!menuHeight) {

Does this not regress bug 1372043?

Comment 15

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8889430 [details]
> Bug 1375335 - Always set custom titlebar buttonbox height.
> 
> https://reviewboard.mozilla.org/r/160488/#review167280
> 
> ::: browser/base/content/browser-tabsintitlebar.js:183
> (Diff revision 5)
> >  
> > +      // On Windows 10, adjust the window controls to span the entire
> > +      // tab strip height if we're not showing a menu bar.
> >        if (AppConstants.MOZ_PHOTON_THEME &&
> >            AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> > -        if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
> > +        if (!menuHeight) {
> 
> Does this not regress bug 1372043?

Oh, uh, not for me. What was causing bug 1372043? Always setting the height without adjusting the padding sounds broken, is that it maybe?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review170358

I believe johann is working on a new patch.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/5-6/

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review171856

::: commit-message-5928d:18
(Diff revision 6)
> +  which could have been handled in browser-tabsintitlebar.js, but since
> +  it's not part of the Photon spec we decide to remove it entirely.
> +
> +- Makes window control height calculations ignore vertical tabs toolbar
> +  margins. The only margin it has right now is -1px and the calculation
> +  code doesn't work right with negative margins.

Where does it fail? At a first glance it seems correct to me that we should calculate with the tab height -1 for the negative bottom margin.

::: browser/base/content/browser-tabsintitlebar.js:148
(Diff revision 6)
>      let menubar = $("toolbar-menubar");
>  
>      if (allowed) {
> +
> +      // Reset the custom titlebar height if the menubar is shown,
> +      // because we will want to calculate its original height.

I'd put this after the "We set the tabsintitlebar attribute first..." block.

::: browser/themes/windows/browser-aero.css:8
(Diff revision 6)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %define glassActiveBorderColor rgb(37, 44, 51)
>  %define glassInactiveBorderColor rgb(102, 102, 102)
>  
> -@media not all and (-moz-windows-classic) {
> +@media (-moz-os-version: windows-win7) {

Presumably this was intentionally excluding classic and should become:

@media (-moz-os-version: windows-win7) {
 @media not all and (-moz-windows-classic) {

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review171856

> Where does it fail? At a first glance it seems correct to me that we should calculate with the tab height -1 for the negative bottom margin.

The tab bar uses the negative margin to overlay the nav bar border, and the window controls need to do the same to avoid having a visible 0.8px bottom margin to the nav bar. The visible height of the tab bar doesn't change. You can see it if you compare a current Windows Nightly in maximized mode to my version.

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/6-7/

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review172824

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/739d6afc27be
Fix window control height calculation on Windows 10. r=dao

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This seems to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=122611167&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/33d0cd5c672aff559d00edfb38d7bc9408599bca

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/7-8/

Comment 25

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c6f7d4e3ef24b685c884268b03b9b992ffe0d5

Comment 26

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb5db7e5b410cb45cef054d5cbec41048211f75

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/8-9/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37ba4f932f57
Fix window control height calculation on Windows 10. r=dao

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_windowopen_reflows.js on OS X:

https://hg.mozilla.org/integration/autoland/rev/c218d170bd314525d9e0e83090a8668753a23ed2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=37ba4f932f57fc60bf27554b5d36df3c721df75f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122973911&repo=autoland

04:44:57     INFO - TEST-PASS | browser/base/content/test/performance/browser_windowopen_reflows.js | expected uninterruptible reflow: '[
04:44:57     INFO - 	"select@chrome://global/content/bindings/textbox.xml:115:11",
04:44:57     INFO - 	"focusAndSelectUrlBar@chrome://browser/content/browser.js:2236:5",
04:44:57     INFO - 	"_delayedStartup@chrome://browser/content/browser.js:1540:10",
04:44:57     INFO - 	"EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1372:5",
04:44:57     INFO - 	"onload@chrome://browser/content/browser.xul:1:1",
04:44:57     INFO - 	""
04:44:57     INFO - ]' - true == true - 
04:44:57     INFO - Buffered messages finished
04:44:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen_reflows.js | Unused expected reflow: [
04:44:57     INFO - 	"verticalMargins@chrome://browser/content/browser-tabsintitlebar.js",
04:44:57     INFO - 	"_update@chrome://browser/content/browser-tabsintitlebar.js",
04:44:57     INFO - 	"init@chrome://browser/content/browser-tabsintitlebar.js",
04:44:57     INFO - 	"handleEvent@chrome://browser/content/tabbrowser.xml"
04:44:57     INFO - ]
04:44:57     INFO - This reflow was supposed to be hit 2 more time(s).
04:44:57     INFO - This is probably a good thing - just remove it from the expected list. - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: withReflowObserver :: line 131
04:44:57     INFO - Stack trace:
04:44:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:withReflowObserver:131

Comment 30

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5e741cc4ca897ef125ab3e807bc009cd540ade6

Comment 31

[Johann Hofmann [:johannh]]

Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160488/diff/9-10/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdc6247f8fca
Fix window control height calculation on Windows 10. r=dao

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fdc6247f8fca

Comment 34

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=824d4f269c6323e1ad2bd8ebeb6496d60b8ba3e5&newProject=mozilla-central&newRev=92f3de33d97f55d54b2baad585b87e76aaa5ec58

Unfortunately it seems like we moved the tab bar up by 1px on Mac, which also caused bug 1390246. It's quite weird, since I had thoroughly checked this on my Mac and I'm surprised that we reduced the height (not regarding the -1px vertical margin should add 1px to the previous number). It might be because we're not taking the maximum of either menubar height or tab height anymore.

Anyway, I'll file a new bug and investigate. All other platforms look great!

Comment 35

[Jukka Jylänki]

It looks like this has regressed WebGL rendering in an odd manner: see bug 1392076.

Comment 36

[Deac Alin]

Hi Johann
Can you please advise about what needs to be verified(QA) for this bug? As the attachments screenshots are obsolete on latest Nightly. Thank you

Comment 37

[Johann Hofmann [:johannh]]

(In reply to Deac Alin from comment #36)
> Hi Johann
> Can you please advise about what needs to be verified(QA) for this bug? As
> the attachments screenshots are obsolete on latest Nightly. Thank you

Hi,

the menu bar should be positioned at the top of the window, not higher, in a maximized browser window on Windows 10. Additionally, the window controls on Windows 10 (and all other platforms, for that matter) should conform to the Photon spec (http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html).

Comment 38

[Brindusa Tot, Desktop QA]

Hi,
I looked over the Menu bar on an older Nightly build 56.0a1 from 07/07/2017 and on latest Nightly 57.0a1 from 09/08/2017, and I observe the followings:
 - on an older build, the height of the menu bar buttons is 20px
 - on the latest Nightly the height of the menu bar buttons is 24px.

I am not sure if this is the correct height since I did not find any specific documentation for menu bar into http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html or on https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/237801528.

Johann, please confirm if this is the correct value, or if you have other specs please link them here.

Comment 39

[Johann Hofmann [:johannh]]

(In reply to Brindusa Tot[:brindusat] from comment #38)
> Hi,
> I looked over the Menu bar on an older Nightly build 56.0a1 from 07/07/2017
> and on latest Nightly 57.0a1 from 09/08/2017, and I observe the followings:
>  - on an older build, the height of the menu bar buttons is 20px
>  - on the latest Nightly the height of the menu bar buttons is 24px.
> 
> I am not sure if this is the correct height since I did not find any
> specific documentation for menu bar into
> http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html
> or on https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/237801528.
> 
> Johann, please confirm if this is the correct value, or if you have other
> specs please link them here.

Yeah I don't think it's exactly specified, the point of this bug is that the menu bar is not oddly positioned, like too high so that you can't fully read the menu labels. If you can confirm that I think we're good :)

Comment 40

[Brindusa Tot, Desktop QA]

Thanks Johann. In this case, I can confirm them menu bar is nicely positioned, easily readable, on all Density options: Compact, Normal Touch.

Verified on latest Nightly 57.0a1, build ID 20170914100122