Closed Bug 1023607 Opened 5 years ago Closed 4 years ago

Implement Windows HiDPI theme for other icons and tiny widgets

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.2 - Jun 8
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: Dolske, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

This is a somewhat arbitrary breakdown of bug 1023511.

Part of the OS X Retina work (in bug 781327 and bug 817366) was adding HiDPI versions of various random icons and tiny widgets, which don't really have a better-defined grouping. See those bugs for details of which icons. We may also want to just look through the Windows theme dirs, and fix a big batch of such random icons here.

This is a somewhat loose "breakdown", since there's a bunch of these because we are basically have no existing Windows HiDPI assets. I'd suggest we just try to fix a big batch of them in this pass, and then do a separate pass to do remaining work in detail. (As opposed to trying to be perfect and complete all in one shot.)
Flags: firefox-backlog+
Do we have the necessary UX assets to start implementing this?
Points: --- → 8
This bug can probably pick up things like the stop/reload icons, the search icon, and the awesome bar dropdown arrow.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Flags: qe-verify+
Attached patch Patch (obsolete) — Splinter Review
This patch adds hidpi icons for the identity icons, stop/reload/go buttons, urlbar dropdown, customize mode titlebar button, and notification arrow in urlbar.

Still need hidpi notification icons.
Attachment #8609629 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8609629 [details] [diff] [review]
Patch

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

Almost there. We should probably include the urlbar- things on Linux, too, for consistency. There's a couple of places where we are (or the patch starts) using 2dppx instead of 1.1dppx which should be updated.

