Closed Bug 1337856 Opened 7 years ago Closed 7 years ago

Installation assets in Windows are low dpi

Categories

(Firefox :: Installer, defect, P1)

All
Windows
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: sole, Assigned: molly)

References

Details

(Whiteboard: [fce-active-legacy])

Attachments

(10 files)

I just installed Nightly on a Windows 10 laptop. The graphics assets displayed while the installer is running are low dpi, which unfortunately makes the process look quite unpolished when running on this high resolution laptop where everything else is very sharp looking.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86
Priority: -- → P3
What images do we need to fix this? bgstub.bmp is the only image I see in the branding directory and it's low res.
Flags: needinfo?(mhowell)
That's the only image involved (for each branding).

There's a bigger issue though, which is how to get that image actually displayed; ideally I'd like to include just a high res image and scale it down for low res displays, but the scaling that's used in there right now makes that look hideous. And including both images would be too much file size. So I need to come up with something clever before we can make any use of a higher res image.
Flags: needinfo?(mhowell)
Attached image Scaling.png
I've noticed this issue too. Lines are not smooth in the background and the logo is not scaled nicely. See attachment which illustrates this behavior. Probably you could use vector images?

Thanks.
Whiteboard: [fce-active]
We keep getting more reports of this. It needs to get fixed.
OS: Windows 10 → Windows
Priority: P3 → P1
Hardware: x86 → All
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Stephen, I think I finally have a way to fix this, but I need high-res versions of the stub installer background image for nightly, dev edition, and release; there's one in bug 1365998 that I can use for the unofficial branding, but the others there are out of date. I seem to remember you've offered to provide those before, are you still who I should be asking?
Flags: needinfo?(shorlander)
Attached image bgstub@2x.jpg - Firefox
Flags: needinfo?(shorlander)
Thanks @shorlander, those are perfect.

I'm going to verify that all of these images look nice under different DPI settings with the new scaling method I'm trying to use, and I'll have patches up here for review once that's done.
The above testing didn't go quite perfectly. The 2x images look amazing on a hi-DPI display, but have lighter text on the "Nightly" and "Developer Edition" labels, and that text shows aliasing when scaled down by half for a display not in a hi-DPI mode.

