Closed Bug 1185482 Opened 9 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect, P1)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- verified

People

(Reporter: phlsa, Assigned: urbankrajnc92)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-visual][p3][57])

Attachments

(2 files)

Attached image 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)
Depends on: 1173731
Depends on: 1355764
No longer depends on: 1173725, 1173731
Whiteboard: [photon-visual][57]
Version: 40 Branch → Trunk
Priority: -- → P3
Blocks: photon-tabs
No longer blocks: photon-visual
Flags: qe-verify+
Priority: P3 → P2
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][57] → [photon-visual][p3][57]
QA Contact: ovidiu.boca → brindusa.tot
Whiteboard: [photon-visual][p3][57] → [photon-visual][p3]
Should be fixed now.
Assignee: nobody → dao+bmo
Whiteboard: [photon-visual][p3] → [photon-visual][p3] fixed by bug 1355764
Whiteboard: [photon-visual][p3] fixed by bug 1355764 → [photon-visual][p3][57] fixed by bug 1355764
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Actually it looks like bug 1355764 fixed this only in non-maximized windows...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [photon-visual][p3][57] fixed by bug 1355764 → [photon-visual][p3][57]
Depends on: 1370989
No longer depends on: 1370989
Assignee: dao+bmo → urbankrajnc92
Status: REOPENED → ASSIGNED
Iteration: 55.7 - Jun 12 → ---
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 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 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 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 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.
Attachment #8875929 - Flags: review?(dao+bmo)
(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?
No, it's fixed with `Math.max(titlebarContentHeight, fullTabsHeight)`.
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!
Attachment #8875929 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42f53d05f6f1
Remove gap between titlebar buttons and navigation toolbar r=dao
Backout by archaeopteryx@coole-files.de:
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
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/806cf3746d11
Remove gap between titlebar buttons and navigation toolbar r=dao
https://hg.mozilla.org/mozilla-central/rev/806cf3746d11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.7 - Jun 12
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)
Depends on: 1372043
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)
Depends on: 1372289
Depends on: 1375335
Verified as fixed on latest Nightly version 56.0a1, Build ID 20170704030203 on Windows 10 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
No longer depends on: 1390237
You need to log in before you can comment on or make changes to this bug.