1794159 - support pinning to taskbar in msix builds

Status: VERIFIED FIXED

(Firefox :: Shell Integration, enhancement, P3)

Description

[[e-mail] (:bhearsum)]

(Essentially a clone of https://bugzilla.mozilla.org/show_bug.cgi?id=1712628, because we already landed a patch there, which will mess up bug flags if we land additional ones.)

nsWindowsShellService.isCurrentAppPinnedToTaskbarAsync() currently relies on matching paths in the shortcut. This does not work for a packaged app. The UWP API TaskbarManager.IsCurrentAppPinnedAsync is the way to handle this for a packaged app.

I have a rough WIP, from trying to get RequestPinCurrentAppAsync working. It would need to be in addition to the existing implementation since this is only available from within a package.

This would only really be of use for telemetry. Aside: os.environment.launch_method is always Other when launching a package, whether it was opened from a taskbar pin, start menu shortcut, or desktop shortcut; there must be some amount of indirection so that we don't know the shortcut that opened us.

Comment 1

[[e-mail] (:bhearsum)]

I've been digging into this a bit. It's not too difficult to call the TaskbarManager methods -- but per this tutorial on pinning - RequestPinCurrentAppAsync must be called from a "foreground UI thread" because it prompts the user to confirm the request. It's not clear to me how to do this, or even if we can -- all examples I've found use cppwinrt methods with co_await (eg: https://learn.microsoft.com/en-us/archive/msdn-magazine/2018/june/c-effective-async-with-coroutines-and-c-winrt) -- which we're unable to do.

Even calling it from our own "main thread" causes it the async operation to never complete, and never show the UI.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to [e-mail] (:bhearsum) from comment #1)

I've been digging into this a bit. It's not too difficult to call the TaskbarManager methods -- but per this tutorial on pinning - RequestPinCurrentAppAsync must be called from a "foreground UI thread" because it prompts the user to confirm the request. It's not clear to me how to do this, or even if we can -- all examples I've found use cppwinrt methods with co_await (eg: https://learn.microsoft.com/en-us/archive/msdn-magazine/2018/june/c-effective-async-with-coroutines-and-c-winrt) -- which we're unable to do.

Even calling it from our own "main thread" causes it the async operation to never complete, and never show the UI.

Correction: it does complete, but with AsyncStatus::Error. There doesn't appear to be any way to get more detailed information, but my best guess is that it's throwing an error because it was unable to throw up the consent dialog for whatever reason.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: WIP: (not even working yet) Bug 1794159: support pinning to taskbar in MSIX builds

Comment 4

[Nick Rishel [:nrishel]]

After a bit of research I'm circling around an understanding that this is a UWP API that is not accessible from Win32 - albeit I think there is an escape hatch in XAML Islands. I'm not versed in XAML Islands and a cursory investigation suggests this is another half built bridge between Win32 and UWP that works for some APIs and not others, but this is likely the most fruitful direction for further investigation.

Comment 5

[Masatoshi Kimura [:emk]]

FYI: Microsoft seems to consider adding a new public API to pin apps to the taskbar.
https://blogs.windows.com/windowsexperience/2023/03/17/a-principled-approach-to-app-pinning-and-app-defaults-in-windows/
But I could not find any documentation yet.

Comment 6

[Nick Rishel [:nrishel]]

:emk, thanks for the link. At this time the API discussed doesn't yet exist, afaik not even in the Insider channel. If you notice a change definitely bring it to our attention.

Comment 7

[Nick Rishel [:nrishel]]

Follow up on XAML Islands, after testing it seems to be another dead end.

Comment 9

[Mathew Hodson]

As of bug 1712628, the "pinningStatusTelemetry" telemetry in BrowserGlue.sys.mjs logs an error to the browser console on Firefox from the Microsoft Stroe:

NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIWindowsShellService.isCurrentAppPinnedToTaskbarAsync]
    task resource:///modules/BrowserGlue.sys.mjs:2630
    _scheduleStartupIdleTasks resource:///modules/BrowserGlue.sys.mjs:3161
BrowserGlue.sys.mjs:2641:21

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Mathew Hodson from comment #9)

As of bug 1712628, the "pinningStatusTelemetry" telemetry in BrowserGlue.sys.mjs logs an error to the browser console on Firefox from the Microsoft Stroe:

NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIWindowsShellService.isCurrentAppPinnedToTaskbarAsync]
    task resource:///modules/BrowserGlue.sys.mjs:2630
    _scheduleStartupIdleTasks resource:///modules/BrowserGlue.sys.mjs:3161
BrowserGlue.sys.mjs:2641:21

Do we still need this check? The latest taskbar pinning method (for Win11 23H2+) is using WinRT TaskbarManager that this bug was going to implement.

Comment 12

[Nick Rishel [:nrishel]]

MSIX (Windows Store) builds still need to be tweaked a bit to accommodate pinning, specifically the Limited Access Feature token differs from unpackaged builds.

Comment 13

[Nipun Shukla]

Attachment: Bug 1794159 - Part 2: Add LAF unlocking and check for pinning status using new APIs r=nrishel,nalexander

Comment 14

[Nipun Shukla]

Attachment: Bug 1794159 - Part 1: Change C-style strings to nsString classes in LimitedAccessFeatures r=nrishel

Comment 15

[Nipun Shukla]

Attachment: Bug 1794159 - Part 3: Enable MSIX pinning in nsWindowsShellService r=nrishel,nalexander

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f28a29199ba8
Part 1: Change C-style strings to nsString classes in LimitedAccessFeatures r=nrishel
https://hg.mozilla.org/integration/autoland/rev/76b8d6cebe8b
Part 2: Add LAF unlocking and check for pinning status using new APIs r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/764e14e5df4a
Part 3: Enable MSIX pinning in nsWindowsShellService r=nrishel,nalexander

Comment 17

[pstanciu]

Backed out for causing bc failures on browser_browserGlue_telemetry.js

• Backout link
• Push with failures
• Failure Log

[task 2024-08-08T06:45:06.902Z] 06:45:06     INFO - TEST-START | browser/components/tests/browser/browser_browserGlue_telemetry.js
[task 2024-08-08T06:45:06.912Z] 06:45:06     INFO - TEST-INFO | started process screenshot
[task 2024-08-08T06:45:06.979Z] 06:45:06     INFO - TEST-INFO | screenshot: exit 0
[task 2024-08-08T06:45:06.980Z] 06:45:06     INFO - Buffered messages logged at 06:45:06
[task 2024-08-08T06:45:06.980Z] 06:45:06     INFO - Entering test bound check_startup_pinned_telemetry
[task 2024-08-08T06:45:06.981Z] 06:45:06     INFO - Buffered messages finished
[task 2024-08-08T06:45:06.982Z] 06:45:06     INFO - TEST-UNEXPECTED-FAIL | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_taskbar_pinned must not be reported. - false == true - 
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - Stack trace:
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - resource://testing-common/TelemetryTestUtils.sys.mjs:assertScalarUnset:28
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochitests/content/browser/browser/components/tests/browser/browser_browserGlue_telemetry.js:check_startup_pinned_telemetry:17
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:handleTask:1145
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1217
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1358
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1134
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1058
[task 2024-08-08T06:45:06.983Z] 06:45:06     INFO - Not taking screenshot here: see the one that was previously logged
[task 2024-08-08T06:45:06.984Z] 06:45:06     INFO - TEST-UNEXPECTED-FAIL | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_taskbar_pinned_private must not be reported. - false == true - 
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - Stack trace:
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - resource://testing-common/TelemetryTestUtils.sys.mjs:assertScalarUnset:28
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochitests/content/browser/browser/components/tests/browser/browser_browserGlue_telemetry.js:check_startup_pinned_telemetry:21
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:handleTask:1145
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1217
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1358
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1134
[task 2024-08-08T06:45:06.985Z] 06:45:06     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1058
[task 2024-08-08T06:45:06.986Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_kept_in_dock must not be reported. - true == true - 
[task 2024-08-08T06:45:06.986Z] 06:45:06     INFO - Leaving test bound check_startup_pinned_telemetry
[task 2024-08-08T06:45:06.987Z] 06:45:06     INFO - Entering test bound check_is_default_handler_telemetry
[task 2024-08-08T06:45:06.988Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | true == true - 
[task 2024-08-08T06:45:06.988Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | .pdf handler present in telemetry - true == true - 
[task 2024-08-08T06:45:06.989Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | mailto handler present in telemetry - true == true - 
[task 2024-08-08T06:45:06.989Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler must be recorded. - true == true - 
[task 2024-08-08T06:45:06.990Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler must contain the '.pdf' key. - true == true - 
[task 2024-08-08T06:45:06.991Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler['.pdf'] must contain the expected value - false == false - 
[task 2024-08-08T06:45:06.991Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler must be recorded. - true == true - 
[task 2024-08-08T06:45:06.992Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler must contain the 'mailto' key. - true == true - 
[task 2024-08-08T06:45:06.992Z] 06:45:06     INFO - TEST-PASS | browser/components/tests/browser/browser_browserGlue_telemetry.js | os.environment.is_default_handler['mailto'] must contain the expected value - false == false - 
[task 2024-08-08T06:45:06.992Z] 06:45:06     INFO - Leaving test bound check_is_default_handler_telemetry
[task 2024-08-08T06:45:07.387Z] 06:45:07     INFO - GECKO(8228) | MEMORY STAT | vsize 2112654MB | vsizeMaxContiguous 64996544MB | residentFast 378MB | heapAllocated 174MB
[task 2024-08-08T06:45:07.388Z] 06:45:07     INFO - TEST-OK | browser/components/tests/browser/browser_browserGlue_telemetry.js | took 486ms
[task 2024-08-08T06:45:07.396Z] 06:45:07     INFO - GECKO(8228) | [Child 5100: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 1e9f03f7000 == 3 [pid = 5100] [id = 2]
[task 2024-08-08T06:45:07.397Z] 06:45:07     INFO - GECKO(8228) | [Child 5100: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 5 (1e9f5206c00) [pid = 5100] [serial = 5] [outer = 0]
[task 2024-08-08T06:45:07.397Z] 06:45:07     INFO - GECKO(8228) | [Child 5100: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 6 (1e9f03f7800) [pid = 5100] [serial = 6] [outer = 1e9f5206c00]
[task 2024-08-08T06:45:07.418Z] 06:45:07     INFO - checking window state
[task 2024-08-08T06:45:07.438Z] 06:45:07     INFO - TEST-START | browser/components/tests/browser/browser_browserGlue_upgradeDialog_trigger.js

Comment 18

[Nipun Shukla]

Updated tests and related telemetry to account for the new functionality and that private browser pinning is still missing. Waiting on review for new changes before attempting to land again.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df3d21f6704e
Part 1: Change C-style strings to nsString classes in LimitedAccessFeatures r=nrishel
https://hg.mozilla.org/integration/autoland/rev/bc1a6a793cff
Part 2: Add LAF unlocking and check for pinning status using new APIs r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/61bd9c87a76e
Part 3: Enable MSIX pinning in nsWindowsShellService r=nrishel,nalexander,firefox-desktop-core-reviewers

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/df3d21f6704e
https://hg.mozilla.org/mozilla-central/rev/bc1a6a793cff
https://hg.mozilla.org/mozilla-central/rev/61bd9c87a76e

Comment 21

[Pulsebot]

Backout by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/22f4cf67e8ad
Backed out 3 changesets for causing bc failures on browser_browserGlue_telemetry.js CLOSED TREE

Comment 22

[Eric Chen]

Attachment: Bug 1794159 - Part 1: Change C-style strings to nsString classes in LimitedAccessFeatures

Original Revision: https://phabricator.services.mozilla.com/D217762

Comment 23

[Eric Chen]

Attachment: Bug 1794159 - Part 2: Add LAF unlocking and check for pinning status using new APIs

Original Revision: https://phabricator.services.mozilla.com/D213331

Comment 24

[Eric Chen]

Attachment: Bug 1794159 - Part 3: Enable MSIX pinning in nsWindowsShellService

Original Revision: https://phabricator.services.mozilla.com/D217763

Comment 25

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Onboarding messaging not consistent with actual browser capabilities
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: 1. Have a Firefox 131+ MSIX build. 2. Launch the browser and navigate to about:welcome (should open automatically) 3. Make sure "Pin Firefox to task bar and start menu" is checked. 4. Click "save and continue" and Firefox should be pinned to the taskbar
• Risk associated with taking this patch: Not risky
• Explanation of risk level: Worst case scenario is that taskbar pinning is still not possible in MSIX builds, which is already the case right now
• String changes made/needed: N/A
• Is Android affected?: no

Comment 26

[Bogdan Maris, Desktop Test Engineering]

Verified that using both latest Nightly 132.0a1 and Firefox 131.0b2 the pin to taskbar and start menu work now with MSIX builds on Windows 10, Windows 11 and Windows 11 ARM. I did found 2 bugs bug 1917202 and bug 1917206 but the functionality itself works as expected. Will close this one as Verified and other bugs found will be handled separately.

Comment 27

[Pascal Chevrel:pascalc]

Let's have it ship with 131.