Menu bar is too high

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
4 months ago
a month ago

People

(Reporter: Oriol, Assigned: johannh)

Tracking

(Depends on: 1 bug, {regression})

unspecified
Firefox 57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

4 months ago
Created attachment 8880210 [details]
bad.png

Press Alt key to show the menu bar.

After bug 1185482, it's too high.
(Reporter)

Comment 1

4 months ago
Created attachment 8880211 [details]
good.png
(Assignee)

Updated

4 months ago
Whiteboard: [photon-visual][triage]

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]

Updated

4 months ago
Whiteboard: [photon-visual] → [photon-visual][p2]
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox56: --- → affected
The regressing change is behind MOZ_PHOTON_THEME, so it will only affect 57, right? Can I mark it fix-optional for 56?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 3

4 months ago
Yup, makes sense.
status-firefox56: affected → fix-optional
Flags: needinfo?(jhofmann)
(Assignee)

Updated

3 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

Updated

3 months ago
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 5

3 months ago
mozreview-review
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?

Updated

3 months ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
(Assignee)

Comment 6

3 months ago
Yeah, it actually slightly squeezed the window controls vertically. I think we need to re-flush and calculate the dimensions again then.
Comment hidden (mozreview-request)

Comment 8

3 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 10

3 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

3 months ago
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

3 months ago
mozreview-review
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?
(Assignee)

Comment 15

3 months ago
(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?

Updated

3 months ago
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15

Updated

3 months ago
Blocks: 1387273
(Assignee)

Updated

3 months ago
Blocks: 1387173

Comment 16

3 months ago
mozreview-review
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.
Attachment #8889430 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 18

2 months ago
mozreview-review
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) {
Attachment #8889430 - Flags: review?(dao+bmo)
(Assignee)

Comment 19

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 21

2 months ago
mozreview-review
Comment on attachment 8889430 [details]
Bug 1375335 - Fix window control height calculation on Windows 10.

https://reviewboard.mozilla.org/r/160488/#review172824
Attachment #8889430 - Flags: review?(dao+bmo) → review+

Comment 22

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/739d6afc27be
Fix window control height calculation on Windows 10. r=dao
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
Flags: needinfo?(jhofmann)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c6f7d4e3ef24b685c884268b03b9b992ffe0d5
(Assignee)

Comment 26

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb5db7e5b410cb45cef054d5cbec41048211f75
Comment hidden (mozreview-request)

Comment 28

2 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37ba4f932f57
Fix window control height calculation on Windows 10. r=dao
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
(Assignee)

Comment 30

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5e741cc4ca897ef125ab3e807bc009cd540ade6
Comment hidden (mozreview-request)

Comment 32

2 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fdc6247f8fca
Fix window control height calculation on Windows 10. r=dao

Updated

2 months ago
Blocks: 1390246

Comment 33

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdc6247f8fca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 34

2 months ago
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!
Flags: needinfo?(jhofmann)
(Assignee)

Updated

2 months ago
Depends on: 1390448
(Assignee)

Updated

2 months ago
No longer blocks: 1390246
Depends on: 1390246
status-firefox56: fix-optional → disabled
status-firefox-esr52: --- → unaffected
Blocks: 1390468
Depends on: 1390796

Updated

2 months ago
QA Contact: brindusa.tot → ovidiu.boca

Updated

2 months ago
Depends on: 1391209

Comment 35

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

Comment 36

2 months ago
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
Flags: needinfo?(jhofmann)
(Assignee)

Comment 37

a month ago
(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).
Flags: needinfo?(jhofmann)
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.
Flags: needinfo?(jhofmann)
(Reporter)

Updated

a month ago
Depends on: 1398395
Depends on: 1392076
No longer depends on: 1392076
(Assignee)

Comment 39

a month ago
(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 :)
Flags: needinfo?(jhofmann)
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
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.