Fortunately, since we'll be able to use JPEG's now, we can include both the 1x and 2x image in the stub installer build without affecting the file size (in fact the file size still ends up dropping about 45K) and select the correct one to use based on the system DPI. So that's my new plan.
We want to make sure things look good at 150% on Windows (since that's the recommended settings). If we can, we should explicitly choose 1x or 2x on 150% rather than trying to do scaling.
Well, we have to either scale the 1x image up or the 2x image down for 150%. Scaling down the 2x seems to look better to me at 150%, so that's what the above patch does.
Comment on attachment 8918450 [details]
Bug 1337856 Part 1 - Use a better up/down-scaling method for the stub installer background.

https://reviewboard.mozilla.org/r/189290/#review195100
Attachment #8918450 - Flags: review?(agashlin) → review+
Comment on attachment 8918451 [details]
Bug 1337856 Part 2 - Swap out stub installer background images with JPEG's, and add high-res versions.

https://reviewboard.mozilla.org/r/189292/#review195102
Attachment #8918451 - Flags: review?(agashlin) → review+
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1ded70b5b84
Part 1 - Use a better up/down-scaling method for the stub installer background. r=agashlin
https://hg.mozilla.org/integration/autoland/rev/e34bf13e7042
Part 2 - Swap out stub installer background images with JPEG's, and add high-res versions. r=agashlin
57 is not on nightly anymore.
We should seriously consider this for 57. We're putting a lot of energy into the stub installer for 57 and it looks really crappy on hires Windows.
Attached image scaling_125_fixed.png
For reference, this is what the fixed version of attachment 8904796 [details] looks like; the difference in quality of the wordmark is huge.
Indeed but do we have the same issue with release?
Attached image release_150.png
(In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> Indeed but do we have the same issue with release?

Yes.

On the left is the current beta stub at 150% scaling. On the right is one I just built with the release branding and this patch.

I actually didn't think it would be that bad, I'm kind of surprised; I thought a lot of the problem was due to the lighter weight of the type on e.g. the word "Nightly" but this is just as bad, and the logo looks a lot worse over the lighter background.
(Can we reconsider the 57 wontfix, then? IMO we should strongly consider 57 uplift here. This bug gives a bad first impression to users who we anticipate/hope will be coming back to Firefox to try 57 -- so if this patch is OK from a riskiness perspective, I think we should try to take it.)
Flags: needinfo?(sledru)
If this is impacting release, we probably want to take it.
Leave the final call to Ritu as it is late here.
Flags: needinfo?(sledru) → needinfo?(rkothari)
Hi Matt, this looks like a safe/low risk change that we can uplift to 57. I am assuming this is also a recent regression in 57. Please nominate a patch for uplift to Beta57. Thanks!
Flags: needinfo?(rkothari) → needinfo?(mhowell)
Thanks, Ritu. The patches that are on here apply cleanly to beta for me, so I'll put in the request on those.

This isn't a recent regression at all I'm afraid, it's been present in one form or another since the stub installer has been around, but became more noticeable as of 55.
Flags: needinfo?(mhowell)
Comment on attachment 8918450 [details]
Bug 1337856 Part 1 - Use a better up/down-scaling method for the stub installer background.

Approval Request Comment
[Feature/Bug causing the regression]:
This bug has been present since the stub installer has been around, but was made worse by bug 1365998.

[User impact if declined]:
See comment 30.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
I've verified it manually myself; I checked how all the backgrounds look on multiple display scaling settings.

[Needs manual test from QE? If yes, steps to reproduce]:
No, I've done manual testing already.

[List of other uplifts needed for the feature/fix]:
No other bugs, just both parts on this one.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's a small, targeted patch that doesn't affect the installation process itself, and I've tested it manually.

[String changes made/needed]:
None
Attachment #8918450 - Flags: approval-mozilla-beta?
Comment on attachment 8918451 [details]
Bug 1337856 Part 2 - Swap out stub installer background images with JPEG's, and add high-res versions.

Approval Request Comment
See above comment for other patch.
Attachment #8918451 - Flags: approval-mozilla-beta?
Comment on attachment 8918450 [details]
Bug 1337856 Part 1 - Use a better up/down-scaling method for the stub installer background.

Thanks Matt. I agree, this feels like a must fix to make 57 more compelling/awesome, Beta57+
Attachment #8918450 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8918451 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached file issueswin10.zip
I've tested on Windows 7 x64 and Windows 10 x64 (on laptop) using Firefox 57 Beta 13 and latest Nightly 58.0a1, DPI 125%, 150%, 175%/200% and I have the following mentions: 
- on Windows 7 x64 - all the icons, background images, installation assets and the text of Stub Installer look good
- on Windows 10 (laptop) - the background images and the text of Stub Installer look good, but the Title and the Taskbar icons are low dpi (on 150% and 175%). Also, I noticed that the Title Bar is a few pixels shorter than the rest of the Stub window (see "TitleBar.ing"). Please see attachment "issueswin10.png". 

Any thoughts?
Flags: needinfo?(mhowell)
Were you logging out of Windows after changing the scaling setting? Not doing that is the only way I can reproduce either issue. There's not really any way around needing to do that just because of how the relevant Windows API's work, which is why the display settings dialog recommends it after the scaling is changed.
Flags: needinfo?(mhowell)
Yes, you are right: I'm not logging out of Windows after changing the scaling setting. I've tested again and I've performed log out after changing the scaling setting: the Title and the Taskbar icons look better now, but I consider that the icons still are a bit low dpi.  What do you think? 
Please see the attachment "logout-after-changing-scaling.zip".
Hmm. I see what you're talking about, but I can't reproduce it. It looks like Explorer is selecting the wrong size icon to display for some reason, but I'm not sure what we can do about that. What happens if you turn off small taskbar buttons, is it still broken?

Also, this is something that should already have been working okay prior to this patch and that should not have been affected by this patch, so it might be better to file a separate bug.
 (In reply to Matt Howell [:mhowell] from comment #42)
> Hmm. I see what you're talking about, but I can't reproduce it. It looks
> like Explorer is selecting the wrong size icon to display for some reason,
> but I'm not sure what we can do about that. What happens if you turn off
> small taskbar buttons, is it still broken?
> 
> Also, this is something that should already have been working okay prior to
> this patch and that should not have been affected by this patch, so it might
> be better to file a separate bug.

I logged bug 1417928. 
Marking this issue as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: