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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
2 years ago
3 days ago

People

(Reporter: phlsa, Assigned: UK92)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

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

MozReview Requests

()

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

Attachments

(2 attachments)

Created attachment 8635980 [details]
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)

Updated

2 years ago
Depends on: 1173731

Updated

4 months ago
Blocks: 1325171
Depends on: 1355764
No longer depends on: 1173725, 1173731
Whiteboard: [photon-visual][57]
Version: 40 Branch → Trunk

Updated

4 months ago
Priority: -- → P3

Updated

4 months ago
Blocks: 1355767
No longer blocks: 1325171

Updated

4 months ago
Flags: qe-verify+
Priority: P3 → P2
QA Contact: ovidiu.boca

Updated

4 months ago
Whiteboard: [photon-visual][57] → [photon-visual][p3][57]

Updated

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

Updated

3 months ago
Whiteboard: [photon-visual][p3][57] → [photon-visual][p3]

Comment 1

3 months ago
Should be fixed now.
Assignee: nobody → dao+bmo
Whiteboard: [photon-visual][p3] → [photon-visual][p3] fixed by bug 1355764

Updated

3 months ago
Whiteboard: [photon-visual][p3] fixed by bug 1355764 → [photon-visual][p3][57] fixed by bug 1355764

Updated

3 months ago
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED

Updated

3 months ago
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1

Comment 2

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

Updated

2 months ago
Depends on: 1370989

Updated

2 months ago
No longer depends on: 1370989

Updated

2 months ago
Duplicate of this bug: 1370989

Updated

2 months ago
Assignee: dao+bmo → urbankrajnc92

Updated

2 months ago
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request)

Updated

2 months ago
Iteration: 55.7 - Jun 12 → ---

Comment 5

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

Comment 7

2 months ago
mozreview-review-reply
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

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

Comment 10

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

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

Comment 14

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

Comment 15

2 months ago
mozreview-review
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+

Comment 16

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42f53d05f6f1
Remove gap between titlebar buttons and navigation toolbar r=dao

Comment 17

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

Comment 18

2 months ago
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
Last Resolved: 3 months ago2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Iteration: --- → 55.7 - Jun 12

Comment 20

2 months ago
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)

Updated

2 months ago
Depends on: 1372043

Comment 21

2 months ago
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)

Updated

2 months ago
Depends on: 1372289

Updated

2 months ago
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
status-firefox55: fixed → verified
Flags: qe-verify+
Depends on: 1390237
No longer depends on: 1390237
You need to log in before you can comment on or make changes to this bug.