::: browser/themes/shared/notification-icons.inc.css
@@ +316,1 @@
>  @media (min-resolution: 2dppx) {

2dppx instead of 1.1 ?

::: browser/themes/windows/browser.css
@@ +1400,5 @@
>    background-image: radial-gradient(circle closest-side, hsla(205,100%,70%,.1), transparent);
>    -moz-image-region: var(--urlbar-dropmarker-active-region);
>  }
>  
> +@media (min-resolution: 2dppx) {

1.1dppx

::: browser/themes/windows/jar.mn
@@ +137,5 @@
>          skin/classic/browser/undoCloseTab.png                        (../shared/undoCloseTab.png)
>          skin/classic/browser/undoCloseTab@2x.png                     (../shared/undoCloseTab@2x.png)
>          skin/classic/browser/update-badge.svg                        (../shared/update-badge.svg)
>          skin/classic/browser/urlbar-arrow.png
> +        skin/classic/browser/urlbar-arrow@2x.png

Why is this not on Linux as well?

@@ +142,3 @@
>          skin/classic/browser/urlbar-popup-blocked.png
>          skin/classic/browser/urlbar-history-dropmarker.png
> +        skin/classic/browser/urlbar-history-dropmarker@2x.png

Ditto
Attachment #8609629 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch InterdiffSplinter Review
Attachment #8609629 - Attachment is obsolete: true
Attached patch Patch v1.1Splinter Review
(In reply to :Gijs Kruitbosch from comment #4)
> ::: browser/themes/shared/notification-icons.inc.css
> @@ +316,1 @@
> >  @media (min-resolution: 2dppx) {
> 
> 2dppx instead of 1.1 ?

Fixed.
 
> ::: browser/themes/windows/browser.css
> @@ +1400,5 @@
> >    background-image: radial-gradient(circle closest-side, hsla(205,100%,70%,.1), transparent);
> >    -moz-image-region: var(--urlbar-dropmarker-active-region);
> >  }
> >  
> > +@media (min-resolution: 2dppx) {
> 
> 1.1dppx

Fixed

> ::: browser/themes/windows/jar.mn
> @@ +137,5 @@
> >          skin/classic/browser/undoCloseTab.png                        (../shared/undoCloseTab.png)
> >          skin/classic/browser/undoCloseTab@2x.png                     (../shared/undoCloseTab@2x.png)
> >          skin/classic/browser/update-badge.svg                        (../shared/update-badge.svg)
> >          skin/classic/browser/urlbar-arrow.png
> > +        skin/classic/browser/urlbar-arrow@2x.png
> 
> Why is this not on Linux as well?

Fixed.

> @@ +142,3 @@
> >          skin/classic/browser/urlbar-popup-blocked.png
> >          skin/classic/browser/urlbar-history-dropmarker.png
> > +        skin/classic/browser/urlbar-history-dropmarker@2x.png
> 
> Ditto

This is not used on Linux. Linux uses -moz-appearance: toolbarbutton-dropdown. We can probably change Linux to use this custom styling, but I don't think this is the right bug for that.
Attachment #8610736 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8610736 [details] [diff] [review]
Patch v1.1

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > @@ +142,3 @@
> > >          skin/classic/browser/urlbar-popup-blocked.png
> > >          skin/classic/browser/urlbar-history-dropmarker.png
> > > +        skin/classic/browser/urlbar-history-dropmarker@2x.png
> > 
> > Ditto
> 
> This is not used on Linux. Linux uses -moz-appearance:
> toolbarbutton-dropdown. We can probably change Linux to use this custom
> styling, but I don't think this is the right bug for that.

For sure.
Attachment #8610736 - Flags: review?(gijskruitbosch+bugs) → review+
Iteration: 41.1 - May 25 → 41.2 - Jun 8
https://hg.mozilla.org/mozilla-central/rev/9ec0b5699f52
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Why the Linux changes? Were you aware that we don't have support for using this stuff on Linux?
Flags: needinfo?(jaws)
(In reply to Dão Gottwald [:dao] from comment #10)
> Why the Linux changes? Were you aware that we don't have support for using
> this stuff on Linux?

I wasn't aware. I admit that having untested/unusable code in the tree is generally unwelcome, but do you think when we have support for this that much will need to be changed?
Flags: needinfo?(jaws) → needinfo?(dao)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Why the Linux changes? Were you aware that we don't have support for using
> > this stuff on Linux?
> 
> I wasn't aware. I admit that having untested/unusable code in the tree is
> generally unwelcome, but do you think when we have support for this that
> much will need to be changed?

It's unclear when we'll have support, and as you said the styles being untested makes it more likely that they'll rot over time.
Flags: needinfo?(dao)
QA Contact: cornel.ionce
(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > (In reply to Dão Gottwald [:dao] from comment #10)
> > > Why the Linux changes? Were you aware that we don't have support for using
> > > this stuff on Linux?
> > 
> > I wasn't aware. I admit that having untested/unusable code in the tree is
> > generally unwelcome, but do you think when we have support for this that
> > much will need to be changed?
> 
> It's unclear when we'll have support, and as you said the styles being
> untested makes it more likely that they'll rot over time.

Just for my curiosity, what bit isn't supported? I'm almost sure I've seen people experiment with this by setting the relevant dpi prefs to something other than -1 by themselves in the past. So do you just mean automatic DPI/resolution detection? That seems to have been fixed for 38 by https://bugzilla.mozilla.org/show_bug.cgi?id=1031073 . Am I missing something?
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > > (In reply to Dão Gottwald [:dao] from comment #10)
> > > > Why the Linux changes? Were you aware that we don't have support for using
> > > > this stuff on Linux?
> > > 
> > > I wasn't aware. I admit that having untested/unusable code in the tree is
> > > generally unwelcome, but do you think when we have support for this that
> > > much will need to be changed?
> > 
> > It's unclear when we'll have support, and as you said the styles being
> > untested makes it more likely that they'll rot over time.
> 
> Just for my curiosity, what bit isn't supported? I'm almost sure I've seen
> people experiment with this by setting the relevant dpi prefs to something
> other than -1 by themselves in the past. So do you just mean automatic
> DPI/resolution detection?

yes

> That seems to have been fixed for 38 by
> https://bugzilla.mozilla.org/show_bug.cgi?id=1031073 . Am I missing
> something?

Interesting, I didn't know about that.
Flags: needinfo?(dao)
Verified fixed on latest Nightly, build ID: 20150602055237 using a MS Pro 2 device running Windows 8.1 64-bit.
Status: RESOLVED → VERIFIED
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 #8627206 - Flags: approval-mozilla-beta?
Comment on attachment 8627206 [details] [diff] [review]
Patch for 40

Verified fix in support of Windows 10. Beta+
Attachment #8627206 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Also confirming the fix on Firefox 40 beta 2, build ID: 20150706172413.
Duplicate of this bug: 1181514
Depends on: 1189202
You need to log in before you can comment on or make changes to this bug.