Closed Bug 1761389 Opened 3 years ago Closed 3 years ago

Make Firefox PDF handler display Windows default PDF handler icon

Categories

(Firefox :: Installer, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
100 Branch
Tracking Status
firefox100 --- fixed

People

(Reporter: nalexander, Assigned: nalexander, NeedInfo)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fidedi-pdf])

Attachments

(3 files, 4 obsolete files)

With reference to this OS Default Browsing Handling Spec, section 2.2.1.2 has the following:

Acceptance criteria

  • Update the currently displayed icon to the Windows system PDF icon
  • Update the File type that gets displayed from currently “Firefox HTML document” to “Firefox PDF document”

The latter piece is, I think straightforward: we duplicate FirefoxHTML-... to FirefoxPDF-..., bump the string, and use it for .pdf.

The former piece is, however, not so straightforward. The "Windows default" PDF icon is, in fact, a Microsoft Edge PDF icon. It's defined in msedge.exe. That means, I expect, that we can't ship it. Now, Edge is part of Windows 10, so we can refer to it and have a reasonable expectation that it exists, but we do accrue some risk that is not particularly easy to mitigate. The icon may disappear but it's more likely that it evolves without Firefox knowing. (The only times we can reasonably check that it's the expected icon is at install time and during the post-update process. And if it changes, what do we do --fall back to our existing document icon?) We cede control of our visual presence.

I will note that Chrome does not show a document-specific icon: it shows the Chrome "wheel" icon for PDF files. I've not checked other browsers (Brave, Yandex).

rtestard: how much do we care for this exact icon? Would we accept the risk of referring to Edge's icon, given that Edge could change the icon (to refer to Edge, say) without Firefox being able to intervene? My expectation is that we would not accept that risk. We could take the Beta 100 cycle to get an updated "Firefox PDF Document" icon from UX without impacting anything else here.

mhowell: suppose we were to make Computer\HKEY_CLASSES_ROOT\FirefoxPDF-...\DefaultIcon refer to C:\Program Files (x86)\Microsoft\Edge\Application\msedge.exe,13, aping Edge's icon choice. Let's assume that can be C:\Program Files\... to benefit from the OS-aliasing or that we can determine the appropriate path at install/post-update time. Can you think of any issues not mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1761389#c0? If we did this, can you think of a slick way to detect icon changes? I was thinking that if we must we can extract the bits of some/all resolutions of the icon and verify they are well known values to determine if the icon is changed, although I'm not really eager to do that in NSIS. (Perhaps another thing to put in the WDBA binary.)

Flags: needinfo?(rtestard)
Flags: needinfo?(mhowell)

I would strongly recommend against reading that icon directly out of the Edge binary's resources. Apps pulling icons out of Windows binaries (especially shell32.dll) has been a thing for a long time, and from what I understand, MS are not fans at all. I would not be surprised to see either an incidental change that breaks our usage somehow or even some kind of deliberate sabotage.

I think there is a bit more stable thing we could do though, besides designing an icon of our own: call QueryCurrentDefault to get the ProgID for the default PDF handler (this would have to happen before we override that, of course), then look that up to see what its icon is, and use whatever we get from that. That way, if either the path to Edge itself, the content of icon resource ID 13, or even what the default app is ever changes, we'll be able to follow the shell itself to where the new PDF icon is. So, I'd prefer if we brought our own icon, to avoid any potential unpleasantness whatsoever, but if we're determined to use the system icon, that's how I'd recommend going about it.

Flags: needinfo?(mhowell)

(In reply to Molly Howell (she/her) [:mhowell] from comment #2)

I would strongly recommend against reading that icon directly out of the Edge binary's resources. Apps pulling icons out of Windows binaries (especially shell32.dll) has been a thing for a long time, and from what I understand, MS are not fans at all. I would not be surprised to see either an incidental change that breaks our usage somehow or even some kind of deliberate sabotage.

Thanks for this context.

I think there is a bit more stable thing we could do though, besides designing an icon of our own: call QueryCurrentDefault to get the ProgID for the default PDF handler (this would have to happen before we override that, of course), then look that up to see what its icon is, and use whatever we get from that. That way, if either the path to Edge itself, the content of icon resource ID 13, or even what the default app is ever changes, we'll be able to follow the shell itself to where the new PDF icon is. So, I'd prefer if we brought our own icon, to avoid any potential unpleasantness whatsoever, but if we're determined to use the system icon, that's how I'd recommend going about it.

One problem is that the current default might not, in fact, be Edge. But we can certainly get the ProgID for Edge (it's MSEdgePDF) and track down its icon directly, rather than baking in a (fragile) path. Thanks for that nudge.

rtestard: I'll plan to front-load everything else for Fx100, and then do this Edge-fishing last. In the interim we get Beta 100 to try to cook up our own resource, and if we don't, we will race getting our icon against Edge making us look foolish.

Thanks for all that context. I was trying to find a way to progress this without UX dependencies though it sounds like there is no choice but designing a new icon or using an existing one that is free to be used such as https://commons.wikimedia.org/wiki/File:PDF_icon.svg
Let me validate with UX what their take is on designing VS re-using an icon that is useable given our licenses.

Flags: needinfo?(rtestard)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED

Aaron, once you have validation for the icon to use can you please post here?

Flags: needinfo?(abenson)

This anticipates separating the FirefoxPDF-... handler from the
FirefoxHTML-... handler.

This is simply the mechanics of s/FirefoxHTML-.../FirefoxPDF-.../. We
need this to alter the description ("Firefox PDF Document") and, in
subsequent commits, the display icon.

Generally I tried to keep alphabetical ordering: FirefoxHTML-...,
FirefoxPDF-..., FirefoxURL-...; and I generally tried to use the '-'
suffix to disambiguate the older FirefoxHTML from the newer suffixed
FirefoxHTML-... form.

Depends on D142301

This does two things. First, it expects the special
FirefoxPDF-... ProgID to be available for UserChoice. We could manage
without it for a while, but eventually we expect set-to-default to
include PDFs. When that is the case, if it doesn't exist, something
has gone very wrong, and we'd like to find that out (via our existing
telemetry, which reports "missing" ProgIDs).

Second, it arranges to use the new FirefoxPDF-... ProgID when
setting-to-default.

Depends on D142302

In addition, this makes the other file associations have a non-default
description, like "Firefox HTML Document", which agrees (at least for
Release and Beta) with what the installer arranges. (The Windows
default description is otherwise "HTML Document", "PDF Document",
etc.)

Depends on D142303

The actual icon will follow when UX delivers it.

Depends on D142395

Comment on attachment 9269793 [details]
Bug 1761389 - Part 1: Accommodate arbitrary ProgID roots in set-default-browser-user-choice. r?bhearsum

Revision D142301 was moved to bug 1762100. Setting attachment 9269793 [details] to obsolete.

Attachment #9269793 - Attachment is obsolete: true

Comment on attachment 9269794 [details]
Bug 1761389 - Part 2: Add and use FirefoxPDF-... file association. r?bhearsum

Revision D142302 was moved to bug 1762100. Setting attachment 9269794 [details] to obsolete.

Attachment #9269794 - Attachment is obsolete: true

Comment on attachment 9269795 [details]
Bug 1761389 - Part 3: Expect and use FirefoxPDF-.... r?bhearsum

Revision D142303 was moved to bug 1762100. Setting attachment 9269795 [details] to obsolete.

Attachment #9269795 - Attachment is obsolete: true

Comment on attachment 9269796 [details]
Bug 1761389 - Part 4: Make MSIX have PDF-specific document description. r?bhearsum

Revision D142304 was moved to bug 1762100. Setting attachment 9269796 [details] to obsolete.

Attachment #9269796 - Attachment is obsolete: true

No need to keep this private.

Group: mozilla-employee-confidential
Attached file Icon PNG files (deleted) —
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fc94655ca051 Pre: Use defines rather than inline numerals for icon indices. r=bhearsum https://hg.mozilla.org/integration/autoland/rev/6f721a9a39c0 Add and use new PDF-specific document icon on Windows. r=bhearsum
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: