1185482 - In maximized windows, the hover effect of the caption buttons should span the entire height of the tab strip

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: hover.png

There's currently a ~3px gap between the caption buttons and the toolbar. We should fill that by just extending the box of the hover effect (see mock)

Comment 1

[Dão Gottwald [:dao]]

Should be fixed now.

Comment 2

[Dão Gottwald [:dao]]

Actually it looks like bug 1355764 fixed this only in non-maximized windows...

Comment 4

[UK92]

Attachment: Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

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

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review151742

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

I'd suggest rearranging this as follows:

if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
  if (!menuHeight) {
    $("titlebar-buttonbox").style.height = fullTabsHeight + "px";
    titlebarContentHeight = rect(titlebarContent).height;
  } else {
    $("titlebar-buttonbox").style.removeProperty("height");
  }
}

Also, setting style.height would invalidate layout and then rect(titlebarContent) would flush layout, which is not ideal performance-wise. See the comments earlier in this code:

// Try to avoid reflows in this code by calculating dimensions first and
// then later set the properties affecting layout together in a batch.

...

// Begin setting CSS properties which will cause a reflow


How about:

titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight);

Comment 6

[UK92]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

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

Comment 7

[UK92]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review151742

> I'd suggest rearranging this as follows:
> 
> if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
>   if (!menuHeight) {
>     $("titlebar-buttonbox").style.height = fullTabsHeight + "px";
>     titlebarContentHeight = rect(titlebarContent).height;
>   } else {
>     $("titlebar-buttonbox").style.removeProperty("height");
>   }
> }
> 
> Also, setting style.height would invalidate layout and then rect(titlebarContent) would flush layout, which is not ideal performance-wise. See the comments earlier in this code:
> 
> // Try to avoid reflows in this code by calculating dimensions first and
> // then later set the properties affecting layout together in a batch.
> 
> ...
> 
> // Begin setting CSS properties which will cause a reflow
> 
> 
> How about:
> 
> titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight);

I moved code before `titlebarContentHeight`, to avoid double calculations. and also i did change to `min-height`, because in normal window, titlebar buttons icons have shrunk with `height`.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review151972

::: browser/base/content/browser-tabsintitlebar.js:176
(Diff revision 2)
>          fullMenuHeight = verticalMargins(menuStyles) + menuHeight;
>        }
>  
> +#ifdef MOZ_PHOTON_THEME
> +      if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> +        if(!menuHeight) {

nit: add a space after 'if'

::: browser/base/content/browser-tabsintitlebar.js:177
(Diff revision 2)
>        }
>  
> +#ifdef MOZ_PHOTON_THEME
> +      if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> +        if(!menuHeight) {
> +          $("titlebar-buttonbox").style.minHeight = fullTabsHeight + "px";

Setting style.minHeight invalidates layout, and the following rect(titlebarContent) call will flush layout again. That's exactly the performance pitfall we should avoid.

Did my titlebarContentHeight = Math.max(titlebarContentHeight, fullTabsHeight); suggestion not work out?

Comment 9

[UK92]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

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

Comment 10

[UK92]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review152002

::: browser/base/content/browser-tabsintitlebar.js:176
(Diff revision 3)
>          fullMenuHeight = verticalMargins(menuStyles) + menuHeight;
>        }
>  
> +#ifdef MOZ_PHOTON_THEME
> +      if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> +        if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {

Now only set height when window is maximized, is this better?

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review152140

::: browser/base/content/browser-tabsintitlebar.js:177
(Diff revisions 2 - 3)
>        }
>  
>  #ifdef MOZ_PHOTON_THEME
>        if (AppConstants.isPlatformAndVersionAtLeast("win", "10.0")) {
> -        if(!menuHeight) {
> -          $("titlebar-buttonbox").style.minHeight = fullTabsHeight + "px";
> +        if (!menuHeight && window.windowState == window.STATE_MAXIMIZED) {
> +          $("titlebar-buttonbox").style.height = fullTabsHeight + "px";

This needs to happen after the "// Begin setting CSS properties" comment below. See my previous remark about invalidating layout.

Comment 12

[UK92]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

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

Comment 13

[Dão Gottwald [:dao]]

(In reply to UK92 from comment #7)
> and also i did change to `min-height`, because in normal window, titlebar
> buttons icons have shrunk with `height`.

Is this not an issue anymore with your latest patch?

Comment 14

[UK92]

No, it's fixed with `Math.max(titlebarContentHeight, fullTabsHeight)`.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8875929 [details]
Bug 1185482 - Remove gap between titlebar buttons and navigation toolbar

https://reviewboard.mozilla.org/r/147328/#review152148

Looks good. Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42f53d05f6f1
Remove gap between titlebar buttons and navigation toolbar r=dao

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f16a9ed93a6
Backed out changeset 42f53d05f6f1 for breaking eslint because it tries to validate a file which needs preprocessing. r=backout on a CLOSED TREE

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/806cf3746d11
Remove gap between titlebar buttons and navigation toolbar r=dao

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/806cf3746d11

Comment 20

[Rodrigo]

I think this patch broke the minimize button when the window is not maximized. Also the close button looks weird compared to other Windows apps. See: https://i.imgur.com/g3IZBQq.png (Today's Nightly vs Paint, both not maximized)

Comment 21

[u596834]

This patch also created a small space at the top of the tabs and because of that you can't switch between them when you click at the top:

https://puu.sh/whTSU/d1a831cffb.gif (BAD)

https://puu.sh/whU0q/fdd9ae580f.gif (GOOD)

Comment 22

[Brindusa Tot, Desktop QA]

Verified as fixed on latest Nightly version 56.0a1, Build ID 20170704030203 on Windows 10 and Mac OS X 10.12.