Add support for native Windows "Share" feature

NEW
Assigned to

Status

()

Firefox
Menus
P3
normal
a year ago
4 hours ago

People

(Reporter: Dolske, Assigned: daleharvey)

Tracking

(Blocks: 2 bugs, {feature})

Trunk
Unspecified
Windows
feature
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(relnote-firefox ?, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox57 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 affected)

Details

(Whiteboard: [reserve-photon-structure][tpi:+])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Bug 1352697 is implementing a "page actions" menu (e.g. attachment 8853671 [details]). One item in the menu is the ability to share the current page. We'd like to make use of OS-supplied functionality to do this.

Bug 1360054 has the UX spec for this (https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/232626421_Explainer_-_Share).

On Windows, this is basically just bringing up the modal Windows 10 UI as shown. (For other Windows versions, the Share menu will just be hidden.)

I'm not exactly sure what API this actually is. One of these?

https://docs.microsoft.com/en-us/uwp/api/Windows.ApplicationModel.DataTransfer.DataTransferManager#Windows_ApplicationModel_DataTransfer_DataTransferManager_ShowShareUI
https://msdn.microsoft.com/en-us/library/windows/apps/hh465251.aspx
(Reporter)

Comment 1

a year ago
Jim, can you help find an owner to implement this?
Flags: needinfo?(jmathies)

Updated

a year ago
Blocks: 1363180

Updated

a year ago
Flags: needinfo?(jmathies)
Whiteboard: [photon-structure][tpi:+]
Flags: qe-verify?
Priority: -- → P2
Flags: qe-verify? → qe-verify-
(Reporter)

Comment 2

a year ago
Moving to reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-structure][tpi:+] → [reserve-photon-structure][tpi:+]

Updated

a year ago
Duplicate of this bug: 1322813

Updated

11 months ago
Priority: P3 → P4

Updated

10 months ago
Priority: P4 → P5

Updated

10 months ago
Priority: P5 → P3

Comment 4

10 months ago
Downgrading the priority here while we focus on 57 (and have to wait for help with the native stuff).
Priority: P3 → P4
(Assignee)

Updated

4 months ago
Assignee: nobody → dharvey
(Assignee)

Comment 5

3 months ago
So got the gecko side of this done, was wondering if there was anyone that could point me in the right direction for the windows side of it

I would like to add shareUrl() to https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp, 

NS_IMETHODIMP
WindowsUIUtils::ShareUrl() 
{
// Ignore if mingw32
  ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager::ShowShareUI();
  return NS_OK;
}

Now when I do that, I get : 

 0:46.20 force-cargo-library-build
 0:58.97 [m[m[32m[1m    Finished[m release [optimized] target(s) in 0.54 secs
 1:38.51 WindowsUIUtils.cpp
 1:38.52 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(192): error C2027: use of undefined type 'ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager'
 1:38.52 c:\program files (x86)\windows kits\10\include\10.0.16299.0\winrt\Windows.ApplicationModel.DataTransfer.h(1226): note: see declaration of 'ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager'
 1:38.53 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(192): error C3861: 'ShowShareUI': identifier not found
 1:38.54 mozmake.EXE[4]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:1026: WindowsUIUtils.obj] Error 2
 1:38.54 mozmake.EXE[3]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:73: widget/windows/target] Error 2
 1:38.54 mozmake.EXE[3]: *** Waiting for unfinished jobs....
 1:38.65 mozmake.EXE[2]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:33: compile] Error 2
 1:38.65 mozmake.EXE[1]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:418: default] Error 2
 1:38.65 mozmake.EXE: *** [client.mk:168: build] Error 2
1:38.68 123 compiler warnings present.

Inside c:\program files (x86)\windows kits\10\include\10.0.16299.0\winrt\Windows.ApplicationModel.DataTransfer.h there are 10k lines of various definitions and I can see like 3000 references to datatransfer and 8 references to ShowShareUI, it seem like from the other code in WindowsUIUtils I want to be copying some of the definitions across, but not sure which ones (or quite why)

Gijs you pointed me in the right direction about this before, is this something you know about or know someone who will do? many thanks
Flags: needinfo?(gijskruitbosch+bugs)

Comment 6

3 months ago
You're not a UWP app so this is a bit more tricky than just calling that method. For one, all the UWP stuff is mirrored into win32 land a bit differently. But even then, apparently you normally need a CoreWindow instance for this particular method, which we don't have because (again) we're not a UWP app.

Fortunately, it seems some MSFT person has written a tutorial for this exact problem and this exact API. See https://blogs.msdn.microsoft.com/oldnewthing/20170315-00/?p=95735 . Hopefully that helps?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dharvey)

Comment 7

3 months ago
Note that we'll need to check both on the Gecko side and on the frontend side that we're actually on Win10+ (not win8 or win7 or something). For frontend, you can use AppConstant.isPlatformAndVersionAtLeast("win", "6.4"). For the C++ side, you can use IsWin10OrLater() .

Comment 8

3 months ago
Also (ugh, sorry for the triple post) - note that folks filed a few deps on the macOS bug that seem to imply we may want to pass more than just a URL. I wouldn't be surprised if you would need to ensure you provide that (e.g. window title info) here, too. It's kind of sad that the route we took in the mac bug means we have to have separate interfaces, however... it seems on Windows you would need to pass a window reference to get an hwnd from... so looks like the API needs more/different data on Windows anyway. :-(

Note that to provide actual sharing data on Windows, you can look at this blogpost: https://blogs.msdn.microsoft.com/oldnewthing/20170316-00/?p=95745 .
(Assignee)

Comment 9

3 months ago
Hugely helpful thanks, trying to construct the code as per the demo is giving me a compile error, specifically: 

 0:47.12 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): error C2039: 'IDataTransferManagerInterop': is not a member of 'ABI::Windows::ApplicationModel::DataTransfer'
 0:47.19 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\windows.applicationmodel.datatransfer.h(4755): note: see declaration of 'ABI::Windows::ApplicationModel::DataTransfer'
 0:47.30 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(209): error C2787: 'IDataTransferManagerInterop': no GUID has been associated with this object
 0:47.34 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(208): error C2660: 'RoGetActivationFactory': function does not take 2 arguments
 0:47.41 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(210): error C2039: 'ShowShareUIForWindow': is not a member of 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>'
 0:47.52 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): note: see declaration of 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>'
0:47.64 mozmake.EXE[4]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:1026: WindowsUIUtils.obj] Error 2

trying to figure out why now, apologies for being fairly lost here finding this win32 stuff pretty tricky to figure out, (the cocoa stuff was also new but was a lot easier), will try figuring out and ping you / someone if I cant make any progress, thanks again
Flags: needinfo?(dharvey)
(Assignee)

Comment 10

3 months ago
Full gist @ https://gist.github.com/daleharvey/c1e76edaa7abe548fd766391cfbad2f1 (I realise you didnt recommend getting the hwnd that way, was going to fix after). Also probably worth mentioning

$ grep -r "IDataTransferManagerInterop" "/c/Program Files (x86)/Windows Kits/10/include/10.0.16299.0/winrt"

is giving me nothing
(Assignee)

Comment 11

3 months ago
Hi Jim, Gijs mentioned I should talk to you about windows related issues, I have all the firefox frontent side done and basically am just trying to call a single windows api function, ShowShareUI(). Gis pointed me to an article explaining how to do that in a win32 environment (https://blogs.msdn.microsoft.com/oldnewthing/20170315-00/?p=95735) however using that code pretty much as is @ https://gist.github.com/daleharvey/c1e76edaa7abe548fd766391cfbad2f1 gives me a compile error @ https://bugzilla.mozilla.org/show_bug.cgi?id=1363169#c9

Any chance you could point me in the right direction here, not super familiar with any of the win32 intricacies

Cheers
Dale
Flags: needinfo?(jimm)
(Assignee)

Comment 12

2 months ago
Seems like Jim is busy, Chris found your name touching a few of the WindowsUtils files so wondering if you by any chance could help me here? Thanks
Flags: needinfo?(jimm) → needinfo?(cpeterson)
(Assignee)

Comment 13

2 months ago
Ok taking a shot at trying to do what UIViewSettings looks like its doing (copying and pasting over definitioned from windows kits)

New patch and new / less errors @ https://gist.github.com/daleharvey/147f1ff56f2a5114708c3c859a4ba470

Currently having problems finding the equivalent of https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp#L62 for IDataTransferManager

daleharvey@DESKTOP-UELI0OF /c/Program Files (x86)/Windows Kits/10/Include
$ grep -r "interface IDataTransferManager;" .
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.h:                interface IDataTransferManager;
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.h:interface IDataTransferManager;
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.idl:            interface IDataTransferManager;

Where those definitions are stubs and dont contain the MIDL_INTERFACE

Gijs since I was basing this off https://github.com/mozilla/gecko-dev/commit/6c7bdf65167060865d9f9a09c0aa3dae0d91680c I was wondering if you could help out here, am I on the right track and if so do you know where I could find the definitions? even googling the guid of the uiwindowssettings one doesnt give me anywhere
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dale Harvey (:daleharvey) from comment #12)
> Seems like Jim is busy, Chris found your name touching a few of the
> WindowsUtils files so wondering if you by any chance could help me here?

Sorry, Dale. I don't know anything about the Windows Share feature. <:)

Looks like Gijs is the best contact for this feature.

(In reply to :Gijs (he/him) from comment #4)
> Downgrading the priority here while we focus on 57 (and have to wait for
> help with the native stuff).

I'm bumping this bug's priority from P4 to P3 because we've shipped 57 and the current Bugzilla prioritization regime recommends using P3 or P5 instead of P4.
status-firefox57: affected → wontfix
status-firefox60: --- → wontfix
status-firefox61: --- → wontfix
status-firefox62: --- → affected
status-firefox-esr52: --- → wontfix
status-firefox-esr60: --- → wontfix
Flags: needinfo?(cpeterson)
OS: Unspecified → Windows
Priority: P4 → P3

Comment 15

2 months ago
The type of definition as used in https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp#L62 shouldn't be necessary anymore (in fact, maybe I should file a good first bug to remove it) because we now require the windows 10 sdk to build, whereas when that code was written we "only" required the win8 sdk.

As far as I can tell the confusion is as follows:

based on:

https://blogs.msdn.microsoft.com/oldnewthing/20170316-00/?p=95745

#include <shlobj.h> // IDataTransferManagerInterop

namespace dt = ABI::Windows::ApplicationModel::DataTransfer;

using Microsoft::WRL::Wrappers::HStringReference;

WRL::ComPtr<IDataTransferManagerInterop> g_dtmInterop;
WRL::ComPtr<DT::IDataTransferManager> g_dtm;

I can only assume that IDataTransferManagerInterop isn't actually in the 'dt' namespace. I also don't see it being in a namespace in either your fallback definition or in the definition in shobjidl.h

IDataTransferManagerStatics and IDataTransferManager look like they *are* in the `dt` namespace. Or something. Does that help make things work?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 16

2 months ago
> because we now require the windows 10 sdk to build,

Knowing that that was what that code was for and that its no longer needed is a huge help 

> I can only assume that IDataTransferManagerInterop isn't actually in the 'dt' namespace.

Without the <dt::IDataTransferManagerInterop> I still get "'IDataTransferManagerInterop': no GUID has been associated with this object" which seems to suggest it still doesnt find it correctly, but knowing that the .h files can be accessed I will keep on digging and see if I can figure out whats up. Seems strange that those examples are different

Cheers
(Assignee)

Comment 17

2 months ago
So yeh this is still confusing me, I have reduced the code down to a single definition

    Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;

And with either using #include <ShObjIdl.h> or #include <shlobj.h> I see 

 0:46.56 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2027: use of undefined type 'IDataTransferManagerInterop'
 0:46.59 c:\program files (x86)\windows kits\10\include\10.0.16299.0\um\shobjidl_core.h(1654): note: see declaration of 'IDataTransferManagerInterop'
 0:46.64 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(219): note: while compiling class template member function 'unsigned long Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void) throw()'
 0:46.67 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(281): note: see reference to function template instantiation 'unsigned long Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void) throw()' being compiled
 0:46.70 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): note: see reference to class template instantiation 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>' being compiled
 0:46.72 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2227: left of '->Release' must point to class/struct/union/generic type

I have found some other examples that use IDataTransferManagerInterop that use either of the above includes fine, I dont understand how it can find a definition that it cant use? Gijs hopefully this is my last need info here, know whats up with this?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

2 months ago
(In reply to Dale Harvey (:daleharvey) from comment #17)
> So yeh this is still confusing me, I have reduced the code down to a single
> definition
> 
>     Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;
> 
> And with either using #include <ShObjIdl.h> or #include <shlobj.h> I see 
> 
>  0:46.56 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2027: use of
> undefined type 'IDataTransferManagerInterop'
>  0:46.59 c:\program files (x86)\windows
> kits\10\include\10.0.16299.0\um\shobjidl_core.h(1654): note: see declaration
> of 'IDataTransferManagerInterop'
>  0:46.64 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(219): note: while compiling
> class template member function 'unsigned long
> Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void)
> throw()'
>  0:46.67 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(281): note: see reference to
> function template instantiation 'unsigned long
> Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void)
> throw()' being compiled
>  0:46.70
> c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207):
> note: see reference to class template instantiation
> 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>' being compiled
>  0:46.72 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2227: left of
> '->Release' must point to class/struct/union/generic type
> 
> I have found some other examples that use IDataTransferManagerInterop that
> use either of the above includes fine, I dont understand how it can find a
> definition that it cant use? Gijs hopefully this is my last need info here,
> know whats up with this?

