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

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: phlsa, Assigned: UK92)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 55
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed)

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

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

Updated

2 months ago
Priority: -- → P3

Updated

2 months ago
Blocks: 1355767
No longer blocks: 1325171

Updated

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

Updated

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

Updated

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

Updated

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

Updated

23 days ago
Whiteboard: [photon-visual][p3] fixed by bug 1355764 → [photon-visual][p3][57] fixed by bug 1355764

Updated

23 days ago
Status: NEW → RESOLVED
Last Resolved: 23 days ago
Resolution: --- → FIXED

Updated

23 days ago
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]

Updated

17 days ago
Depends on: 1370989

Updated

17 days ago
No longer depends on: 1370989

Updated

17 days ago
Duplicate of this bug: 1370989

Updated

16 days ago
Assignee: dao+bmo → urbankrajnc92

Updated

16 days ago
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request)

Updated

16 days ago
Iteration: 55.7 - Jun 12 → ---

Comment 5

16 days 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

15 days 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

15 days 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

15 days 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

14 days 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

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

Comment 15

14 days 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

14 days 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

14 days 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

14 days 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: 23 days ago14 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

13 days ago
Iteration: --- → 55.7 - Jun 12

Comment 20

13 days 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

13 days ago
Depends on: 1372043

Comment 21

12 days 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

12 days ago
Depends on: 1372289

Updated

3 days ago
Depends on: 1375335
You need to log in before you can comment on or make changes to this bug.