Tab close icons on Windows HiDPI are blurry

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Theme
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
status-firefox41: affected → ---
Created attachment 8610786 [details] [diff] [review]
Patch

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)
Created attachment 8610805 [details] [diff] [review]
Patch, part 2 (new tab and tab arrows)
Attachment #8610805 - Flags: review?(gijskruitbosch+bugs)
Attachment #8610786 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8610815 [details] [diff] [review]
Patch, part 1 (close icons)
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 5

3 years ago
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 6

3 years ago
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)
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.
Attachment #8610815 - Attachment is obsolete: true
Attachment #8611326 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8611327 [details] [diff] [review]
Patch, part 2 (new tab and tab arrows)

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 9

3 years ago
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+

Comment 10

3 years ago
(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?

Comment 11

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/634e5b445a57
https://hg.mozilla.org/integration/fx-team/rev/028160c9d05c

Comment 12

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/d93f6b15c003
https://hg.mozilla.org/mozilla-central/rev/634e5b445a57
https://hg.mozilla.org/mozilla-central/rev/028160c9d05c
https://hg.mozilla.org/mozilla-central/rev/d93f6b15c003
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

3 years ago
Depends on: 1173711
Created attachment 8627204 [details] [diff] [review]
Patch for 40

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?
status-firefox40: --- → affected
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/1688652717d3
status-firefox40: affected → fixed

Updated

2 years ago
Blocks: 1166442

Updated

2 years ago
No longer blocks: 1166442

Updated

2 years ago
Depends on: 1210469
You need to log in before you can comment on or make changes to this bug.