1454610 - DMG quantum background image not properly sized

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[Axel Hecht [:Pike]]

Attachment: Screen Shot 2018-04-17 at 12.35.50.png

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.

Comment 1

[Gyula Palko]

Reproducible on Mac OS, all Quantum builds.

Comment 2

[Matt Howell (he/him) [:mhowell]]

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.

Comment 3

[Matt Howell (he/him) [:mhowell]]

:spohl, do you have any ideas about this, or know someone better to ask? Because I've got nothing.

Comment 4

[Stephen A Pohl [:spohl]]

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.

Comment 5

[Matt Howell (he/him) [:mhowell]]

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

[Stephen A Pohl [:spohl]]

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

[Matt Howell (he/him) [:mhowell]]

Okay, you've got bug 1455768. Congratulations. ;)

I'll move this and bug 1439072 over there also.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

This affects Firefox from 60.0, including 60.0esr and the 61.0 betas. Not present in 59.0.

Comment 10

[Justin Dolske [:Dolske]]

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?

Comment 11

[Justin Dolske [:Dolske]]

Tim: Anything you see in the old-vs-new background.png that changed?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1454610 - Restore PNG density metadata on DMG backgrounds

Review commit: https://reviewboard.mozilla.org/r/250664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250664/

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d1602adb04e9db7ad4efa7b8847564bdac1549

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This affects install experiences, perhaps we should uplift.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or IRC)]

(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?

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds

https://reviewboard.mozilla.org/r/250662/#review259846

Comment 23

[Justin Dolske [:Dolske]]

Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds

https://reviewboard.mozilla.org/r/250662/#review259848

Comment 24

[Justin Dolske [:Dolske]]

(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

[Justin Dolske [:Dolske]]

(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.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a63eb13bc2
Restore PNG density metadata on DMG backgrounds r=dolske

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/19a63eb13bc2

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 30

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or IRC)]

(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

Comment 31

[Liz Henry (:lizzard)]

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.

Comment 32

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0d7247da1b7

Comment 33

[Cristian Fogel, QA [:cfogel]]

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?

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 35

[Cristian Fogel, QA [:cfogel]]

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 [...]?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/c7663317f3d4

Comment 38

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

[Cristian Fogel, QA [:cfogel]]

(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.

Comment 41

[Cristian Fogel, QA [:cfogel]]

Fix confirmed on 61.0.1 as well.

Comment 42

[Cristian Fogel, QA [:cfogel]]

Updating bug status as well.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984884 [details]
Bug 1454610 - Restore PNG density metadata on DMG backgrounds

Approved for ESR 60.2 also.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/c01e26f35e02

Comment 45

[Cristian Fogel, QA [:cfogel]]

Fix confirmed on ESR60, using the build(s) from taskCluster.