Closed
Bug 1187268
Opened 9 years ago
Closed 9 years ago
Remove the border around background tabs on hover on Windows 10
Categories
(Firefox :: Theme, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 42
People
(Reporter: phlsa, Assigned: bwinton)
References
(Blocks 1 open bug)
Details
Attachments
(7 files, 2 obsolete files)
256 bytes,
image/png
|
Details | |
75 bytes,
image/png
|
Details | |
257 bytes,
image/png
|
Details | |
400 bytes,
image/png
|
Details | |
86 bytes,
image/png
|
Details | |
417 bytes,
image/png
|
Details | |
9.81 KB,
patch
|
bwinton
:
review+
shorlander
:
ui-review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
To be consistent with the general appearance of tabs, we should remove the outline of the tab curve on background tabs (on hover) as well. NI Stephen for graphics
Flags: needinfo?(shorlander)
Comment 1•9 years ago
|
||
Tracking 40 as Philipp flagged this as a bug that we need for Windows 10 support in this release.
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
tracking-firefox40:
--- → +
Comment 2•9 years ago
|
||
Flags: needinfo?(shorlander)
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
I couldn't get a screenshot yet, but the build is available for UI-Review at https://www.dropbox.com/s/ay9inf3rnnn5690/firefox.zip?dl=0 The code is pretty simple, too, so it would be awesome if we could get it uplifted… :)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8639409 -
Flags: ui-review?(philipp)
Attachment #8639409 -
Flags: review?(jaws)
Assignee | ||
Comment 9•9 years ago
|
||
Well, I got a kinda-blurry video of the differences, at least: http://youtu.be/PHwNEYTfY_Q
Comment 10•9 years ago
|
||
Comment on attachment 8639409 [details] [diff] [review] The first cut at the patch. Review of attachment 8639409 [details] [diff] [review]: ----------------------------------------------------------------- I got the following error when trying to build this patch locally, 0:05.32 RuntimeError: File "tabbrowser/tab-background-start-preWin10.png" not found in c:\fx\browser\themes\windows, c:\fx\obj-i686-pc-mingw32\browser\themes\windows
Attachment #8639409 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Now with renamed and added files!
Attachment #8639409 -
Attachment is obsolete: true
Attachment #8639409 -
Flags: ui-review?(philipp)
Attachment #8639429 -
Flags: ui-review?(philipp)
Attachment #8639429 -
Flags: review?(jaws)
Comment 12•9 years ago
|
||
Comment on attachment 8639429 [details] [diff] [review] The second cut at the patch. Review of attachment 8639429 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/windows/jar.mn @@ +350,5 @@ > + skin/classic/browser/tabbrowser/tab-background-start-preWin10@2x.png (tabbrowser/tab-background-start-preWin10@2x.png) > + skin/classic/browser/tabbrowser/tab-background-middle-preWin10.png (tabbrowser/tab-background-middle-preWin10.png) > + skin/classic/browser/tabbrowser/tab-background-middle-preWin10@2x.png (tabbrowser/tab-background-middle-preWin10@2x.png) > + skin/classic/browser/tabbrowser/tab-background-end-preWin10.png (tabbrowser/tab-background-end-preWin10.png) > + skin/classic/browser/tabbrowser/tab-background-end-preWin10@2x.png (tabbrowser/tab-background-end-preWin10@2x.png) Please adjust the whitespace so that there are proper columns for these changes (here and below).
Attachment #8639429 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 13•9 years ago
|
||
The reviewed version of the patch, with things lined up, and the r+ carried forward.
Attachment #8639429 -
Attachment is obsolete: true
Attachment #8639429 -
Flags: ui-review?(philipp)
Attachment #8639459 -
Flags: ui-review?(shorlander)
Attachment #8639459 -
Flags: ui-review?(philipp)
Attachment #8639459 -
Flags: review+
Comment 14•9 years ago
|
||
Comment on attachment 8639459 [details] [diff] [review] The patch with things lined up. Review of attachment 8639459 [details] [diff] [review]: ----------------------------------------------------------------- ui-r+ if it looks like the shaky cam video ;)
Attachment #8639459 -
Flags: ui-review?(shorlander) → ui-review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8639459 [details] [diff] [review] The patch with things lined up. Approval Request Comment [Feature/regressing bug #]: Windows 10 [User impact if declined]: Uglier background tabs on Windows 10. [Describe test coverage new/current, TreeHerder]: Manual testing on Windows 8 and 10. [Risks and why]: Reasonably low risk. Image swap only. [String/UUID change made/needed]: None.
Attachment #8639459 -
Flags: ui-review?(philipp)
Attachment #8639459 -
Flags: approval-mozilla-beta?
Attachment #8639459 -
Flags: approval-mozilla-aurora?
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e3d21ae44c1d
Keywords: checkin-needed
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e3d21ae44c1d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment on attachment 8639459 [details] [diff] [review] The patch with things lined up. Seems straightforward and simple. Approved for uplift to Aurora and Beta given that this was manually verified by dev.
Attachment #8639459 -
Flags: approval-mozilla-beta?
Attachment #8639459 -
Flags: approval-mozilla-beta+
Attachment #8639459 -
Flags: approval-mozilla-aurora?
Attachment #8639459 -
Flags: approval-mozilla-aurora+
Otilia, could you please verify this image swap on FF40 whenever you get a chance? Thanks!
Flags: qe-verify+
Flags: needinfo?(otilia.anica)
Comment 21•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #19) > Otilia, could you please verify this image swap on FF40 whenever you get a > chance? Thanks! We'll only be able to verify this on Fx 40.0 Beta 9, as the fix missed Beta 8. We did manage to confirm the fix for this issue on Nightly 42.0a1 (2015-07-29) and Aurora 41.0a2 (2015-07-29), using Windows 10 Pro x64 (Build 10240). Blake, I'm not sure if you noticed this, but Windows 8 and Windows 8.1 are still showing slightly visible borders (perhaps they're just shadows?) on the very same builds and for some reason, the text from the background tabs turned white - see http://i.imgur.com/q9jNaV6.png.
Flags: needinfo?(otilia.anica) → needinfo?(bwinton)
Assignee | ||
Comment 22•9 years ago
|
||
I believe the borders should be there (because they should only be removed for Windows 10 and up). I'm not sure what’s up with the background text, but I can’t see any way my patch could have changed that… (I'll dig into it this afternoon, and see what I can find.)
Flags: needinfo?(bwinton)
Assignee | ||
Comment 23•9 years ago
|
||
(Oh, can you give me a revision where the text wasn't white, just to make my debugging easier?)
Flags: needinfo?(rkothari)
Andrei, I think Blake's question was directed at you.
Flags: needinfo?(rkothari) → needinfo?(andrei.vaida)
Assignee | ||
Comment 26•9 years ago
|
||
So, I can't replicate it on Windows 7 or Windows 10, and nothing jumps out at me from the code, so, uh, I'm really going to need a regression range to try and fix it. (Also, I still suspect that it wasn't my patch, but I'm sure every developer says that. ;) Thank you!
Comment 27•9 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #26) > So, I can't replicate it on Windows 7 or Windows 10, and nothing jumps out > at me from the code, so, uh, I'm really going to need a regression range to > try and fix it. (Also, I still suspect that it wasn't my patch, but I'm > sure every developer says that. ;) > > Thank you! I wasn't really trying to point out that this issue was caused by this specific patch, but I thought it was something you or someone else might know about as I didn't find any bug on file for it. I've further investigated the problem and it turns out to be a configuration issue isolated to my test machine, so this is NOT a bug. Blake, thank you for looking into it and sorry for wasting your time! Since according to Comment 22 the borders should still be visible on Windows 8 and Windows 8.1, this bug is verified fixed. As previously mentioned, this patch will be verified on Fx 40 as soon as builds are available for Beta 9.
Status: RESOLVED → VERIFIED
QA Whiteboard: needs verification on 40 Beta 9
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Updated•9 years ago
|
Flags: qe-verify+
Comment 28•9 years ago
|
||
Verified fixed on Beta 40.0b9 (20150730171029) as well, using Windows 10 Pro x64 (10240).
You need to log in
before you can comment on or make changes to this bug.
Description
•