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)
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | verified |
People
(Reporter: phlsa, Assigned: urbankrajnc92)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-visual][p3][57])
Attachments
(2 files)
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•7 years ago
|
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify+
Priority: P3 → P2
QA Contact: ovidiu.boca
Updated•7 years ago
|
Whiteboard: [photon-visual][57] → [photon-visual][p3][57]
Updated•7 years ago
|
QA Contact: ovidiu.boca → brindusa.tot
Updated•7 years ago
|
Whiteboard: [photon-visual][p3][57] → [photon-visual][p3]
Comment 1•7 years ago
|
||
Should be fixed now.
Assignee: nobody → dao+bmo
Whiteboard: [photon-visual][p3] → [photon-visual][p3] fixed by bug 1355764
Updated•7 years ago
|
Whiteboard: [photon-visual][p3] fixed by bug 1355764 → [photon-visual][p3][57] fixed by bug 1355764
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Comment 2•7 years 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•7 years ago
|
Assignee: dao+bmo → urbankrajnc92
Updated•7 years ago
|
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Iteration: 55.7 - Jun 12 → ---
Comment 5•7 years 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) |
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•7 years 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•7 years 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•7 years 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) |
Comment 13•7 years ago
|
||
(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•7 years ago
|
||
No, it's fixed with `Math.max(titlebarContentHeight, fullTabsHeight)`.
Comment 15•7 years 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•7 years 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•7 years 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•7 years 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
![]() |
||
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/806cf3746d11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Iteration: --- → 55.7 - Jun 12
Comment 20•7 years 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)
Comment 21•7 years 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)
Comment 22•7 years ago
|
||
Verified as fixed on latest Nightly version 56.0a1, Build ID 20170704030203 on Windows 10 and Mac OS X 10.12.
You need to log in
before you can comment on or make changes to this bug.
Description
•