[User story] As a desktop client user I want to share a conversation URL through my favorite social network so it's easy to share

VERIFIED FIXED in Firefox 39

Status

Hello (Loop)
Client
P1
enhancement
Rank:
9
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: RT, Assigned: mikedeboer)

Tracking

(Depends on: 3 bugs)

unspecified
mozilla40
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ verified, firefox40+ verified, relnote-firefox 39+)

Details

(Whiteboard: [social][strings])

User Story

As a desktop client user I want to share a conversation URL through my favorite social network so it's easy to share.

UX: https://bugzilla.mozilla.org/attachment.cgi?id=8569938

Acceptance criteria:
* A new "Share button" is available in the conversation window when waiting alone in a conversation
* Using the share button opens a menu listing all share providers which have already been added and an option to add a new share provider. If no providers have been added yet only the option to add a new provider is shown.
* Clicking the share button again closes the menu
* Selecting the option to add a new provider if the "Firefox share" button is on the toolbar or on the panel menu will open the share panel.
* Selecting the option to add a new provider if the "Firefox share" button is on the customize palette will open a dialogue with the user offering to add "Firefox share" to the toolbar. 
- If the user acknowledges the dialog the "Firefox share" button gets added to the toolbar and the "Firefox share" panel opens
- If the user dismisses the dialog nothing happens when the dialogue closes
* The panel opens with a pre-populated message used for the selected provider and the message can be shared. The URL of the conversation is the URL getting shared through that mechanism.
* The conversation window remains opened whilst sharing action occurs on the sharing panel
* A Telemetry event sent each time the "Share button" gets used

* TBD: text and image being shared through the different providers
Note: the position of the "Firefox share3 icon in Fx39 has yet to be confirmed

Attachments

(7 attachments, 10 obsolete attachments)

15.75 KB, application/zip
Details
2.04 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
20.68 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
7.92 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
53.38 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
18.18 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
2.32 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

3 years ago
User Story: (updated)
(Reporter)

Updated

3 years ago
Depends on: 1128101
(Reporter)

Updated

3 years ago
No longer depends on: 1128101
(Reporter)

Updated

3 years ago
Depends on: 1128101
(Reporter)

Updated

3 years ago
Depends on: 1134133
(Reporter)

Updated

3 years ago
User Story: (updated)
(Reporter)

Updated

3 years ago
User Story: (updated)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1144628
(Assignee)

Updated

3 years ago
Assignee: nobody → mdeboer
Severity: normal → enhancement
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Points: --- → 8
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
(Assignee)

Updated

3 years ago
User Story: (updated)
(Assignee)

Updated

3 years ago
Whiteboard: [strings]

Updated

3 years ago
Rank: 9
Whiteboard: [strings] → [social][strings]
(Assignee)

Comment 2

3 years ago
Created attachment 8579471 [details] [diff] [review]
WIP: feature complete patch
(Assignee)

Comment 3

3 years ago
Hi Michael! Can you provide two SVG and one PNG assets for me, based on https://bugzilla.mozilla.org/attachment.cgi?id=8569938:

1) SVG: Icon for 'Add a Service' menu item in the share menu (a 'Plus' glyph)
2) SVG: Icon for inside the share menu/ panel when the Share button is not available (see bottom part of the mockup)
3) PNG: Hello logo that can serve as a preview image for the Hello link clicker as shown in the Facebook share panel.

Thanks!
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 4

3 years ago
Hi Shane, I have three questions for you wrt OpenGraph data:

1) For G+, LinkedIn and Facebook sharing (perhaps others as well), you can see a short blurb that is put next to the URL preview image and below the URL title that contains an excerpt of the page that belongs to the URL. Can we manipulate that text to display something we define specifically (thus can also localize) through the OpenGraph data that we pass to `SocialShare.sharePage()`?
2) If we pass a preview image in the OpenGraph data, will that show up as the image next to the blurb I mentioned above?
3) For the GMail sharing provider, can we manipulate the email body text to be different than the title we supply via the OpenGraph data?

Thanks!
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 5

