Closed Bug 1363168 Opened 7 years ago Closed 6 years ago

Add support for native OS X "Share" feature

Categories

(Firefox :: Menus, enhancement, P4)

Product:
Firefox
Component:
Menus
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
relnote-firefox --- 61+
firefox57 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: Dolske, Assigned: daleharvey)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file)

Bug 1363168 - Add support for OSX Share feature.
59 bytes, text/x-review-board-request
Gijs
: review+
mstange
: review+
Details 
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 OS X, this appears to be move involved than simply launching an OS UI... Looks like the relevant API (NSSharingService) gives you the methods to get a list of providers (+ icons, titles, and if it can handle a particular type), so that the app can build a submenu itself.

https://developer.apple.com/reference/appkit/nssharingservice

Since we want to put these items into our own menu panel UI, we'll need some API that allows the Firefox frontend JS to build up this menu from what's available, and then launch a particular one upon user action.
Jim, can you help find an owner to implement this? (i.e. flip the markus-or-spohl coin ;).
Flags: needinfo?(jmathies)
Blocks: 1363180
Flags: needinfo?(jmathies)
Whiteboard: [photon-structure][tpi:+]
Flags: qe-verify?
Priority: -- → P2
Flags: qe-verify? → qe-verify-
Moving to reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-structure][tpi:+] → [reserve-photon-structure][tpi:+]
Priority: P3 → P4
Priority: P4 → P5
Priority: P5 → P3
Downgrading the priority here while we focus on 57 (and have to wait for help with the native stuff).
status-firefox57: affected → wontfix
Priority: P3 → P4
Gonna see if I can get this done before baby gets here
Assignee: nobody → dharvey
So this is a working prototype, right now I could use some guidance from someone familiar with webIdl + cocoa about what is the best way to implement the support for images (can idls return json and we use datauris?) as well transferring the id of the service between js and objc
Jared, was wondering if you had an idea about this or at least who would?

I think ideally GetSharingProviders would return [{title: 'Twitter', name: 'NSSharingServiceNamePostOnTwitter', image: '/filepath'}

Can idl return json / data structures? I can only see basic values and some stuffing them into strings @ https://dxr.mozilla.org/mozilla-central/source/browser/components/nsIBrowserHandler.idl, I could just use json either side and parse + stringify?

Can we get a filepath from NsImage? first time I have ever looked at objc however doesnt seem trivial from https://developer.apple.com/documentation/appkit/nsimage?language=objc, we could use base64 but is that going to be too much overhead for ipc?

Cheers
Dale
Flags: needinfo?(jaws)
You'll need to convert it to base64 data anyways to be able to load it as a (background-)image, so doing the conversion in obj-C would be best. NSImage is a pixel buffer, without metadata and the source doesn't even need to be a file on disk.
Stefan, can you help out Dave here?
Flags: needinfo?(jaws) → needinfo?(sarentz)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review233964


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser-pageActions.js:1055
(Diff revision 2)
> +    let shareProviders = JSON.parse(mwaUtils.getSharingProviders(url));
> +
> +    let bodyNode = panelViewNode.firstChild;
> +    const fragment = document.createDocumentFragment();
> +
> +    shareProviders.forEach(function (share) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: browser/base/content/browser-pageActions.js:1061
(Diff revision 2)
> +
> +      let item = document.createElement("toolbarbutton");
> +      item.setAttribute("label", share.title);
> +      item.setAttribute("share-name", share.title);
> +      item.setAttribute("image", share.image);
> +      item.classList.add('subviewbutton', 'subviewbutton-iconic');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/base/content/browser-pageActions.js:1061
(Diff revision 2)
> +
> +      let item = document.createElement("toolbarbutton");
> +      item.setAttribute("label", share.title);
> +      item.setAttribute("share-name", share.title);
> +      item.setAttribute("image", share.image);
> +      item.classList.add('subviewbutton', 'subviewbutton-iconic');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/base/content/browser-pageActions.js:1074
(Diff revision 2)
> +
> +      fragment.appendChild(item);
> +    });
> +
> +    // TODO: Probably not the right way to do this in chrome
> +    bodyNode.innerHTML = '';

