Closed
Bug 1454610
Opened 7 years ago
Closed 7 years ago
DMG quantum background image not properly sized
Categories
(Firefox :: Theme, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 63
People
(Reporter: Pike, Assigned: timdream)
References
Details
(Keywords: regression)
Attachments
(2 files)
197.26 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
Dolske
:
review+
lizzard
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-release+
RyanVM
:
approval-mozilla-esr60+
|
Details |
While trying the DMG from http://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/mac/de/, I found that the background graphic gets the scaling wrong.
Attaching a screenshot.
I have a non-retina secondary monitor, but the dialog showed on my retina display, maybe that's part of it.
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Reproducible on Mac OS, all Quantum builds.
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Comment 2•7 years ago
|
||
Yeah, this reproduces on my non-retina MacBook too (running 10.13.4). Between this and bug 1439072 I'm definitely starting to think we're coming up against Finder bugs.
Component: Installer → Theme
Comment 3•7 years ago
|
||
:spohl, do you have any ideas about this, or know someone better to ask? Because I've got nothing.
Flags: needinfo?(spohl.mozilla.bugs)
Comment 4•7 years ago
|
||
I don't know off-hand. I would set a need-info on whoever added this background graphic to see if this is a known issue and to get this addressed.
Flags: needinfo?(spohl.mozilla.bugs)
Comment 5•7 years ago
|
||
Looking into that is taking me through a whole list of people who are no longer active. :( Well, and also :glandium, but this doesn't really seem like his problem. I wouldn't mind owning this thing myself, I just don't know how to even start investigating these bugs; my Mac expertise is limited anyway, and this is a particularly esoteric corner even within that realm. But I'll see if there's anything I can do.
Also, I've just discovered in the process of looking into this bug that the current dev edition release just doesn't show the background art at all, apparently because the volume name isn't what the prebaked .DS_Store file expects. So that's cool. I'll file that separately, I guess.
Comment 6•7 years ago
|
||
Feel free to kick this over to widget:cocoa and mark it as P2 if you can't find the time to look into it. I will prioritize alongside other platform work.
Comment 7•7 years ago
|
||
Okay, you've got bug 1455768. Congratulations. ;)
I'll move this and bug 1439072 over there also.
Updated•7 years ago
|
Component: Theme → Widget: Cocoa
Product: Firefox → Core
Updated•7 years ago
|
Priority: -- → P2
Comment 9•7 years ago
|
||
This affects Firefox from 60.0, including 60.0esr and the 61.0 betas. Not present in 59.0.
Comment 10•7 years ago
|
||
This has broken before, the process to make this all work is a bit delicate/complicated...
See bug 1435359 comment 2 on the steps that are normally needed to do this.
I don't see any changes to the DSStore file since, although bug 1234008 did touch the background.png used... Stephen, was there anything special about the PNG used that might have been affected by the image optimizer?
I wonder if we need to figure out a way to automatically test this... Screenshot the Finder window, and compare with a reference image? Add it to the QA manual verification for Release?
Flags: needinfo?(shorlander)
Comment 11•7 years ago
|
||
Tim: Anything you see in the old-vs-new background.png that changed?
Flags: needinfo?(timdream)
Assignee | ||
Comment 12•7 years ago
|
||
Comment 1 and comment 9 contracts to one another.
Anyway, I've tried to see https://ftp.mozilla.org/pub/firefox/releases/59.0.3/mac/en-US/Firefox%2059.0.3.dmg and it shows up correctly. Nightly seems unaffected.
Will dig deeper... keeping needinfo.
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> Nightly seems unaffected.
Nightly is not affected because the image is not hi-res anyway.
From tools, I can see the original background.png comes with DPI set to 144 pixels/inch.
https://hg.mozilla.org/mozilla-central/raw-file/69c3cb2e731c/browser/branding/official/background.png
Playing around with Preview.app and exiftools I can see the following command will allow the new file to be displayed in the same dimensions as the original file in Preview.app.
exiftool -XResolution=144 -YResolution=144 -PixelsPerUnitX=5669 -PixelsPerUnitY=5669 background.png
I'll push a try build with this change.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8984884 -
Flags: review?(dolske)
Assignee | ||
Comment 16•7 years ago
|
||
Try result confirms this bug can be fixed by comment 13.
I am not entirely sure how to prevent this footgun in the future. Taking a screenshot of the Finder window is perhaps an overkill?
How about implementing a small PNG parser in mochitest to read out the metadata tags? Would that be sufficient? Do we have that in the tree already?
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> How about implementing a small PNG parser in mochitest to read out the
> metadata tags? Would that be sufficient? Do we have that in the tree already?
Hum, that won't work -- we don't put these files in the jarfiles.
Assignee | ||
Comment 18•7 years ago
|
||
This affects install experiences, perhaps we should uplift.
tracking-firefox60:
--- → ?
tracking-firefox61:
--- → ?
tracking-firefox62:
--- → ?
tracking-firefox-esr60:
--- → ?
Comment 19•7 years ago
|
||
Too late for ESR 60.1 and Fx61 at this point (we're already building RCs), but this seems like a good dot release ride-along candidate should the opportunity arise.
Keywords: regression
Comment 20•7 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #10)
> This has broken before, the process to make this all work is a bit
> delicate/complicated...
>
> See bug 1435359 comment 2 on the steps that are normally needed to do this.
>
> I don't see any changes to the DSStore file since, although bug 1234008 did
> touch the background.png used... Stephen, was there anything special about
> the PNG used that might have been affected by the image optimizer?
>
> I wonder if we need to figure out a way to automatically test this...
> Screenshot the Finder window, and compare with a reference image? Add it to
> the QA manual verification for Release?
Reading the rest of the comments, it seems like the issue is some processing of PNGs stripping the meta information from the PNG?
Flags: needinfo?(shorlander)
Updated•7 years ago
|
Component: Widget: Cocoa → Theme
Product: Core → Firefox
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
https://reviewboard.mozilla.org/r/250662/#review259846
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
https://reviewboard.mozilla.org/r/250662/#review259848
Updated•7 years ago
|
Attachment #8984884 -
Flags: review?(dolske) → review+
Comment 24•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> I am not entirely sure how to prevent this footgun in the future. Taking a
> screenshot of the Finder window is perhaps an overkill?
>
> How about implementing a small PNG parser in mochitest to read out the
> metadata tags? Would that be sufficient? Do we have that in the tree already?
Yeah, I'm not really sure we have test infra set up for that. You'd also need the test to be able to differentiate between Nightly/DevEd/Release, and screenshots can be annoying if slight differences exist (e.g. maybe scaling is slightly different on some OS versions).
Maybe easiest thing to do here is have QA add this to their list of periodic release checks?
Comment 25•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> Nightly is not affected because the image is not hi-res anyway.
Do we want a hidpi image for the Nightly DMG? If so, new bug w/ desired image.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #25)
> Do we want a hidpi image for the Nightly DMG? If so, new bug w/ desired
> image.
Yes, let me know if there is an asset ready and I can compress and land it.
Comment 27•7 years ago
|
||
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a63eb13bc2
Restore PNG density metadata on DMG backgrounds r=dolske
Comment 28•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee | ||
Comment 29•7 years ago
|
||
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1234008
[User impact if declined]: Mac installer will come with an incorrect background
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Cannot be verified since this only affect official branding. Verified on Try in comment 15.
[Needs manual test from QE? If yes, steps to reproduce]: Manually inspect the Mac installer on Beta and official build
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just an asset update.
[String changes made/needed]: None.
Can this also ride-along a dot-release in 61? Do we want to take this in esr60 too?
Attachment #8984884 -
Flags: approval-mozilla-release?
Attachment #8984884 -
Flags: approval-mozilla-beta?
Comment 30•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #26)
> (In reply to Justin Dolske [:Dolske] from comment #25)
> > Do we want a hidpi image for the Nightly DMG? If so, new bug w/ desired
> > image.
>
> Yes, let me know if there is an asset ready and I can compress and land it.
Created bug 1471657
Flags: needinfo?(shorlander)
Comment 31•7 years ago
|
||
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
Fix for recent regression. Let's take this for beta 4 and verify it in beta.
Attachment #8984884 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
![]() |
||
Comment 32•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
Flags: qe-verify+
Comment 33•7 years ago
|
||
Managed to reproduce with the link provided in comment 0.
Fixed on latest nightly (63.0a1 - 2018-06-29).
Still reproducible on 62.0b4, Timothy can you take a look at this please?
Flags: needinfo?(timdream)
Assignee | ||
Comment 34•7 years ago
|
||
I cannot reproduce this on 62.0b4, downloaded from this URL
https://download-installer.cdn.mozilla.net/pub/firefox/releases/62.0b4/mac/en-US/Firefox%2062.0b4.dmg
(redirected from https://download.mozilla.org/?product=firefox-beta-latest&os=osx )
I am on macOS 10.12.6.
Perhaps this can only reproduce on a different macOS version?
Flags: needinfo?(timdream) → needinfo?(cristian.fogel)
Comment 35•7 years ago
|
||
Rechecked on macOS: 10.9.5, 10.11.6, 10.13.4.
The fix is working after all with both the provided link and a download from http://archive.mozilla.org/pub/firefox/releases/62.0b4/mac/en-US/
During the re-testing I've noticed a scenario that was causing the issue to re-appear:
- opening the installer from the affected build;
- close it and open the 62.0b4 build.
The layout is still the broken one.
How should we proceed next; mark this as fixed and open up a follow-up bug or [...]?
Flags: needinfo?(cristian.fogel) → needinfo?(timdream)
Comment 36•7 years ago
|
||
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
Taking this as a ride-along for Fx 61.0.1. Please also request ESR60 approval on it when you get a chance.
Attachment #8984884 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•7 years ago
|
Attachment #8984884 -
Flags: approval-mozilla-esr60?
Comment 37•7 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to Cristi Fogel [:cfogel] from comment #35)
> Rechecked on macOS: 10.9.5, 10.11.6, 10.13.4.
>
> The fix is working after all with both the provided link and a download from
> http://archive.mozilla.org/pub/firefox/releases/62.0b4/mac/en-US/
>
> During the re-testing I've noticed a scenario that was causing the issue to
> re-appear:
> - opening the installer from the affected build;
> - close it and open the 62.0b4 build.
> The layout is still the broken one.
>
> How should we proceed next; mark this as fixed and open up a follow-up bug
> or [...]?
Please file a new bug. Mounting different builds is unlikely something ordinary users do every day. Thanks.
Flags: needinfo?(timdream)
Assignee | ||
Comment 39•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM][PTO July 4-9] from comment #36)
> Taking this as a ride-along for Fx 61.0.1. Please also request ESR60
> approval on it when you get a chance.
Given that you have the flag that already I assume I don't need to do anything more here?
Comment 40•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #38)
> Please file a new bug. Mounting different builds is unlikely something
> ordinary users do every day. Thanks.
New bug can be found at: https://bugzilla.mozilla.org/show_bug.cgi?id=1472937
Marking firefox62 as verified.
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Updated•7 years ago
|
Comment 43•7 years ago
|
||
Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds
Approved for ESR 60.2 also.
Attachment #8984884 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 44•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
Flags: qe-verify+
Comment 45•7 years ago
|
||
Fix confirmed on ESR60, using the build(s) from taskCluster.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•