3 years ago
Created attachment 8580749 [details] [diff] [review]
Patch 1: share button should emit notifications when it is added to or removed from a customizable area
Attachment #8580749 - Flags: review?(mixedpuppy)
(Assignee)

Comment 6

3 years ago
Created attachment 8580750 [details] [diff] [review]
Patch 2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Shane, can you review if I'm interacting with Social API correctly here?
Attachment #8580750 - Flags: review?(standard8)
Attachment #8580750 - Flags: review?(mixedpuppy)
(Assignee)

Comment 7

3 years ago
Created attachment 8580751 [details] [diff] [review]
Patch 3: hide the Loop dropdowns when the content window loses focus
Attachment #8580751 - Flags: review?(standard8)
(Assignee)

Comment 8

3 years ago
Created attachment 8580754 [details] [diff] [review]
Patch 4: add a Share Link button in the Loop conversation window

Mark, just a feedback request for now, because this patch is missing some assets I requested from Michael. But adding those will not change the architecture here, unless you suggest another.
Attachment #8580754 - Flags: feedback?(standard8)
(Assignee)

Comment 9

3 years ago
Test coverage for part/ patch 4 is coming up in a part 5.
(Assignee)

Comment 10

3 years ago
Patch 4 also fixes the mis-aligned icons for the dropdown menus in the panel.
Comment on attachment 8580749 [details] [diff] [review]
Patch 1: share button should emit notifications when it is added to or removed from a customizable area

As far as the patch goes, r+.  

I'm curious why we would add these rather than just use the CUI listener in the loop code where you want to get the notifications.
Flags: needinfo?(mixedpuppy)
Attachment #8580749 - Flags: review?(mixedpuppy) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Hi Shane, I have three questions for you wrt OpenGraph data:
> 
> 1) For G+, LinkedIn and Facebook sharing (perhaps others as well), you can
> see a short blurb that is put next to the URL preview image and below the
> URL title that contains an excerpt of the page that belongs to the URL. Can
> we manipulate that text to display something we define specifically (thus
> can also localize) through the OpenGraph data that we pass to
> `SocialShare.sharePage()`?
> 2) If we pass a preview image in the OpenGraph data, will that show up as
> the image next to the blurb I mentioned above?
> 3) For the GMail sharing provider, can we manipulate the email body text to
> be different than the title we supply via the OpenGraph data?
> 
> Thanks!

Some of these we can modify the text in the call to share.  Others we may need to have a page off the services website that has the opengraph tags for the share services to consume.  We should be able to easily manipulate the body text for GMail.
Attachment #8580750 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 13

3 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> Some of these we can modify the text in the call to share.  Others we may
> need to have a page off the services website that has the opengraph tags for
> the share services to consume.  We should be able to easily manipulate the
> body text for GMail.

Alright, cool! Do you mean that I'd have to inspect the source of the respective share providers' share URL and see how they consume the OpenGraphData event?
Flags: needinfo?(mixedpuppy)
(In reply to Mike de Boer [:mikedeboer] from comment #13)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> > Some of these we can modify the text in the call to share.  Others we may
> > need to have a page off the services website that has the opengraph tags for
> > the share services to consume.  We should be able to easily manipulate the
> > body text for GMail.
> 
> Alright, cool! Do you mean that I'd have to inspect the source of the
> respective share providers' share URL and see how they consume the
> OpenGraphData event?

None of the major sites use the event.  The OpenGraph data is also used to generate a url from a url template.  What data the various share providers use in the url varies from provider to provider.  Facebook only has the "url" in the template, which means Facebook fetches the url you give them, looks at og meta tags to get any text, images, etc.  GMail has a body tag where we can define what's in the email, some providers have title, etc.  If it will help to chat, grab me on irc.
Flags: needinfo?(mixedpuppy)
Comment on attachment 8580750 [details] [diff] [review]
Patch 2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Review of attachment 8580750 [details] [diff] [review]:
-----------------------------------------------------------------

Its a little strange that the share button is disabled on pages like about:home, especially when it makes sense to start up FF and attempt to jump straight into a conversation. It certainly confused me when I started FF and then tried to share the conversation but found the button disabled.

Please could you file a bug about that for UX to consider (or ensure there is one)

::: browser/components/loop/MozLoopAPI.jsm
@@ +899,5 @@
> +     *
> +     * @return {Boolean} `true` if the widget is available and `false` when it's
> +     *                   still in the Customization palette.
> +     */
> +    isSocialShareButtonAvailable: {

I'm partially thinking we should put these functions in their own sub-namespace, but I'm not sure its actually worth it.

@@ +968,5 @@
> +
> +    /**
> +     * Returns a sorted list of Social Providers that can share URLs. See
> +     * `updateSocialProvidersCache()` for more information.
> +     * 

nit: trailing whitespace

::: browser/components/loop/test/mochitest/head.js
@@ +70,5 @@
>      });
>    });
>  }
>  
> +function waitForCondition(condition, nextTest, errorMsg) {

I wonder if we shouldn't start filing bugs on the core testing framework to get these harmonised in one location rather than copy/pasting them all the time.
Attachment #8580750 - Flags: review?(standard8) → review+
Attachment #8580751 - Flags: review?(standard8) → review+
Comment on attachment 8580754 [details] [diff] [review]
Patch 4: add a Share Link button in the Loop conversation window

Review of attachment 8580754 [details] [diff] [review]:
-----------------------------------------------------------------

In general looks good. I'm not quite sure about the roomStore additions, I almost think they should be to activeRoomStore, but I can also see why you put them there.
Attachment #8580754 - Flags: feedback?(standard8) → feedback+
Created attachment 8581848 [details]
Hello_New_Assets.zip

Updated assets attached.
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 18

3 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Comment on attachment 8580749 [details] [diff] [review]
> Patch 1: share button should emit notifications when it is added to or
> removed from a customizable area
> 
> As far as the patch goes, r+.  
> 
> I'm curious why we would add these rather than just use the CUI listener in
> the loop code where you want to get the notifications.

This is because the Loop window(s), especially the conversation ones have a lifetime that is not connected to any browser window. The CustomizableUI object is, so when a window is closed, the listener(s) will be removed as well. If the chat window is undocked, it will continue to exist even though its opener window may be gone.
That's why I preferred to rely on global notifications, rather than window-scoped event listeners.
(Assignee)

Comment 19

3 years ago
Created attachment 8582438 [details] [diff] [review]
Patch 2.1: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Added support for the `body` graphData property for GMail - see comment 14.
Attachment #8580750 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Comment on attachment 8582438 [details] [diff] [review]
Patch 2.1: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Carrying over r=Standard8,mixedpuppy
Attachment #8582438 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8582446 [details] [diff] [review]
Patch 4.1: add a Share Link button in the Loop conversation window

 - Added support for the `body` graphData for GMail
 - Added support for dropdown anchoring in the mixin
 - And a whole lot of other goodies

Currently working on the tests for this part (will be Patch 5).
Attachment #8580754 - Attachment is obsolete: true
Attachment #8582446 - Flags: review?(standard8)
(Assignee)

Comment 22

3 years ago
Created attachment 8583129 [details] [diff] [review]
Patch 5: unit tests covering patch 4
(Assignee)

Updated

3 years ago
Attachment #8579471 - Attachment is obsolete: true
Comment on attachment 8582446 [details] [diff] [review]
Patch 4.1: add a Share Link button in the Loop conversation window

Review of attachment 8582446 [details] [diff] [review]:
-----------------------------------------------------------------

Testing this against latest fx-team, I'm seeing a couple of issues:

- The "Invite somone to join you" text is overlapped by the link action buttons
- There's a scroll bar which shows blank space to the right of the video.

::: browser/components/loop/content/js/roomViews.jsx
@@ +83,5 @@
> +        return provider.origin == origin;
> +      })[0];
> +
> +      // XXXmikedeboer: this might later contain the roomToken to retrieve more
> +      //                context details to share.

I'm not sure we need this comment unless we're positive its going to happen, if so, then we should file a bug for it and reference it here.

@@ +92,5 @@
> +      }));
> +    },
> +
> +    render: function() {
> +      // Don't render a thing yet when no data has been fetched yet.

nit: drop the first yet.

::: browser/components/loop/content/shared/img/icons-16x16.svg
@@ +164,5 @@
>        h-8.3C1.65,2,1,2.68,1,3.52v6.24c0,0.83,0.65,1.51,1.45,1.51h0.52V5.43c0-0.84,0.65-1.51,1.45-1.51h6.61l-0.81,
>        0.81H5.25 c-0.8,0-1.45,0.68-1.45,1.51v4.91l-1.89,1.89l0.99,0.99l1-1l8.3-8.3L14.21,2.72z"/>
>    </g>
>  </defs>
> +<use id="add"                 xlink:href="#add-shape"/>

Please add the new icons (add + share) to the ui-showcase.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +286,5 @@
>      /**
> +     * Handles an update of the position of the Share widget, notified by the
> +     * mozLoop API.
> +     */
> +    _handleShareWidgetUpdate: function() {

I think we should make these new handle functions dispatch actions (like we do with _handleRoom{Update,Delete} already).

The reason is this gives us consistency - information coming in from external interfaces is always handled as actions, hence we can more easily track it and side effects are less likely.

I'm also thinking that in future if we want to split out some of these mozLoop interfaces or create multiple stores (due to complexity), then having everything going through actions already will make that easier.

::: browser/components/loop/standalone/content/index.html
@@ +137,5 @@
>        }, false);
>      </script>
> +
> +    <noscript>
> +      <img src="img/logo.png" border="0" alt="Logo"/>

I can't see any reason to have this in this bug. I think we should split it out somewhere else. On my browser its also shown half way down the page (scrolled off the bottom). I'm also tempted to say we should have some default noscript text here even if its english only.
Attachment #8582446 - Flags: review?(standard8) → review-
(Assignee)

Comment 24

3 years ago
(In reply to Mark Banner (:standard8) from comment #23)
> Testing this against latest fx-team, I'm seeing a couple of issues:
> 
> - The "Invite somone to join you" text is overlapped by the link action
> buttons

I don't see this. Odd. How can I reproduce this?

> - There's a scroll bar which shows blank space to the right of the video.

The above might be due to this issue. I added `body { overflow: hidden }` at some point, but removed it from this patch. It seems I need to re-add it!

> I can't see any reason to have this in this bug. I think we should split it
> out somewhere else. On my browser its also shown half way down the page
> (scrolled off the bottom). I'm also tempted to say we should have some
> default noscript text here even if its english only.

I added it to this patch, because the Facebook & G+ share providers fetch the page on their servers and prune it for content to use in the preview that is shown on the timeline after publishing the intent.
We can add a generic intro text in the <noscript> block too, but that one won't be localizable without pre-processing on the server that serves the LC page. I think that's something we should discuss and define in a separate bug.
The fact that it's shown half way down the page is not an issue. It's essential only that the image is there for a server side script to find, not for the user to see.
(Assignee)

Comment 25

3 years ago
Created attachment 8583773 [details] [diff] [review]
Patch 4.2: add a Share Link button in the Loop conversation window

Patch with comments addressed.
Attachment #8582446 - Attachment is obsolete: true
Attachment #8583773 - Flags: review?(standard8)
(Assignee)

Comment 26

3 years ago
Created attachment 8583774 [details] [diff] [review]
Patch 5.1: unit tests covering patch 4
Attachment #8583129 - Attachment is obsolete: true
Attachment #8583774 - Flags: review?(standard8)
Comment on attachment 8583773 [details] [diff] [review]
Patch 4.2: add a Share Link button in the Loop conversation window

Review of attachment 8583773 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the note about the noscript image. That helps explain why.

r=Standard8
Attachment #8583773 - Flags: review?(standard8) → review+
Comment on attachment 8583774 [details] [diff] [review]
Patch 5.1: unit tests covering patch 4

Review of attachment 8583774 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple of minor nits. r=Standard8

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +452,5 @@
>    });
> +
> +  describe("SocialShareDropdown", function() {
> +    var view;
> +    var fakeProvider = {

nit: normally we initialise these inside the beforeEach to prevent issues with tests accidentally modifying them and having unexpected side-effects on other tests.

::: browser/components/loop/test/shared/roomStore_test.js
@@ +73,5 @@
>        rooms: [],
>        activeRoom: {}
>      };
>  
> +    var mozL10n = document.mozL10n || navigator.mozL10n;

This doesn't seem to be doing anything, I've tried removing it and tests still pass without.
Attachment #8583774 - Flags: review?(standard8) → review+
(Assignee)

Comment 32

3 years ago
Pushed strings only. *sigh*

https://hg.mozilla.org/integration/fx-team/rev/b9caf6715b7e
Keywords: leave-open

Comment 34

3 years ago
FTR, I don't see why this should preland strings, and not just ride the trains when it's ready.
(Assignee)

Updated

3 years ago
Iteration: 39.2 - 23 Mar → 40.1 - 13 Apr
Blocks: 1084291
(Assignee)

Comment 35

3 years ago
Shane, no matter what I try, I keep failing mochitests on our infra - not locally!! - for Patch 2.
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb2a808e1a6 for an example.

As you can see, it consistently leaks _two_ 'about:mozilla' windows, which I have no idea where they're coming from. Do you have any ideas?
Flags: needinfo?(mixedpuppy)
Only place I see you using about:mozilla is in browser_mozLoop_sharingListener.js.  Tabs are added and removed, but you don't wait on the removal of the tabs, I would try that.  I also see various other js failures in there, such as https connection failures with loop, not sure at what point it is happening or whether it would be a contributing factor.

05:48:25     INFO -  578 INFO Console message: [JavaScript Error: "An error occurred during a connection to fail-handshake.example.com:443.
05:48:25     INFO -  SSL received an unexpected Client Hello handshake message.
05:48:25     INFO -  (Error code: ssl_error_rx_unexpected_client_hello)

I'm out today, can look further next week if you need help.
Flags: needinfo?(mixedpuppy)
With leaks like this I often try --repeat 20 or something to see if it will force a leak.  Tried that here, but other problems crop up.

Running with ."/mach mochitest-browser --repeat 2 browser/components/loop/test/mochitest/" I get a number of errors.  kShareProvider isn't removed in the new test file (could cause more issues if socialapi tests were run after loop tests), and some errors in browser/components/loop/test/mochitest/browser_toolbarbutton.js  (which I didn't look into)

This fixes the provider removal:

registerCleanupFunction(function() {
  yield new Promise(resolve => SocialService.disableProvider(kShareProvider.origin, resolve));
  yield new Promise(resolve => SocialService.disableProvider(kShareProviderInvalid.origin, resolve));
  Assert.strictEqual(Social.providers.length, 0, "all providers should be removed");
  SocialShare.uninit();
});

IMO you can also remove the worker script from kShareProvider, shouldn't be necessary for your tests. 


There is also lots of these in the output:

JavaScript error: resource:///modules/loop/MozLoopAPI.jsm, line 1059: TypeError: targetWindow.CustomEvent is not a constructor

I'm not able to reproduce the leak of about:mozilla windows from browser_mozLoop_sharingListener.js.  I suspect you need to wait on the tab removal, ensure the leak is not from that test.  e.g.

gBrowser.removeTab(gBrowser.selectedTab);
gBrowser.tabContainer.addEventListener("TabClose", function onTabClose() {
  gBrowser.tabContainer.removeEventListener("TabClose", onTabClose);
  // continue after this
});
Depends on: 1152197
(Assignee)

Comment 38

3 years ago
Shane, thanks so much for that analysis! I'll be off trying these things and more. Super helpful.

Mark, I think you linked the wrong bug just now...
(Assignee)

Comment 39

3 years ago
owait, you didn't. oops.
(Assignee)

Comment 40

3 years ago
Try push with latest updates. Fingers crossed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1c1a2029e7
(Assignee)

Updated

3 years ago
User Story: (updated)
(Assignee)

Updated

3 years ago
User Story: (updated)
Mike it looks like some of this landed before 39 merged to aurora, and some after.  Is 38 affected at all? Thanks.
status-firefox38: --- → ?
status-firefox39: --- → affected
status-firefox40: --- → affected
tracking-firefox39: --- → +
tracking-firefox40: --- → +
Flags: needinfo?(mdeboer)
interesting that didn't show up in try.  @mdeboer, make a copy of share.html for your tests and remove the port related lines in the event handler, they are not necessary for your tests, and [I'm pretty certain] are the culprit here.
(Assignee)

Comment 47

3 years ago
(In reply to Liz Henry (:lizzard) from comment #43)
> Mike it looks like some of this landed before 39 merged to aurora, and some
> after.  Is 38 affected at all? Thanks.

Hi Liz, Fx38 is not affected. This feature is targeted for Fx39.
(Assignee)

Comment 48

3 years ago
Created attachment 8590481 [details] [diff] [review]
Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area
Attachment #8580749 - Attachment is obsolete: true
Attachment #8590481 - Flags: review+
(Assignee)

Comment 49

3 years ago
Created attachment 8590482 [details] [diff] [review]
Patch 2.2: add navigator.mozLoop methods to allow interaction between Loop and the Social API
Attachment #8582438 - Attachment is obsolete: true
Attachment #8590482 - Flags: review+
(Assignee)

Comment 50

3 years ago
Created attachment 8590483 [details] [diff] [review]
Patch 3.1: hide the Loop dropdowns when the content window loses focus
Attachment #8580751 - Attachment is obsolete: true
Attachment #8590483 - Flags: review+
(Assignee)

Comment 51

3 years ago
Created attachment 8590486 [details] [diff] [review]
Patch 4.3: add a Share Link button in the Loop conversation window
Attachment #8583773 - Attachment is obsolete: true
Attachment #8590486 - Flags: review+
(Assignee)

Comment 52

3 years ago
Created attachment 8590490 [details] [diff] [review]
Patch 5.2: unit tests covering patch 4
Attachment #8583774 - Attachment is obsolete: true
Attachment #8590490 - Flags: review+
status-firefox38: ? → unaffected
(Assignee)

Comment 56

3 years ago
Created attachment 8591577 [details] [diff] [review]
Patch 6: allow data: URIs as img source in Loop documents

Forgot to add this :/ This patch make the favicons for the social providers show up.
Attachment #8591577 - Flags: review?(standard8)
(Assignee)

Comment 57

3 years ago
Comment on attachment 8590481 [details] [diff] [review]
Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello new Social Share feature
[User impact if declined]: It will become significantly easier for a user to share a room URL through the Social API provided methods of sharing via enabled providers, like Twitter, Facebook, etc.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests added and passing.
[Risks and why]: moderate, this is a new feature.
[String/UUID change made/needed]: n/a, strings have already landed during the regular Fx39 cycle.
Attachment #8590481 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 58

3 years ago
Comment on attachment 8590482 [details] [diff] [review]
Patch 2.2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Please see comment 57 for the approval request.
Attachment #8590482 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 59

3 years ago
Comment on attachment 8590483 [details] [diff] [review]
Patch 3.1: hide the Loop dropdowns when the content window loses focus

Please see comment 57 for the approval request.
Attachment #8590483 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 60

3 years ago
Comment on attachment 8590486 [details] [diff] [review]
Patch 4.3: add a Share Link button in the Loop conversation window

Please see comment 57 for the approval request.
Attachment #8590486 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 61

3 years ago
Comment on attachment 8590490 [details] [diff] [review]
Patch 5.2: unit tests covering patch 4

Please see comment 57 for the approval request.
Attachment #8590490 - Flags: approval-mozilla-aurora?
Depends on: 1127493
Comment on attachment 8591577 [details] [diff] [review]
Patch 6: allow data: URIs as img source in Loop documents

Review of attachment 8591577 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, I can't think of any issues enabling these.
Attachment #8591577 - Flags: review?(standard8) → review+
(Assignee)

Comment 63

3 years ago
Thanks! Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/a3f65d0021f7
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a3f65d0021f7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 65

3 years ago
Comment on attachment 8591577 [details] [diff] [review]
Patch 6: allow data: URIs as img source in Loop documents

Please see comment 57 for the approval request.
Attachment #8591577 - Flags: approval-mozilla-aurora?
Comment on attachment 8590481 [details] [diff] [review]
Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area

Approving for uplift as this has had a day on m-c and it's aimed for release for 39.
Attachment #8590481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8591577 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8590482 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8590483 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8590486 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8590490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mike, you marked this as not needing qe verification, but since this adds new UI and behavior, I think it would benefit from some testing. That doesn't have to be verifying every aspect of each patch. 

Let's at least give a heads up to QE that this is coming in 39. 

Florin I leave it to you how you want to handle this; maybe you can add the URL sharing interface to your team's exploratory testing for 39 once this lands. Thanks!
Flags: needinfo?(florin.mezei)
Depends on: 1154806
(Assignee)

Updated

3 years ago
status-firefox39: affected → fixed
Keywords: branch-patch-needed
(Reporter)

Updated

3 years ago
Blocks: 1155603
Depends on: 1156205
Depends on: 1156213
Depends on: 1156237
I tested this implementation across platforms (Windows 7 64-bit, Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) using latest Aurora and Nightly. I found a few issues that were added to the dependencies. Based on my testing I`m marking this bug verified fixed.
For more information about testing please see the etherpad https://etherpad.mozilla.org/ShareLink-Hello.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → wontfix
status-firefox40: fixed → verified
Flags: needinfo?(florin.mezei)
status-firefox39: wontfix → verified
Mike, or maybe Gavin, do the dependent bugs here need fixing and uplifting for this feature to be released in 39?  Who is the right person for me to talk with about making that judgement call or what the priority of the newly uncovered bugs should be?
Flags: needinfo?(mdeboer)
Flags: needinfo?(gavin.sharp)
RT+Mike should have you covered.
Flags: needinfo?(gavin.sharp) → needinfo?(rtestard)
(Assignee)

Comment 75

3 years ago
Liz, these bugs are not blockers for release in 39. I _am_ working on them, though.
Flags: needinfo?(rtestard)
Flags: needinfo?(mdeboer)
(Reporter)

Comment 76

3 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #75)
> Liz, these bugs are not blockers for release in 39. I _am_ working on them,
> though.

Agreed, these are not blockers for Fx39.
Great. Thanks Mike. Please do request uplift if you need it!
(Reporter)

Updated

3 years ago
Depends on: 1167183
(Assignee)

Updated

3 years ago
Depends on: 1170535
Depends on: 1171381
Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Hello link sharing
[Links (documentation, blog post, etc)]: Is there anything?
relnote-firefox: --- → ?
(Assignee)

Updated

3 years ago
Flags: needinfo?(rtestard)
(Reporter)

Comment 79

3 years ago
[Why is this notable]: New feature
[Suggested wording]: Share Hello URLs with social networks
[Links (documentation, blog post, etc)]: We could (should?) have a blog post and a SUMO page for launch. 

Joni can you please help us with a SUMO page to help users with the "Share" feature?
Fabio - is there a blog post planned for the Hello Fx39 share feature?
Flags: needinfo?(rtestard)
Flags: needinfo?(jsavage)
Flags: needinfo?(frios)
Sure, I've set up a placeholder. The article will be up in a couple of hours.

If we need a URL for the release notes, please use this one: https://support.mozilla.org/kb/share-firefox-hello-social

If we need an in-product link, please let me know and I'll set up a special URL for it.
Flags: needinfo?(jsavage)
(Reporter)

Comment 81

3 years ago
Thanks Joni! No in-product link is needed.
This was added to Firefox39 release notes on 06-11-2015. Updating the relnote-firefox flag to reflect that.
relnote-firefox: ? → 39+

Comment 83

3 years ago
(In reply to Romain Testard [:RT] from comment #79)
> [Why is this notable]: New feature
> [Suggested wording]: Share Hello URLs with social networks
> [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> and a SUMO page for launch. 
> 
> Fabio - is there a blog post planned for the Hello Fx39 share feature?

Per Whistler meeting, we will continue promoting new features on Hello. There will be a blog post which includes a mention of Hello share feature for 39. 

Draft of Hello portion:

"Today, we’re announcing that Firefox Share has been integrated into Hello.
{SCREENSHOT}
Hello beta, which we’ve been developing with our close partner, Telefonica, is the only in-browser video chat tool that doesn’t require an account or extra software downloads. You might recall that we recently added screensharing to Hello to make it easier to share anything you’re looking at in your conversation. Now you can also send a link to friends via the social network or email of your choice to invite them to a Hello video conversation."
Flags: needinfo?(frios)
(In reply to Fabio Rios [:frios] from comment #83)
> Hello beta, which we’ve been developing with our close partner, Telefonica,

The first half of this sounds awkward. "Hello beta, which we've been ..." in particular. To me it sounds awkward because I perceived "Hello" as a greeting, not a proper noun. I then understood "beta" to be a reference to the beta population. So reading "Hello beta, which we've been ..." sounds like "Hi beta users, which we've been ..." which doesn't make grammatical sense.

I'm not sure the "beta" wording is necessary to call out here. This would all sound much less ambiguous if it was written as "Firefox Hello, which we've been ...".
(Reporter)

Comment 85

3 years ago
(In reply to Fabio Rios [:frios] from comment #83)
> (In reply to Romain Testard [:RT] from comment #79)
> > [Why is this notable]: New feature
> > [Suggested wording]: Share Hello URLs with social networks
> > [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> > and a SUMO page for launch. 
> > 
> > Fabio - is there a blog post planned for the Hello Fx39 share feature?
> 
> Per Whistler meeting, we will continue promoting new features on Hello.
> There will be a blog post which includes a mention of Hello share feature
> for 39. 
> 
> Draft of Hello portion:
> 
> "Today, we’re announcing that Firefox Share has been integrated into Hello.
> {SCREENSHOT}
> Hello beta, which we’ve been developing with our close partner, Telefonica,
> is the only in-browser video chat tool that doesn’t require an account or
> extra software downloads. You might recall that we recently added
> screensharing to Hello to make it easier to share anything you’re looking at
> in your conversation. Now you can also send a link to friends via the social
> network or email of your choice to invite them to a Hello video
> conversation."

Is "Firefox Share" a product name?
Agreed with jaws too, I don't think we referred to "Hello Beta" before, should we rather refer to "Firefox Hello"?
Flags: needinfo?(frios)

Comment 86

3 years ago
(In reply to Romain Testard [:RT] from comment #85)
> (In reply to Fabio Rios [:frios] from comment #83)
> > (In reply to Romain Testard [:RT] from comment #79)
> > > [Why is this notable]: New feature
> > > [Suggested wording]: Share Hello URLs with social networks
> > > [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> > > and a SUMO page for launch. 
> > > 
> > > Fabio - is there a blog post planned for the Hello Fx39 share feature?
> > 
> > Per Whistler meeting, we will continue promoting new features on Hello.
> > There will be a blog post which includes a mention of Hello share feature
> > for 39. 
> > 
> > Draft of Hello portion:
> > 
> > "Today, we’re announcing that Firefox Share has been integrated into Hello.
> > {SCREENSHOT}
> > Hello beta, which we’ve been developing with our close partner, Telefonica,
> > is the only in-browser video chat tool that doesn’t require an account or
> > extra software downloads. You might recall that we recently added
> > screensharing to Hello to make it easier to share anything you’re looking at
> > in your conversation. Now you can also send a link to friends via the social
> > network or email of your choice to invite them to a Hello video
> > conversation."
> 
> Agreed with jaws too, I don't think we referred to "Hello Beta" before,
> should we rather refer to "Firefox Hello"?

We want to make sure to call out that Hello is still in beta as it has a label. I agree with you guys we should say Firefox Hello, I'll get that updated. We can treat the beta label in a way that makes sense, perhaps italics. 

> Is "Firefox Share" a product name?
Yes, this is the name that has been used since first launch. I refer to it as "shareplane" internally.
Flags: needinfo?(frios)
You need to log in before you can comment on or make changes to this bug.