Closed Bug 1375335 Opened 7 years ago Closed 7 years ago

Menu bar is too high

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: Oriol, Assigned: johannh)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [photon-visual][p2])

Attachments

(3 files)

bad.png
10.42 KB, image/png
Details
good.png
10.42 KB, image/png
Details
Bug 1375335 - Fix window control height calculation on Windows 10.
59 bytes, text/x-review-board-request
dao
: review+
Details
Attached image bad.png — 
Press Alt key to show the menu bar.

After bug 1185482, it's too high.
Attached image good.png — 

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

    
    

    
  
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)

    
    

    
  
Yup, makes sense.

          status-firefox56:
          affected → fix-optional
Flags: needinfo?(jhofmann)

    
  
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1

    
    

    
      
  


  

    
    

    
  
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?
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1

    
    

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

    
    

    
      
  

    
    

    
  
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?

    
    

    
      
  

    
    

    
  
(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.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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 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?

    
    

    
  
(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?
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Blocks: 1387273

    
  
Blocks: 1387173

    
    

    
  
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 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)

    
    

    
  
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 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+

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/739d6afc27be
Fix window control height calculation on Windows 10. r=dao

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
      
  

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fdc6247f8fca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

    
    

    
  
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)

    
  
Depends on: 1390448

    
  
No longer blocks: 1390246
Depends on: 1390246
QA Contact: brindusa.tot → ovidiu.boca

    
  
Depends on: 1391209

    
    

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

    
    

    
  
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)

    
    

    
  
(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)

    
  
Depends on: 1398395

    
    

    
  
(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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: