Closed Bug 1213490 Opened 7 years ago Closed 1 year ago

Correct UX of top level share plane when < 2 devices are synced

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox44 unaffected, firefox45 unaffected)

RESOLVED INCOMPLETE
Firefox 44
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected

People

(Reporter: mcomella, Unassigned)

References

Details

Attachments

(2 files)

via https://bugzilla.mozilla.org/show_bug.cgi?id=1140048#c58:

Wanted to call out the various scenarios/flows here:

Not signed in:
 - Proceed to FxA sign up flow

User is signed in, but has no other connected devices:
 - Display toast with messaging
 - - "Send this tab to another connected device" 

 - At a later point, possibly experiment with using our "in-product promo sheet" UX (bug to-be-filed for this V2 polish)

User is signed in, has 1 other connected device:
 - Just send the tab, confirmation UI will follow (being handled in bug 1147653)

User is signed in, has 2+ other connected device:
 - Show device list, then send the tab to selected device, followed by confirmation UI.

For context, this button sends the current tab to a Firefox Sync connected device. But, we need to display a message for users that don't have another device to send to. 

Essentially, we want the users to understand that they need to sign in on _another_ device to fully benefit from this feature.
tracking-fennec: --- → 44+
(In reply to Michael Comella (:mcomella) from comment #0)
> User is signed in, but has no other connected devices:
>  - Display toast with messaging
>  - - "Send this tab to another connected device" 

I don't really like this String – I think it needs to focus on connecting another device so:

"Connect another device to your account to send this tab"

Anthony, what do you think?
Flags: needinfo?(alam)
Bug 1213490 - Move send to device handling code to a separate method. r=nalexander

This makes space to do more in the upcoming patches.
Attachment #8677092 - Flags: review?(nalexander)
Bug 1213490 - Handle shareplane when device list cannot be shown. r=nalexander

The handled cases are:
  * The user is not signed in (sync setup is shown)
  * The user is signed in but has no other synced devices (toast is shown)

In the future, if there is one other synced device, we should send directly to
that device (bug 1217167).
Attachment #8677093 - Flags: review?(nalexander)
Comment on attachment 8677093 [details]
MozReview Request: Bug 1213490 - Handle shareplane when device list cannot be shown. r=nalexander

https://reviewboard.mozilla.org/r/22881/#review20405

I believe the threading is wrong because the initial option select will happen on the UI thread, but I trust you to get that right :)

::: mobile/android/base/BrowserApp.java:3549
(Diff revision 1)
> +     *   * TODO (bug 1217167) Signed in but 1 other device: send tab & display confirmation

nit: with exactly 1 other device...

::: mobile/android/base/BrowserApp.java:3552
(Diff revision 1)
>      private void handleSendToDevice() {

Comment on the thread restrictions, please.  (This does blocking synchronous IO.)

There's an `AccountLoader`; is it feasible to use it here?

I've been pondering extracting a `Service` that would maintain the Firefox Account state over time, caching this information as much as possible.  Something to think about.

::: mobile/android/base/BrowserApp.java:3583
(Diff revision 1)
> +    private int getOtherSyncClientCount() {

Move this into TabsAccessor?

::: mobile/android/base/locales/en-US/android_strings.dtd:152
(Diff revision 1)
> +<!ENTITY menu_no_synced_devices "Send this tab to another connected device">

This doesn't say what to do.  "Connect another device to send this tab to"?  That's awkward.  There's no immediate action the user can take on this device to achieve the goal -- I wonder if we should send them to accounts.firefox.com and give some instruction on how to connect another device, or offer a QR code/email/...
Attachment #8677093 - Flags: review?(nalexander) → review+
(In reply to Michael Comella (:mcomella) from comment #1)
> (In reply to Michael Comella (:mcomella) from comment #0)
> > User is signed in, but has no other connected devices:
> >  - Display toast with messaging
> >  - - "Send this tab to another connected device" 
> 
> I don't really like this String – I think it needs to focus on connecting
> another device so:
> 
> "Connect another device to your account to send this tab"
> 
> Anthony, what do you think?

I prefer not having multiple "to" in the same sentence because it sounds more complicated of a task.

I also wanted to include "send" in the beginning to give it more prominence. Especially if people are just scanning the sentence, this "action" is very clear as what they want to do as opposed to what needs to be set up.
Flags: needinfo?(alam)
Attachment #8677092 - Flags: review?(nalexander) → review+
Comment on attachment 8677092 [details]
MozReview Request: Bug 1213490 - Move send to device handling code to a separate method. r=nalexander

https://reviewboard.mozilla.org/r/22879/#review20401

Gah!  Sorry, I started this yesterday, and must not have hit publish.

::: mobile/android/base/BrowserApp.java:3539
(Diff revision 1)
> +        final Tab tab = Tabs.getInstance().getSelectedTab();

I think it makes more sense to extract the tab URL at the call site.  (It's weird to look at the selected tab without context.)

Also, consider returning a boolean indicating whether we tried to send the tab.
https://reviewboard.mozilla.org/r/22879/#review20401

> I think it makes more sense to extract the tab URL at the call site.  (It's weird to look at the selected tab without context.)
> 
> Also, consider returning a boolean indicating whether we tried to send the tab.

> I think it makes more sense to extract the tab URL at the call site.  (It's weird to look at the selected tab without context.)

Done.

> Also, consider returning a boolean indicating whether we tried to send the tab.

We can do this if we ever need the return value – better to not leave in untested code.
https://reviewboard.mozilla.org/r/22881/#review20405

Corrected by putting calling `ThreadUtils.postToBackgroundThread` before calling `handleSendToDevice` and adding the `@WorkerThread` annotation to `handleSendToDevice`.

> nit: with exactly 1 other device...

Going to remove this line because bug 1217167 is wontfixed.

> Comment on the thread restrictions, please.  (This does blocking synchronous IO.)
> 
> There's an `AccountLoader`; is it feasible to use it here?
> 
> I've been pondering extracting a `Service` that would maintain the Firefox Account state over time, caching this information as much as possible.  Something to think about.

> Comment on the thread restrictions, please.  (This does blocking synchronous IO.)

Done via @WorkerThread annotation (which don't work – see bug 1219067).

> There's an AccountLoader; is it feasible to use it here?

Discussed on irc – it can be done but it's probably not worth the work, especially if we move towards the `Service` approach. We can use the `AccountLoader` but in order to prevent extra work each time the Account state is updated, we need to have a single `AccountLoader` in one `LoaderCallbacks` instance. The logic should all move to `BrowserApp` and the `LoaderCallbacks` should update the `RemoteTabsPanel` as well as the send to device state. However, this creates a dependency between the Fragment and the main Activity so it'd need to be handled carefully.

> This doesn't say what to do.  "Connect another device to send this tab to"?  That's awkward.  There's no immediate action the user can take on this device to achieve the goal -- I wonder if we should send them to accounts.firefox.com and give some instruction on how to connect another device, or offer a QR code/email/...

Let's handle this in bug 1217174 – I flagged for triage and am hoping to land in the same version, but it may be dependent on SUMO.
https://hg.mozilla.org/integration/fx-team/rev/607576699f93afbb0f30e248511c05f441e4148a
Bug 1213490 - Move send to device handling code to a separate method. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/e0fc8e81cc7013751be22a27b538ddf1ffabcbc3
Bug 1213490 - Handle shareplane when device list cannot be shown. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/81f5d579fb2bbef17eeb2a9a80c16d05fc9e399d
Bug 1213490 - review: Move BrowserApp.getOtherSyncClientCount -> TabsAccessor.getRemoteClientCount. r=me
Backing out to target 45 – see bug 1140048 comment 74.
tracking-fennec: 44+ → 45+
re backout: see bug 1140048 comment 83. Since we backed out the bug 1140048 that added this bug, this does not affect any versions until bug 1140048 lands again.
Status: RESOLVED → REOPENED
tracking-fennec: 45+ → ---
Resolution: FIXED → ---
Since bug 1140048 is backed out, not actively working on this.
Assignee: michael.l.comella → nobody
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 7 years ago1 year ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.