Closed Bug 1168528 Opened 4 years ago Closed 4 years ago

Tab close icons on Windows HiDPI are blurry

Categories

(Firefox :: Theme, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.2 - Jun 8
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

No description provided.
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch Patch (obsolete) — Splinter Review
toolkit/themes/windows/global/icons/close-win8.png was unused.

Stephen said over IRC[1] that we should use the Windows8 close icons for all versions of Windows, so I've overwritten close.png with close-win8.png and used close-win8@2x.png from the icons attachment in bug 1147702 for close@2x.png.

[1] http://logs.glob.uno/?c=mozilla%23fx-team#c207859
Attachment #8610786 - Flags: review?(gijskruitbosch+bugs)
Attachment #8610805 - Flags: review?(gijskruitbosch+bugs)
Attachment #8610786 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch, part 1 (close icons) (obsolete) — Splinter Review
Attachment #8610786 - Attachment is obsolete: true
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Stephen said over IRC[1] that we should use the Windows8 close icons for all
> versions of Windows, so I've overwritten close.png with close-win8.png and
> used close-win8@2x.png from the icons attachment in bug 1147702 for
> close@2x.png.

Even better would be to switch to SVG like Linux uses…
Comment on attachment 8610815 [details] [diff] [review]
Patch, part 1 (close icons)

Review of attachment 8610815 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/windows/global/global.css
@@ -336,5 @@
>  .close-icon:hover:active {
>    -moz-image-region: rect(0, 48px, 16px, 32px);
>  }
>  
> -%ifdef XP_WIN

You should keep this, as Linux is already using SVG...
Comment on attachment 8610805 [details] [diff] [review]
Patch, part 2 (new tab and tab arrows)

Review of attachment 8610805 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +1973,5 @@
> +  #TabsToolbar > toolbarpaletteitem > #new-tab-button {
> +    list-style-image: url(chrome://browser/skin/tabbrowser/newtab@2x.png);
> +  }
> +
> +  .tabs-newtab-button > .toolbarbutton-icon {

I would have expected this rule to need to include the other two selectors, too. Am I wrong? If so, please add a comment to the code as to why.
Attachment #8610805 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8610815 - Flags: review?(gijskruitbosch+bugs)
Attachment #8610815 - Flags: review?(gijskruitbosch+bugs)
Added the %ifdef guards around the .close-icon section in global.css. Note that I removed the %ifdef in the previous version because it was only guarding the media queries for lunaBlue, etc.
Attachment #8610815 - Attachment is obsolete: true
Attachment #8611326 - Flags: review?(gijskruitbosch+bugs)
Added the extra selectors to the rule. Previously, there was a couple pixel difference between the new tab button as a tab, the docked new tab button, and the new tab button in customization mode. With this change, they are now all the same size.
Attachment #8610805 - Attachment is obsolete: true
Attachment #8611327 - Flags: review+
Comment on attachment 8611326 [details] [diff] [review]
Patch, part 1 v1.1

Review of attachment 8611326 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but see below.

::: toolkit/themes/windows/global/global.css
@@ +341,5 @@
> +@media (min-resolution: 1.1dppx) {
> +  .close-icon > .button-icon,
> +  .close-icon > .button-box > .button-icon,
> +  .close-icon > .toolbarbutton-icon {
> +    width: 16px;

Should we do this everywhere, not just for hidpi, for consistency and so we can spot issues more quickly? This might also affect add-ons.
Attachment #8611326 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Created attachment 8611326 [details] [diff] [review]
> Patch, part 1 v1.1
> 
> Added the %ifdef guards around the .close-icon section in global.css. Note
> that I removed the %ifdef in the previous version because it was only
> guarding the media queries for lunaBlue, etc.

Huh. Rereading code and doublechecking jar.mn, AFAICT the linux one overrides the Windows one anyway, so we can indeed do without the %ifdef. Sorry about leading you astray there.

I also suspect (but would like shorlander/mmaslaney/some-graphics-y-person to doublecheck) that we could use the linux SVGs on Windows (potentially with minor tweaking). Can you file a followup for that?
Depends on: 1173711
Attached patch Patch for 40Splinter Review
Approval Request Comment
[Feature/regressing bug #]: Windows 10 HiDPI
[User impact if declined]: HiDPI icons on Windows are blurry
[Describe test coverage new/current, TreeHerder]: on mozilla-central for much of 41-nightly
[Risks and why]: none expected 
[String/UUID change made/needed]: none

https://hg.mozilla.org/try/pushloghtml?changeset=71084a9edf1e
Attachment #8627204 - Flags: approval-mozilla-beta?
Comment on attachment 8627204 [details] [diff] [review]
Patch for 40

Visual fix in support of Windows 10. On m-c for more than a month. Beta+
Attachment #8627204 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1166442
No longer blocks: 1166442
Depends on: 1210469
You need to log in before you can comment on or make changes to this bug.