Error: Strings must use doublequote. [eslint: quotes]
Ok updated to use JSON serialised as a string, I have the images transferred as base64 on the JSON. Right now the main thing I would like to do is cleanup how we refer to the service between cocoa and js.

These services have a name which they can be constructed with: 

    NSSharingService * service = [NSSharingService sharingServiceNamed:NSSharingServiceNamePostOnTwitter];

so ideally we return {'title': 'Twitter', 'image': 'data:image/png.....', 'shareName': 'NSSharingServiceNamePostOnTwitter'} and I can use that name to invoke in ShareUrl, however weirdly I cannot seem to find any way to get the serviceName from the NSSharingService object (which we get from sharingServicesForItems) someone more familiar with cocao may be able to figure out how? or we are going to have to either generate id's for them (lifecycle issues) or use the current way to use the title as an ID (seems fragile)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

In all my looking and asking seems impossible to get the serviceName from an instance of a NSSharedService so it seems like using the title is the best plan, we could store the services instead of looping over them in shareUrl but then we have lifecycle issues, happy to find anything better but currently the best I could figure out

Everything is currently bolted on to nsMacWebAppUtils which is probably not what we want, is there a better place for this or should it have its own idl

And the rest, I assume the objc isnt the best as its the first I have written, so general feedback on that
Attachment #8958442 - Flags: feedback?(sarentz)
Aaron could I get the share icon for this? cheers
Flags: needinfo?(abenson)
I *might* need a little more context about where this icon goes, but I'm pretty sure this is what you're looking for: https://design.firefox.com/icons/viewer/ (see Share 16 Macos).
Flags: needinfo?(abenson)
This is for the share url page action from the spec in https://bugzilla.mozilla.org/show_bug.cgi?id=1360054, the the windows share icon is the most similiar to the one in the spec, but will use the platform appropriate ones unless told otherwise, cheers
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

