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)

40 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox40 + verified
firefox41 --- verified
firefox42 --- verified

People

(Reporter: phlsa, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 2 obsolete files)

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)
Tracking 40 as Philipp flagged this as a bug that we need for Windows 10 support in this release.
Flags: needinfo?(shorlander)
Attached patch The first cut at the patch. (obsolete) — Splinter Review
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)
Well, I got a kinda-blurry video of the differences, at least: http://youtu.be/PHwNEYTfY_Q
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-
Attached patch The second cut at the patch. (obsolete) — Splinter Review
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 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+
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 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+
Keywords: checkin-needed
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?
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)
(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)
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)
(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)
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!
(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)
Flags: qe-verify+
Verified fixed on Beta 40.0b9 (20150730171029) as well, using Windows 10 Pro x64 (10240).
QA Whiteboard: needs verification on 40 Beta 9
Flags: qe-verify+
Depends on: 1190462
You need to log in before you can comment on or make changes to this bug.