I don't know, unfortunately... are you sure you have the right headers for the ComPtr stuff? Does including the headers in https://blogs.msdn.microsoft.com/oldnewthing/20030723-00/?p=43073 help? If neither of those help, :jimm is my goto person for the mysteries of windows API header/linkage woes...
Flags: needinfo?(jmathies)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dharvey)
(Assignee)

Comment 19

2 months ago
Yeh I tried using all includes from those 3 examples, also included anything I can see from examples @ https://github.com/search?l=C%2B%2B&q=IDataTransferManagerInterop&type=Code and still getting the same error, Cheers for looking
Flags: needinfo?(dharvey)
(Assignee)

Comment 20

2 months ago
Created attachment 8980217 [details] [diff] [review]
1363169.patch

Just upping the full patch for reference
(Assignee)

Comment 21

2 months ago
It seems like Jim is super busy, Mat I also got pointed in your direction and wondered if you had any idea what to do here? https://bugzilla.mozilla.org/show_bug.cgi?id=1363169#c17 is the sum of the question, what do I need to do to be able to use |Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;| in code without compile failures, thanks
Flags: needinfo?(mhowell)
The problem is that the definition of IDataTransferManagerInterop in shobjidl.h is wrapped in a _WIN32_WINNT version check, and we set that version to Windows 7 since that's the platform we usually need to be supporting; because of that you're only seeing a forward declaration, and trying to create pointers to an incomplete type is causing these cryptic errors. For cases like this where we're adding support for a feature that isn't in Windows 7 we sometimes copy the interface definition into our source files, and that's what I would recommend doing here rather than overriding the version define.
Flags: needinfo?(mhowell)
Flags: needinfo?(jmathies)
(In reply to Matt Howell [:mhowell] from comment #22)
> The problem is that the definition of IDataTransferManagerInterop in
> shobjidl.h is wrapped in a _WIN32_WINNT version check

That should say ShObjIdl_core.h; in the 10.0.15063 SDK the version check is on line 27157, and IDataTransferManagerInterop is defined starting from line 27255.
(Assignee)

Comment 24

26 days ago
Thanks for that Matt, so that fixed it for me, I can now get the share dialog to show however I cant seem to provide data to it

Uploaded new patch @ https://gist.github.com/daleharvey/f859f8785a1d752f82f8bd3c414c726b#file-gistfile1-txt-L184, I have the same code pretty much copy and pasted into a plain win32 app @ https://gist.github.com/daleharvey/2d9a21bdf2ed7329981ab12d08a3e5c9#file-test-cpp-L130 and I can compile and run this and it shows the relevant text in the share dialog, when I run the same code inside firefox, the callback gets called and completes (I can see the printf("Asked for data")), however the share dialog shows a loading screen then dissapears

I realise this is quite API specific but wondering if you had any ideas as I really dont know whats up from this point, Cheers
Flags: needinfo?(mhowell)
Yeah, I don't have any experience with this particular API, so my advice won't be exactly revelatory, sorry. I don't immediately see anything suspicious about your code (and it looks identical to a Raymond Chen example, which is normally a good sign). My first thought would be to log the return values from some of these calls, see if anything is giving you error codes. Since the dialog is opening first and _then_ something is going wrong, I'd probably focus on the callback to start with.
Flags: needinfo?(mhowell)
(Assignee)

Comment 26

23 days ago
Yup I found 2 examples, both worked in the native win32 test app and neither when inside Firefox. There isnt anything different in the return values and the callback completes to return S_OK; so I dont know what could be wrong here. 

Possibly a problem being called from the wrong process, or the interaction with GetForegroundWindow() is not what I am supposed to be using but it seems like it should be working and not sure where to look now.
(Assignee)

Comment 27

19 days ago
On recommendation from Mike I tried adding the ShareUrl method to nsDomWindowsUtils.cpp and accessing the HWND via 

    nsComPtr<nsIWidget> widget = GetWidget();
    HWND hwnd = (HWND)widget->GetNativeData(NS_NATIVE_WINDOW);

The callback code clearly works in the test code so it seems like something must be up with the context, however accessing the hwnd this way doesnt make any difference, the share dialog shows in a loading state momentarily then dissapears. So pretty stumped here, I have attached a console and see no errors being logged, callback complete fine ¯\_(ツ)_/¯
(Assignee)

Comment 28

19 days ago
And the most recent patch (mostly cleaned up previous versions) https://gist.github.com/daleharvey/ef84258cf33fc84605617084b4fdd618, If anyone wants the domwindowutils one I can post that too
Keywords: feature
This should be part of the release notes when the feature is ready.
relnote-firefox: --- → ?
I guess we are not targeting 62 anymore...
status-firefox62: affected → wontfix
status-firefox63: --- → affected
(Assignee)

Comment 31

4 hours ago
Ok so some microsoft devs have been helping me out with this, posting some of their comments below

> it looks like SharePickerUI is throwing an Access Denied 
> exception when trying to retrieve IDataContextChangedEventArgs::NewValue.

> I suspect Firefox is calling CoInitializeSecurity or has registry 
> configuration, and its AccessPermissions are not AppContainer-aware 
> (or maybe they are and intentionally excluding it, but not aware 
> that this can break some app-to-app scenarios).

I am going try to see what effect changing / removing 
https://dxr.mozilla.org/mozilla-central/source/ipc/mscom/MainThreadRuntime.cpp#233
does

and if that works maybe we can modify it to something that allows share to work
You need to log in before you can comment on or make changes to this bug.