Just updated to use the icon, still looking for feedback
Attachment #8958442 - Flags: feedback?(sarentz)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review234582


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser-pageActions.js:1068
(Diff revision 4)
> +      fragment.appendChild(item);
> +    });
> +
> +    let bodyNode = panelViewNode.querySelector(".panel-subview-body");
> +    while (bodyNode.firstChild) {
> +      bodyNode.removeChild(bodyNode.firstChild);

Error: Use element.firstchild.remove() instead of element.removechild(element.firstchild) [eslint: mozilla/avoid-removeChild]
Actually since I am working on the windows version now, confirming this is probably a good idea, Aaron for https://bugzilla.mozilla.org/show_bug.cgi?id=1360054 are we going to use different icons the same thing between windows and mac, or use the same icon (bit simpler) if the same icon which one? Cheers
Flags: needinfo?(abenson)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

Having trouble finding anyone to take a look at this, Gijs can you (or point to someone who can)? cheers
Attachment #8958442 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to Dale Harvey (:daleharvey) from comment #24)
> Comment on attachment 8958442 [details]
> Bug 1363168 - Add support for OSX Share feature.
> 
> Having trouble finding anyone to take a look at this, Gijs can you (or point
> to someone who can)? cheers

I'll look at the JS side either tonight or tomorrow morning - apologies for the delay. For the macOS APIs, I expect :spohl, :sarentz, or :mstange would be appropriate choices.
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review235746

Generally, speaking, the approach of implementing stuff in a platform-specific component that we `getService` from the JS looks good, and the JS used to add the action looks OK to me too. I haven't yet tested this.

As usual, naming and code organization are the hardest problems in CS when there's a large project. I think nsIMacWebAppUtils isn't the right place - in fact, I see *nothing* using it (besides an automated test) so it probably should be removed, but that's a matter for a separate bug. Not 100% sure what the best place is, but if :mstange and/or :spohl OKs it, nsIMacDockUtils would work, otherwise nsIMacShellService, or (shudder) a new component. Probably not worth the fuss of that last option though.

::: browser/base/content/browser-pageActions.js:1032
(Diff revision 5)
>    },
>  };
> +
> +// share URL
> +BrowserPageActions.shareURL = {
> +

Nit: no blank line at the start/end of a block, here and below.

::: browser/base/content/browser-pageActions.js:1040
(Diff revision 5)
> +    let mwaUtils = Cc["@mozilla.org/widget/mac-web-app-utils;1"]
> +        .createInstance(Ci.nsIMacWebAppUtils);

Uh, so, I don't think nsIMacWebAppUtils is still very much alive... It hasn't been touched in like 4 years, and then for web-app-installation stuff that I think is now basically dead code on desktop platforms.

I'm not 100% sure where the best place will be. I expect it'd be either nsIMacDockUtils (for want of somewhere better, really) or nsIMacShellService (which is in browser/ which is why I'd prefer the other option). Hopefully :spohl/:mstange has more context here.

::: browser/base/content/browser-pageActions.js:1043
(Diff revision 5)
> +  onShowingSubview(panelViewNode) {
> +
> +    let mwaUtils = Cc["@mozilla.org/widget/mac-web-app-utils;1"]
> +        .createInstance(Ci.nsIMacWebAppUtils);
> +    let currentURI = gBrowser.selectedBrowser.currentURI.spec;
> +    let shareProviders = JSON.parse(mwaUtils.getSharingProviders(currentURI));

Are these likely to change? Maybe we can cache them instead of requesting + parsing them every time?

What happens if you pass an unusual URL, like "about:mozilla" or "blob:blah" or "moz-extension://..." ?

I'm guessing there should be code that disables this submenu if the current URI isn't either http(s) or (maybe) file - ftp if you're feeling generous. I expect other things won't work.

::: widget/cocoa/nsMacWebAppUtils.mm:116
(Diff revision 5)
> +
> +    [outArray addObject:dict];
> +  }
> +
> +  NSError *error = nil;
> +  NSData *json = [NSJSONSerialization dataWithJSONObject:outArray options:NSJSONWritingPrettyPrinted error:&error];

I take it there's no straightforward way to convert the dict into a plain JS object to avoid the JSON serialization/deserialization?

If we keep the JSON, I guess we don't need the `prettyprinted` option.... :-)


AFAICT this is storing an error object if things don't work, but I don't see any other error handling happening with that - there should probably be some?
Attachment #8958442 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs from comment #26)
> I'm not 100% sure where the best place will be. I expect it'd be either
> nsIMacDockUtils (for want of somewhere better, really) or nsIMacShellService
> (which is in browser/ which is why I'd prefer the other option). Hopefully
> :spohl/:mstange has more context here.

I think nsIShellService/ nsIMacShellService and ShellService.jsm is the best place for this, even though it's in browser/; the platform branches are a good match with what we need to do here and this doesn't really have anything to do with the Dock.
Flags: qe-verify- → qe-verify+
Updated the patch so it now passes a JSObject instead of a JSON string back from cocoa plus does some caching and other review fixes

Only thing I havent done yet is move this out of nsMacWebAppUtils, Markus I got pointed in your direction for advice on the best way to do this, I have some objc implemented @ https://reviewboard.mozilla.org/r/227408/diff/6 (currently in nsMacWebAppUtils.mm) but needs to be moved to somewhere appropriate, nsShellService has been recommended however there isnt currently a nsShellService.mm, should I make one and copy / convert the c++ across, or is there somewhere else appropriate I can put these objc functions? Thanks
Flags: needinfo?(mstange)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review239472

I think a nice place for this would be a new nsIMacSharingService interface + nsMacSharingService implementation. Adding a new service isn't very complicated; you should be able to mostly follow the "part 1" and "part 2" patches from bug 1037433.

For returning a JS object from an XPIDL method, you can follow what nsIProfiler.getProfileData does:

> [implicit_jscontext]
> jsval getProfileData([optional] in double aSinceTime);
> 
> NS_IMETHODIMP
> nsProfiler::GetProfileData(double aSinceTime, JSContext* aCx,
>                            JS::MutableHandle<JS::Value> aResult)
> {
>   mozilla::UniquePtr<char[]> profile = profiler_get_profile(aSinceTime);
>   if (!profile) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   NS_ConvertUTF8toUTF16 js_string(nsDependentCString(profile.get()));
>   auto profile16 = static_cast<const char16_t*>(js_string.get());
> 
>   JS::RootedValue val(aCx);
>   MOZ_ALWAYS_TRUE(JS_ParseJSON(aCx, profile16, js_string.Length(), &val));
> 
>   aResult.set(val);
>   return NS_OK;
> }

It uses JS_ParseJSON to turn a JSON string into a JS object.

In addition, for building up the JSON string, you could try using mozilla::JSONWriter, so that you don't have to go through an NSDictionary first.

::: widget/cocoa/nsMacWebAppUtils.mm:93
(Diff revision 5)
> +NSString* nsAtoNSString(const nsAString& str) {
> +  return [NSString stringWithCharacters:reinterpret_cast<const unichar*>(str.BeginReading())
> +                                 length:str.Length()];
> +}

You can use nsCocoaUtils::ToNSString instead.

::: widget/cocoa/nsMacWebAppUtils.mm:105
(Diff revision 5)
> +
> +  NSURL *url = [NSURL URLWithString:nsAtoNSString(urlToShare)];
> +
> +  NSArray *sharingService = [NSSharingService sharingServicesForItems:[NSArray arrayWithObject:url]];
> +
> +  NSMutableArray *outArray = [[NSMutableArray alloc]init];

This array needs to be released after you're done using it, otherwise it will leak. Rule of thumb: Every "alloc" needs to be paired with a "release".

::: widget/cocoa/nsMacWebAppUtils.mm:117
(Diff revision 5)
> +    [outArray addObject:dict];
> +  }
> +
> +  NSError *error = nil;
> +  NSData *json = [NSJSONSerialization dataWithJSONObject:outArray options:NSJSONWritingPrettyPrinted error:&error];
> +  NSString *jsonString = [[NSString alloc] initWithData:json encoding:NSUTF8StringEncoding];

This string will also need to be released if you're done with it.
> For returning a JS object from an XPIDL method, you can follow what nsIProfiler.getProfileData does:

hrm sorry I think theres some confusion about the mozreview links, this now returns a JSObject that I build directly instead of using NSDictionary https://reviewboard.mozilla.org/r/227408/diff/6

> I think a nice place for this would be a new nsIMacSharingService interface + nsMacSharingService implementation

Will do thanks, and thanks for the rest of the comments, will fix them up
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review235746

> Are these likely to change? Maybe we can cache them instead of requesting + parsing them every time?
> 
> What happens if you pass an unusual URL, like "about:mozilla" or "blob:blah" or "moz-extension://..." ?
> 
> I'm guessing there should be code that disables this submenu if the current URI isn't either http(s) or (maybe) file - ftp if you're feeling generous. I expect other things won't work.

So cocoa doesnt do any url validation here, they are basically treated as a string. Not sure we need to do any url validation ourselves as we already hide the page action menu in some circumstantes (about urls) otherwise it will just proceed to share the url. I can add that code if you still think so but didnt seem necessary.

I added in caching for when the panel is shown so if the user clicks share -> back -> share, we dont go back and forth with cocoa and redrawing etc, but it gets invalidated when the panel is shown so if the user installed a new share service etc it will definitely be picked up.
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review239732

JS looks OK, I haven't looked at the native part much. But this should have a test! Use a `run-if` condition to make it only run on mac. If you want to be really good, it should have an xpcshell test for the cocoa bit (somewhere in widget/, I expect) and a browser test for the browser bit (in browser/base/content/test/urlbar/ )

::: browser/base/content/browser-pageActions.js:1280
(Diff revision 7)
> +  },
> +
> +  onShowingSubview(panelViewNode) {
> +    // We cache the providers + the UI if the user selects the share
> +    // panel multiple times while the panel is open.
> +    if (this.redraw === false) {

That this depends on strict comparison rather than loose comparison is basically a code smell. Can you invert the bool, so that it's set to false by `onShowingInPanel` and to true by this code, and then just check `if (this.cached)` ?

::: browser/base/content/browser-pageActions.js:1284
(Diff revision 7)
> +    let sharingService = Cc["@mozilla.org/widget/macsharingservice;1"]
> +        .createInstance(Ci.nsIMacSharingService);

If it says "service", use `getService` to get it, rather than `createInstance`.

::: browser/base/content/browser-pageActions.js:1291
(Diff revision 7)
> +    let currentURI = gBrowser.selectedBrowser.currentURI.spec;
> +    let shareProviders = sharingService.getSharingProviders(currentURI);
> +    let fragment = document.createDocumentFragment();
> +
> +    shareProviders.forEach(function(share) {
> +

Nit: no empty line at the start of a block (I'd expect eslint to pick this up these days...)

::: widget/cocoa/nsMacWebAppUtils.mm:12
(Diff revision 7)
> +#include "mozilla/MacStringHelpers.h"
> +

I assume this is to fix unified build bustage?
Attachment #8958442 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review240304

Looks good, except for some code style issues:
 - Please make all parameters have the form aParameter
 - Please follow the form "Typename* variable" for all pointers (i.e. asterisk hugging the type name)
 - Make the local functions static and use the following formatting:

static ReturnType
FunctionNameWithAnUppercaseFirstLetter(args)
{

 - Similarly, for methods:

ReturnType / NS_IMETHODIMP
ClassName::MethodName(args)
{

 - And add line breaks so that the lines with the argument lists aren't too long; ideally < 80 chars but < 100 is fine if 80 is too hard.
Attachment #8958442 - Flags: review?(mstange)
Clearing needinfos as got review feedback
Flags: needinfo?(sarentz)
Flags: needinfo?(mstange)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review239732

> I assume this is to fix unified build bustage?

Nope was left over, good catch cheers
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

Clearing the browser review as I am going to update the test for this
Attachment #8958442 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

Seen some issues on try run, will fix before review
Attachment #8958442 - Flags: review?(mstange)
Attachment #8958442 - Flags: review?(gijskruitbosch+bugs)
(In reply to Dale Harvey (:daleharvey) from comment #18)
> This is for the share url page action from the spec in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1360054, the the windows share
> icon is the most similiar to the one in the spec, but will use the platform
> appropriate ones unless told otherwise, cheers

Hey Dale, we should be using the Photon share icons which are provided here: https://design.firefox.com/icons/viewer/#share

Sorry for the late reply.
Flags: needinfo?(abenson)
Still leaving the review since the issue is small, but still having a problem with the test, inexplicably this causes browser_search_favicon.js (subsequent) test to file as when these tests run in a row, the favicon test shows an extra "Search with SearchEngine" in the url bar (as can be seen in https://imgur.com/a/SaASt top=broken, bottom=what should happen)

I have no idea how my test could cause this (I have tried clearing history, using data urls etc) but it does so reliably
Still investigating, but if I change 

    PanelMultiView.hidePopup(BrowserPageActions.panelNode);

to

    BrowserPageActions.panelNode.hidePopup();


in |browser-pageActions.js| then all seems fine
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review241346

Nice. Some comments below, but generally looks fine.

::: browser/base/content/browser-pageActions.js:1269
(Diff revision 10)
> +  get _sharingService() {
> +    return Cc["@mozilla.org/widget/macsharingservice;1"]
> +      .getService(Ci.nsIMacSharingService);
> +  },

Nit: please make this a lazy getter using XPCOMUtils.defineLazyServiceGetter.

::: browser/base/content/browser-pageActions.js:1291
(Diff revision 10)
> +    if (this._cached) {
> +      return;
> +    }
> +
> +    let sharingService = this._sharingService;
> +    let currentURI = gBrowser.selectedBrowser.currentURI.spec;

Sorry, I should have thought about this before now. We should be using the same logic we use for when we copy URLs out of the URL bar, or email them. See also bug 1366327. You should be able to use:

```js
gURLBar.makeURIReadable(gBrowser.selectedBrowser.currentURI).displaySpec
```

to do this. This ensures that we pass a 'pretty' URL for IDN, and don't pass the about:reader? part for reader mode pages, and a few other things to do with URI escaping.

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:1
(Diff revision 10)
> +"use strict";

Nit: add public domain license header please.

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:6
(Diff revision 10)
> +// Keep track of title of service we chose to share with
> +let shareTitle;

Perhaps would be good to check the shared URL too?
Attachment #8958442 - Flags: review?(gijskruitbosch+bugs) → review+
Ok many thanks to Gijs, the test failure was due to the mouse position affecting the favicon test, I have both reset the mouse position here and added a reet to the affected test so it should be a little less fragile in future, fixed the rest of the review comments too
Comment on attachment 8958442 [details]
Bug 1363168 - Add support for OSX Share feature.

https://reviewboard.mozilla.org/r/227408/#review241596

::: widget/cocoa/nsMacSharingService.mm:15
(Diff revision 11)
> +#include "mozilla/MacStringHelpers.h"
> +
> +NS_IMPL_ISUPPORTS(nsMacSharingService, nsIMacSharingService)
> +
> +static NSString*
> +NSImageToBase64(const NSImage* aImage) {

This opening brace still needs to move into its own line. Same for the other three instances of this problem in this file.
Attachment #8958442 - Flags: review?(mstange) → review+
Thanks for the reviews, fixed the nits and just making sure this has a green run before landing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=228f956214ff1caedab58f006b3ca8636c2eec14
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ced1f03536a4
Add support for OSX Share feature. r=Gijs,mstange
https://hg.mozilla.org/mozilla-central/rev/ced1f03536a4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
When I right click on share in the menu and "Add to Address Bar," the resulting spot in the Address Bar has no icon.

Screenshot: https://screenshots.firefox.com/mRrGL6UlmV5is1Pm/null
Depends on: 1454705
Dale, could you request an addition to release notes? You have instructions on this process  documented here:
https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Thanks!
Flags: needinfo?(dharvey)
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]: Nope
[Suggested wording]: Add the ability to share current URL with MacOS sharing providers
[Links (documentation, blog post, etc)]:
I made little video @ https://twitter.com/daleharvey/status/985822146210430976
relnote-firefox: --- → ?
Flags: needinfo?(dharvey)
Not sure if this should spin off into a new bug or not, but the spec included a "••• More..." option at the end of the list of share providers. More takes you to macOS system prefs and helps people to understand how that list is populated. Dale said he would look into whether this was part of the API.
(In reply to Aaron Benson from comment #57)
> Not sure if this should spin off into a new bug or not, but the spec
> included a "••• More..." option at the end of the list of share providers.
> More takes you to macOS system prefs and helps people to understand how that
> list is populated. Dale said he would look into whether this was part of the
> API.

Good point; this is good fodder for a follow-up bug, especially since this bug is resolved already.
Depends on: 1455035
Depends on: 1455304
Depends on: 1455307
Depends on: 1455310
(In reply to Dale Harvey (:daleharvey) from comment #56)
> Release Note Request (optional, but appreciated)
> [Why is this notable]:
> [Affects Firefox for Android]: Nope
> [Suggested wording]: Add the ability to share current URL with MacOS sharing
> providers
> [Links (documentation, blog post, etc)]:
> I made little video @
> https://twitter.com/daleharvey/status/985822146210430976

Thank you, this was added to Nightly release notes for 61. For beta and final release notes, our release managers will decide on the potential inclusion and final wording, hence keeping the flag as 'relnote ?'.
Hello Dale,

I would like to verify this issue, but I need some more information.

Firstly, which share methods are intended to be included in the feature? (In comment 56, you made a video with the implemented share method and you seem to have some extra options: "Add to reading list" and "Mail". Furthermore, the old implementation seen here had a few more options as well: CloudApp and others hidden under the "More..." button.)

Secondly, should I only confirm this issue after all related issues are fixed?

Thanks
Flags: needinfo?(dharvey)
Hey Bodea

> Firstly, which share methods are intended to be included in the feature?

This list is provided by OSX, the default is messages, airdrop, notes, reminders, twitter, facebook, linkedin. OSX could change these by location and users can install their own share providers (can be configured in preferences -> extensions -> share menu), "Add to reading list" and "Mail" are 2 built in providers that we filter out and should not appear

> Secondly, should I only confirm this issue after all related issues are fixed?
Yes likely best to confirm this at least after https://bugzilla.mozilla.org/show_bug.cgi?id=1455310 lands
Flags: needinfo?(dharvey)
I have verified the fix with the reminders type share (1455310) and I have validated the functionality of the other default share types as well.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
status-firefox62: --- → verified
See Also: → 1462974
Depends on: 1512851
Blocks: 1716528
No longer blocks: 1716528
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: