1359894 - Make Send Tab discoverable to non Sync/FxA users

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

When a user has either:
  - no account
  - an unverified account
  - only 1 device in their account
Show the "Send Tab to device" menu in the context menu anyway.

Instead of providing a list of other devices (which is non-existent in our cases), let's have a "Learn more about Sync…" menu item that redirects the user to a marketing web page once clicked.
That page knows about the current local state of Sync, and will show the value of (setting up/verify/add devices to) Sync to our user.

The sooner we know the URL of this future marketing page (a placeholder is fine), the sooner we can implement this behind a nightly-only pref.
Later, when the page is completely ready, we can uplift a patch that flips that pref for every version of Firefox.
I propose https://www.mozilla.org/[locale]/firefox/sync/sendtab/

Implementation note:
As Mark noted, we can deal with localization in the URL using Services.urlFormatter.formatURLPref.

Comment 1

[Edouard Oger [:eoger]]

From triage: Need copy/UX for that.

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1359894 - Show Send Tab to all users.

Review commit: https://reviewboard.mozilla.org/r/137274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137274/

Comment 3

[Edouard Oger [:eoger]]

Attachment: screenshots.zip

Comment 4

[Alex Davis [:adavis]]

This looks awesome! \o/

I think the copy looks good too.

rfeeley: any thoughts?

Michelle, can you review the copy from attached screenshots? (just a couple short lines)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

https://reviewboard.mozilla.org/r/137274/#review140900

Looks good, but I think we need to address the URL before landing.

::: browser/app/profile/firefox.js:1207
(Diff revision 1)
>  pref("services.sync.sendTabToDevice.enabled", true);
>  
> +// Whether we show a "send tab to device" menu item in the context menu even
> +// when the user is logged out/has no other device/is not verified and show
> +// a "learn more about sync" button that redirects to a promo page.
> +#ifdef NIGHTLY_BUILD

I believe we generally want all in-content links like this to go via, say, https://support.mozilla.org/1/firefox/55.0a1/WINNT/en-US/send-tab-promo -  which we'd format via Services.urlFormatter.formatURLPref("app.support.baseURL") + "send-tab-promo"; - otherwise we run the risk of the link getting "lost" in the future (eg, as pages are reorganised, people might forget it is an in-content link, or it might not get the usual localization process etc).

I think the #sumo channel might be able to point you in the right direction for getting that "tail" setup with a placeholder (although it actually looks like we want multiple tails?)

In which case, we wouldn't need this pref at all.

::: browser/base/content/browser-sync.js:441
(Diff revision 1)
>      ["context_sendTabToDevice", "context_sendTabToDevice_separator"]
>      .forEach(id => { document.getElementById(id).hidden = !showSendTab });
>    },
>  
>    initPageContextMenu(contextMenu) {
> -    if (!this.sendTabToDeviceEnabled || !this.weaveService.ready) {
> +    if (!this.sendTabToDeviceEnabled ||

please add parens here to make the precedence clearer.

Comment 6

[Edouard Oger [:eoger]]

Hello Joni,

Per comment 5, would it be possible to set-up a placeholder page for
https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/send-tab-promo ?

At some point in the future we'd like to redirect a marketing page (which will probably live in the Marketing website, e.g https://www.mozilla.org/%LOCALE%/firefox/sync/sendtabpromo), is this possible?

Thanks!

Comment 7

[Joni Chan]

Hi Edouard,

We can set up a placeholder page, although we can only redirect to SUMO pages on our end. 

When you're ready to redirect to a marketing page, check with David Tenser to see how you can do that.

In the meantime, I'd be happy to set up a SUMO page to redirect to.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Alex, If we want to use a different location than (basically) every other in-product link (and there are many "learn more" links in that set), I think we should involve firefox-dev to see what objections and/or requirements exist for this - but to do that, I think we need a clearly articulated reason why sumo isn't suitable to host these links. If you can give me that reason, I'll draft the mail!

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Dave,
  This bug wants to add an in-product link to Desktop Firefox - basically, a marketing "call to action" to download mobile Firefox.

Long ago, I remember Gavin telling me that all such links should use formatURLPref("app.support.baseURL"), meaning we end up with a URL like https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/, and we rely on the sumo infrastructure to "do the right thing" in terms of localization, appropriate redirections, etc.

However, Alex and the marketing team are quite adamant that SUMO isn't a good option for this. Alex has articulated a few reasons, particularly around branding, control of the content, and the existing ability for www.mozilla.org to query the current Sync state (as currently leveraged by about:home snippets etc). Alex assures me they have localization etc under control. They would like a URL https://www.mozilla.org/%LOCALE%/firefox/sync/

I'm asking if it is fine for me to r+ a patch for an in-product link that goes to https://www.mozilla.org/%LOCALE%/firefox/sync/ and assume the owners of that page will do the right thing, or whether there are compelling reasons for using SUMO? Alternatively, can you please point me in the direction of the right people to make that decision?

Thanks!

Comment 10

[Alex Davis [:adavis]]

Hi, yes Mark is correct.

We would like to send users to https://www.mozilla.org/%LOCALE%/firefox/features/sendtab (under construction).

The Send Tab feature will become visible to all users in the product in hopes that the value it provides additional motivation to create an account and setup Sync.

In order for users to learn more about the feature when they discover it, we wish for users to visit the above URL since it
- Will be better optimized for conversion (sumo is not meant to be a high converting landing page)
- Will be easier to A/B test and improve over time
- Will be under the control of the marketing retention team
- Will be use our latest branding and design
- Will have the intelligence to detect if a user is already logged in to FxA in the browser which will allow for the content to be more dynamic resulting in personalized next steps.
- We will be able to embed our send mobile app by SMS form developed by FxA team in an upcoming stage
- Will help support the marketing team's SEO initiative around creating feature specific web pages

I hope this clarifies.

The only other way around this is if the FxA team controls the page and hosts it themselves but the FxA team does not want to own the pages and would prefer that the Product Marketing and Marketing Retention teams be in control of this page without any dependency on the FxA team.

Comment 12

[Dave Camp (:dcamp)]

Perhaws we should add a similar app.productInfo.baseURL for marketing/call-to-action pages vs. support pages, rather than hard coding?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

(In reply to Dave Camp (:dcamp) from comment #12)
> Perhaws we should add a similar app.productInfo.baseURL for
> marketing/call-to-action pages vs. support pages, rather than hard coding?

The intention was always to use a pref for this, but if we are happy with the general approach I agree we should consider semi-formalizing it into a generic "productInfo" preference which would be consistently used for all such features.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Mark Hammond [:markh] from comment #13)
> (In reply to Dave Camp (:dcamp) from comment #12)
> > Perhaws we should add a similar app.productInfo.baseURL for
> > marketing/call-to-action pages vs. support pages, rather than hard coding?
> 
> The intention was always to use a pref for this, but if we are happy with
> the general approach I agree we should consider semi-formalizing it into a
> generic "productInfo" preference which would be consistently used for all
> such features.

Let's go ahead with that

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/1-2/

Comment 16

[Edouard Oger [:eoger]]

This probably mandates a full review again, because bug 1368383 introduced a lot of changes in the code base.

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/2-3/

Comment 18

[Edouard Oger [:eoger]]

Fixed failing tests.

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/3-4/

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

https://reviewboard.mozilla.org/r/137274/#review154930

Sorry for being so vague, but I find this code quite confusing and difficult to work out what the intent is and actually how it works. Worse, I can still see the wrong state on the page action menu - I'll attach a screenshot.

ISTM that we should be able to simplify some of this logic if we step back a little.

::: browser/app/profile/firefox.js:1000
(Diff revision 4)
>  pref("app.feedback.baseURL", "https://input.mozilla.org/%LOCALE%/feedback/firefoxdev/%VERSION%/");
>  #else
>  pref("app.feedback.baseURL", "https://input.mozilla.org/%LOCALE%/feedback/%APP%/%VERSION%/");
>  #endif
>  
> +pref("app.productInfo.baseURL", "https://www.mozilla.org/firefox/features/%FEATURE%/?utm_source=firefox");

Please add a comment along the lines of "// base URL for web-based marketting pages"

I also noticed that this redirects to https://www.mozilla.org/en-US/firefox/features/sendtab/?utm_source=firefox - shouldn't we just include the locale in the URL to avoid that?

::: browser/base/content/browser-sync.js:48
(Diff revision 4)
>    get syncReady() {
>      return Cc["@mozilla.org/weave/service;1"].getService().wrappedJSObject.ready;
>    },
>  
> +  get syncLoading() {
> +    return !this.syncReady && UIState.get().status == UIState.STATUS_SIGNED_IN;

This name seems wrong/misleading - it's a big longer, but something like syncConfiguredButLoading() seems more explicit.

However, as I'll say below, I'm not quite sure how this helps us in practice - just because this.syncReady goes true doesn't mean we've done the first clients sync yet (the first login can take quite a few seconds)

::: browser/base/content/browser-sync.js:288
(Diff revision 4)
>        replaceQueryString: true
>      });
>    },
>  
> +  openSendToDevicePromo() {
> +    let url = Services.prefs.getCharPref("app.productInfo.baseURL");

I'd really rather have this using formatURLPref as it has special handling for the preference being localized - I don't fully understand exactly when that happens, but we should assume that when it does, it's for good reason.

It's a shame it doesn't handle additional params to format, but I'd be inclined to have the pref as just "https://www.mozilla.org/firefox/features/" and explicitly add "sendtab?" + Services.appInfo.name or similar.

::: browser/base/content/browser-sync.js:319
(Diff revision 4)
> -    if (this.syncReady) {
> +
> +    const state = UIState.get();
> +    if (state.status == UIState.STATUS_SIGNED_IN && this.remoteClients.length > 0) {
> +      this._appendSendTabDeviceList(fragment, createDeviceNodeFn, url, title);
> +    } else if (state.status == UIState.STATUS_SIGNED_IN) {
> +      this._appendSendTabSingleDevice(fragment, createDeviceNodeFn);

shouldn't this also be checking if we think we've synced the clients engine? It looks like it will show the "single device" fragment in that case, which would be wrong.

::: browser/base/content/browser-sync.js:434
(Diff revision 4)
>      }
>    },
>  
>    // "Send Tab to Device" menu item
>    updateTabContextMenu(aPopupMenu, aTargetTab) {
> -    const show = this.syncReady &&
> +    const show = !this.syncLoading &&

So IIUC, the intent is that unlike the "page action" menu, we don't show anything on the context menu until we are ready? Given we are not ready only once, I wonder if we can simplify this by just early-returning if not syncReady and relying on the item being hidden by default? That would probably avoid the need for the new syncLoading.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: bad-pageaction-state.png

If I show the page action menu at just the right time, I see this state - even though there *are* other devices connected.

Note also that the page action menu is too narrow in most of these states - also in this screenshot. If we don't fix that here we should get another bug on file to ensure the popup resizes as the state changes.

Comment 22

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

https://reviewboard.mozilla.org/r/137274/#review154930

> Please add a comment along the lines of "// base URL for web-based marketting pages"
> 
> I also noticed that this redirects to https://www.mozilla.org/en-US/firefox/features/sendtab/?utm_source=firefox - shouldn't we just include the locale in the URL to avoid that?

From David Tenser:
"We can detect locale just as fast from the site - that's actually better because it's possible that their first language choice might not be available, but a 2nd or 3rd might and the site knows in which languages the page is available. (Note that currently the new Features pages only exist in en-US - I think this further underscores the importance to get these pages localized, which we have lined up in our backlog.)"

Comment 23

[Alex Davis [:adavis]]

Per the page only being available in English right now, we are looking to expedite localization because it is a blocker IMO. I'll keep you updated.

Comment 25

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/4-5/

Comment 26

[Edouard Oger [:eoger]]

Thanks Mark, I amended the patch with your comments.
We now make sure the clients engine is synced before declaring sync is "ready".
I also merged bug 1372419 patch in that one.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

https://reviewboard.mozilla.org/r/137274/#review156526

This looks much better, thanks - but it looks quite strange when we are initializing - a very narrow popup with no content can be seen for a second or so (I'll upload a screenshot) - this can be seen both when sync is configured and when it isn't. See possible solutions below.

I also think we need to get resizing worked out - I'm not too bothered if it is here or in a different bug, but I don't think we should land this without it. If your PC username is > 4 characters long, every device will look like "Username's Nightly on ..." meaning users will be unable to differentiate the devices. (My username is "skip", and I see only the first character of the device name, which gives *me* enough clues to distinguish the device, but many users will not get even that.

::: browser/base/content/browser-sync.js:47
(Diff revision 5)
>  
>    get syncReady() {
>      return Cc["@mozilla.org/weave/service;1"].getService().wrappedJSObject.ready;
>    },
>  
> +  get syncConfiguredAndLoading() {

The "AndLoading" part is still a little misleading to the casual reader - but a better name isn't obvious - so I think a little comment saying "returns true if sync is configured but is yet to determine if any remote clients exist." or similar.

::: browser/base/content/browser-sync.js:50
(Diff revision 5)
>    },
>  
> +  get syncConfiguredAndLoading() {
> +    return UIState.get().status == UIState.STATUS_SIGNED_IN &&
> +           (!this.syncReady ||
> +           // lastSync is reset when ClientsEngine is constructed.

and maybe expand this comment with "and will be non-zero once it has synced" (or the entire comment could read simply "lastSync will be non-zero after the first sync")

::: browser/base/content/browser-sync.js:316
(Diff revision 5)
>        if (child.classList.contains("sync-menuitem")) {
>          child.remove();
>        }
>      }
>  
> +    // We can only be in that case in the page action menu.

This comment confused me a little - maybe move it to within the block and s/that case/this state/?

::: browser/base/content/browser.css:1380
(Diff revision 5)
>  }
>  
>  /* Page action menu */
> -#page-action-sendToDeviceView-body:not([state="notsignedin"]) > #page-action-sendToDevice-fxa-button,
> -#page-action-sendToDeviceView-body:not([state="nodevice"]) > #page-action-no-devices-button,
>  #page-action-sendToDeviceView-body:not([state="notready"]) > #page-action-sync-not-ready-button {

I wonder if we should invert this, so it is displayed by default, and becomes hidden once the state becomes "ready"? This means users without Sync configured might briefly see "Syncing Devices", but that's likely better than the narrow panel they see now. Alternatively, a separate section saying something like "Determining Sync status" might also work.

::: browser/base/content/browser.js:7893
(Diff revision 5)
> -    }
> -
>      // In the first ~10 sec after startup, Sync may not be loaded and the list
>      // of devices will be empty.
> -    if (!gSync.syncReady) {
> +    if (gSync.syncConfiguredAndLoading) {
>        body.setAttribute("state", "notready");

I also wonder if we should just move this block to near the top of the function and avoid the gSync.populate call when we are syncConfiguredAndLoading - there should be no need to remove existing items in that case as we have never populated them. I'm not sure that will help manage this initial state in practice, but it will make the intent here clearer.

::: browser/base/content/browser.js:7897
(Diff revision 5)
> -    if (!gSync.syncReady) {
> +    if (gSync.syncConfiguredAndLoading) {
>        body.setAttribute("state", "notready");
>        // Force a background Sync
>        Services.tm.dispatchToMainThread(() => {
>          Weave.Service.sync([]);  // [] = clients engine only
> -        if (!window.closed && gSync.syncReady) {
> +        if (!window.closed && !gSync.syncConfiguredAndLoading) {

I think it might be worth a comment here saying something like "there's no sane way sync is still loading here, but we check anyway to avoid infinite looping" or similar.

::: browser/base/content/test/general/browser_contextmenu.js:710
(Diff revision 5)
>       "context-copy",                        true,
>       "context-selectall",                   true,
>       "---",                                 null,
>       "context-searchselect",                true,
> +     "---",                                 null,
> +     "context-sendlinktodevice", true, [], null,

nit: you might as well align the null here with the rest of the list.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Attachment: bad-pageaction-state.png

This is the state I briefly see both when sync is configured and when it isn't.

Comment 29

[Edouard Oger [:eoger]]

So I investigated a little, moving from a "notready" to "ready" class didn't help, I still can see the flickering in some cases (I have to add code to actually see it, otherwise I can't reproduce).

It looks like this is caused by a bad setTimeout() call [0], there is a bug on file for that (bug 1363756) but no one touched yet :/


[0] https://dxr.mozilla.org/mozilla-central/rev/7a9536f89bc75b0672060f16ffbe6eb2c1ff3deb/browser/components/customizableui/PanelMultiView.jsm#605-608

Comment 30

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/5-6/

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

https://reviewboard.mozilla.org/r/137274/#review159398

Great, thanks - but please wait for the page to be available :) There's probably no real need to wait for the resizing bug though.

Comment 32

[kirby]

Thanks Edouard! 

Please note that as per bug 1370574 the URL https://www.mozilla.org/firefox/features/send-tabs/ will have a hyphen between "send" and "tabs".

Comment 33

[Edouard Oger [:eoger]]

Comment on attachment 8865655 [details]
Bug 1359894 - Show Send Tab to all users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137274/diff/6-7/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/451baf8146c6
Show Send Tab to all users. r=markh

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/451baf8146c6

Comment 36

[Oriol Brufau [:Oriol]]

Please stop bloating my context menu with ads of useless things I don't plan to use. How can I get rid of this?

Comment 37

[Alex Davis [:adavis]]

Sorry to hear that you don't want to use Send Page/Tab.

There is no plan at this time to allow users to disable Send Page/Tab in the context menu but we will certainly consider your feedback for future iterations of this feature.

Please note that our goal is not to make this feel like an ad but rather to help our users more easily discover a great feature that many already love and praise but that has been really hard to discover up until now.

It is however understandable that you might not want to use every feature in context menus. For example, "View page source", "View page info" and "Inspect element" are purely targeted at developers but visible to all. At this time, I don't believe any of these are customizable. If they are then we can see about implementing similar logic. In contrast though, "Send Page" (or tab) is one of the few contextual menu items that can provide value to everyone.

That being said, we certainly consider all feedback as it comes in and make adjustments/iterations accordingly.

Comment 38

[Oriol Brufau [:Oriol]]

Thanks for considering the feedback. In fact this context entry is much less annoying than e.g. the one of the Screenshots thingie. But I'm a bit overwhelmed with Sync buttons everywhere (in the hamburguer menu, in the Library, in Preferences, in Tools menu, and now the context menu).