1217174 - Implement helper UI for send to device when user is not ready to send a tab (e.g. no account)

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

In bug 1213490, we introduce the flow that when the user signs into their sync account but has no other devices connected, we show a toast telling the user to connect another device but there isn't enough space to be specific and really tell the user what to do.

Anthony, anything better we can do here? SUMO, or a tab queue style dialog?

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

A super toast (soon to be snackbar) would be a nice start. Do we have an article on SUMO about this feature?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The experience is super crappy atm – I'm thinking tracking 44?

Comment 3

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

:mcomella i don't think we have an article about this aspect of sync, right:joni?
here are the articles we have:

https://support.mozilla.org/en-US/products/mobile/save-share-and-sync-firefox-android

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, what if we used the tab queue contextual hint to display a longer message than we would otherwise be able to? There would only be one call-to-action button (e.g. "Got it").

If there's a sumo article on adding a sync device, we can have a "Learn more" button too.

Comment 5

[:Margaret Leibovic]

It's very late for UX changes for 44, but let's see if we can do something simple.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We decided to punt to 45 – see https://bugzilla.mozilla.org/show_bug.cgi?id=1140048#c74

We want to do a helper UI here to step the user through using this feature if they're not in the correct state (logged into a Firefox account with another device to send a tab to).

Keeping the NI for Anthony for mocks.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Not ready to share means:
  * Not logged into a Firefox account
  * Logged into a Firefox account but there are no other synced devices to send a tab to

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I met with Barbara and Anthony. We decided to go ahead with Anthony's specs for the full helper UI. However, since we can't respond to the user signing into sync without significant work, the onboarding flow will "drop" the user – they'll have to click the menu and shareplane again to continue the flow. Ideally, when the user who is already signed in clicks the shareplane but there are no devices to share to and they're shown the, "You need to sign in on another device" screen, when they click "Got it", we'll sync and show the device list if applicable, but we'll see how much work that is too.

In the event that this doesn't hit 45, we'll back out again rather than put in the work to make a Nightly flag (bug 1220910).

Anthony, can you post your specs for the helper UI?

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #8)
> I met with Barbara and Anthony. We decided to go ahead with Anthony's specs
> for the full helper UI. However, since we can't respond to the user signing
> into sync without significant work, the onboarding flow will "drop" the user
> – they'll have to click the menu and shareplane again to continue the flow.
> Ideally, when the user who is already signed in clicks the shareplane but
> there are no devices to share to and they're shown the, "You need to sign in
> on another device" screen, when they click "Got it", we'll sync and show the
> device list if applicable, but we'll see how much work that is too.

Agreed, this would be a nice to have and if we can't get to it, we should split it off into a follow up bug post-45 I think :)

> In the event that this doesn't hit 45, we'll back out again rather than put
> in the work to make a Nightly flag (bug 1220910).
> 
> Anthony, can you post your specs for the helper UI?

I've collected a rough flow here. https://invis.io/324PYKYQX

I'll also attach the images in this bug (with the device and the plane illustration)

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: illustration_connecteddevice.zip

images!

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Bug 1140048 was backed out. We plan to implement this helper UI in the current menu, then reland bug 1140048 and attach this helper UI to that new menu.

Comment 12

[Alex Johnson (:alex_johnson)]

I'd like to pick this up if no one's currently working on it.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson(:alex_johnson) from comment #12)
> I'd like to pick this up if no one's currently working on it.

Sure! To be explicit, here's what you have to do:

1) Make the "Send to other devices" menu item appear in the share menu all the time (i.e. no matter account/device state).
2) When the user is not signed into an account and they click the "Send to other devices" item, show [1]. The button should open the "Sign into Firefox" account flow (look at the settings code to find out how to do this). Once the user finishes this flow, you don't need to take any action.
3) When the user is signed in but there are no other devices to share to and they click the "Send to other devices" item, show [2]. When they click the "Got it" button, the dialog should be dismissed
4) When there are devices to share to, we activate the current flow that shows the share overlay.

The dialogs we show should re-use the tab queue prompt code – unfortunately, I'm not sure where that is or if it's abstract enough at the moment to support arbitrary UI. Let me know if you need help with this.

A follow-up we considered is to make the transitions between steps smoother so after signing in from 2), we'll open the dialog in 3) (if applicable), and once there is a synced device, we open 4).

Is that enough info to get going on this?

[1]: https://mozilla.invisionapp.com/share/324PYKYQX#/screens/113495935
[2]: https://mozilla.invisionapp.com/share/324PYKYQX#/screens/113496012

Comment 14

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 15

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1217174 - Removed unused icon_shareplane drawables. r?mcomella

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

Comment 16

[Alex Johnson (:alex_johnson)]

Michael,
Are there any docs on fxaccounts somewhere?  I'm having a hard time figuring out how to close the tab and go to the page the user was on after a successful login.  Or should we file a bug in fxaccounts to close the tab after a successful login if the request came with a "sendToDevice" entrypoint url parameter (aka. about:accounts?action=signup&entrypoint=sendToDevice)?

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson (:alex_johnson) from comment #16)
> Are there any docs on fxaccounts somewhere?

Redirect nalexander.

> I'm having a hard time figuring
> out how to close the tab and go to the page the user was on after a
> successful login.  Or should we file a bug in fxaccounts to close the tab
> after a successful login if the request came with a "sendToDevice"
> entrypoint url parameter (aka.
> about:accounts?action=signup&entrypoint=sendToDevice)?

I don't think there's currently a way to do this and a bug exists – bug 1221217. Sorry that I was unclear – reacting to steps the user takes in the helper UI should be left to the follow-up.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

https://reviewboard.mozilla.org/r/29099/#review25959

r+ w/ nits discussed.

::: mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java
(Diff revision 1)
> -                    if (!hasOtherSyncClients()) {

I think this method is unused now – you could remove it here.

::: mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java:783
(Diff revision 1)
> -                    resolveInfo.icon = R.drawable.icon_shareplane;
> +                    resolveInfo.icon = R.drawable.shareplane;

What was the motivation moving to the other icon? Was there any visible change in position?

I think we used the other icon to preserve certain whitespace requirements.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702209 [details]
MozReview Request: Bug 1217174 - Removed unused icon_shareplane drawables. r?mcomella

https://reviewboard.mozilla.org/r/29101/#review25961

Provided we don't actually need this (part 1), lgtm.

Comment 20

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 21

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/29099/#review25959

> What was the motivation moving to the other icon? Was there any visible change in position?
> 
> I think we used the other icon to preserve certain whitespace requirements.

I was looking at Bug 1210989 where we renamed overlay_send_tab_icon to shareplane.  That was a different scenario.  I changed it back.

Comment 22

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

https://reviewboard.mozilla.org/r/29099/#review25971

Comment 24

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 25

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #17)
> (In reply to Alex Johnson (:alex_johnson) from comment #16)
> > Are there any docs on fxaccounts somewhere?
> 
> Redirect nalexander.
> 
> > I'm having a hard time figuring
> > out how to close the tab and go to the page the user was on after a
> > successful login.  Or should we file a bug in fxaccounts to close the tab
> > after a successful login if the request came with a "sendToDevice"
> > entrypoint url parameter (aka.
> > about:accounts?action=signup&entrypoint=sendToDevice)?
> 
> I don't think there's currently a way to do this and a bug exists – bug
> 1221217.

This is correct.  I'm not sure what this should be, tbh, because a user could create an account and be in many states, including both unverified, so they cannot Sync (and cannot send a tab); and verified, but haven't yet Synced, so we have no devices for them.  They could also have an Android account that doesn't actually reflect a server account, due to the server deleting a server account immediately upon recognizing a bad email address.

I think we should surface "account added" messages throughout the app, and then try to centralize "account is synced and healthy" more generally.  We don't have that right now.

 Sorry that I was unclear – reacting to steps the user takes in the
> helper UI should be left to the follow-up.

The interaction with Send Tab in particular is hard.  We might want to add a "sendTab" parameter to about:accounts, and then have the fxa-content-server web UI display a button to tap to "continue to the send tab" experience.

We might also want to understand what Desktop does in this situation.

markh, vlad: can you try to interpret Comment 16 and Comment 17 and explain to me how Desktop manages "continuation after sign up/sign in" actions?  We should be as similar as we can be.

Comment 26

[Alex Johnson (:alex_johnson)]

I think we should make sure the sync has completed after login before we send the user back to the "send to other device" activity. That might make for a smoother flow. We should show a loading screen or something after login until the sync has successfully completed or show a toast and go back to the Web page if sync failed.

Comment 27

[Alex Johnson (:alex_johnson)]

s/should show/could show/

Comment 28

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

(In reply to Nick Alexander :nalexander [Vacation PTO from Dec 23--Jan 4] from comment #25)
> markh, vlad: can you try to interpret Comment 16 and Comment 17 and explain
> to me how Desktop manages "continuation after sign up/sign in" actions?  We
> should be as similar as we can be.

On desktop we have observers that could be used to listen for relevant state changes. However, I suspect that on desktop we wouldn't bother with attempts at continuation in this scenario. There are 3 interesting states here.

* User has an account but isn't logged in.
* User has no account.
* Account has no other devices.

Only the first 1 is possible to continue from. I suspect that in the first 2 cases we'd just show UI saying, eg, "You need to sign in before sending a tab", throw them to about:accounts and leave them to re-initiate the operation after completion. In the second case we'd show a promo for downloading a mobile Firefox. This is what we do for, eg, the new "synced tabs" panel - we throw them to about:accounts, but don't make any attempt to detect the login and reopen the panel UI after completion.

Comment 29

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 30

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 31

[Nick Alexander :nalexander [he/him]]

(Clearing NI from vlad since markh hit the request.)

(In reply to Alex Johnson (:alex_johnson) from comment #26)
> I think we should make sure the sync has completed after login before we
> send the user back to the "send to other device" activity. That might make
> for a smoother flow. We should show a loading screen or something after
> login until the sync has successfully completed or show a toast and go back
> to the Web page if sync failed.

In general, this is hard, since:

1) Sync might not be triggered when requested (by the system);
2) even when triggered, a sync might take a long time to complete, or never complete;
3) Sync is rather a wholistic system and doesn't surface individual errors cleanly.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/29099/#review26457

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

https://reviewboard.mozilla.org/r/29169/#review26459

Technically correct and feasible as a basic implementation, but this doesn't follow the mocks Anthony included in comment 9 – can you try to implement that UI instead?

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java:111
(Diff revision 3)
> -        // Escape hatch: we don't show the option to open this dialog in this state so this should
> +        if (account == null) {

Maybe you want to use `FirefoxAccounts.firefoxAccountsExist` here if you're not going to use the returned account?

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java:112
(Diff revision 3)
> -        // never be run. However, due to potential inconsistencies in synced client state
> +            final Intent fxIntent = new Intent(FxAccountConstants.ACTION_FXA_GET_STARTED);

We should keep this comment unless we remove the code it's associated with.

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java:120
(Diff revision 3)
> +        else {

Rather than indenting all of this code (since excessively indented code becomes difficult to read), you should return early from the branch above. This is typically with error/assumption handling.

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java
(Diff revision 3)
> -

nit: Was this whitespace change intentional? If so, we ususally make whitespace changes in a separate commit (unless we're overwriting the lines we're changing) to make it clear it was intentional. If it was not intentional, try not to change unrelated lines! :) You can use `hg diff` and `hg diff -c .` to ensure you're making the changes you intend to.

::: mobile/android/base/locales/en-US/android_strings.dtd:143
(Diff revision 3)
> +     Localization note (overlay_no_firefox_account): Used when the menu option

You should move this directly above the String it refers to. Don't worry about reducing the number of comment blocks.

Comment 34

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

I'm gonna go ahead and redo this one.  Sorry about that!

Comment 35

[Alex Johnson (:alex_johnson)]

Anthony, do we have a specific font size and color for the "you'll need to connect 2 devices" text or is it the same style as the hint in the History empty state?

Comment 36

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Alex Johnson (:alex_johnson) from comment #35)
> Anthony, do we have a specific font size and color for the "you'll need to
> connect 2 devices" text or is it the same style as the hint in the History
> empty state?

This takes from the same design specs as our "Opening multiple links?" helper UI in bug 1112195. So if we could just inherit the same padding, etc, that would be great :)

It should be Roboto, italic, 14sp, #E66000

Thanks Alex!

Comment 37

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^

Comment 38

[Alex Johnson (:alex_johnson)]

Attachment: device-2016-01-11-182707.png

Comment 39

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 40

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 41

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 42

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 43

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 44

[Alex Johnson (:alex_johnson)]

Added the animateout for the NoAccountsDialog and fixed some nits, might be a good idea to move the animateOut and some of the other common functions to a different "Dialog.java" file. But I'm gonna leave that for a different bug.  It's a little complicated since the ShareDialog and NoAccountsDialog have different layouts.

Comment 45

[Alex Johnson (:alex_johnson)]

Attachment: Tablet Screenshot

Comment 46

[Alex Johnson (:alex_johnson)]

Attachment: Tablet Screenshot

Comment 47

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8706658 [details]
device-2016-01-11-182707.png

Nice! this is almost there Alex!

 - The padding above the image and below the "Sign in to Fennec" button should both be 40 dp :)

 - In addition, the padding below the graphic should be 30 dp

Thanks!

Comment 48

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8706746 [details]
Tablet Screenshot

same feedback regarding the padding :)

Comment 49

[Alex Johnson (:alex_johnson)]

Attachment: Screenshot

How does this look?

Comment 50

[Alex Johnson (:alex_johnson)]

Attachment: Tablet Screenshot

Comment 51

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8707206 [details]
Screenshot

The margins around the image looks better. But the button should stay the same size as before. I meant to increase the negative space surrounding it, not the size of the button itself.

Sorry! I must've used "padding" much too loosely here. The "padding" I was referring to should be created with margins outside the button.

Comment 52

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8707212 [details]
Tablet Screenshot

Same comments about the button size as above :)

The margins under the image look too large here. Is it 30 dp? 

To be clear: 

 - the white space between the bottom of the image and the top of the "Want to send this tab?" should be 30 dp.

 - the white space between the top of the sheet and the top of the image should be 40 dp.

Comment 53

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Anthony Lam (:antlam) from comment #52)
> The margins under the image look too large here. Is it 30 dp? 
> 
> To be clear: 
> 
>  - the white space between the bottom of the image and the top of the "Want
> to send this tab?" should be 30 dp.
> 
>  - the white space between the top of the sheet and the top of the image
> should be 40 dp.

same comments for the phone UI :)

Thanks Alex!

Comment 54

[Alex Johnson (:alex_johnson)]

Attachment: Screenshot_2016-01-14-15-37-28.png

How does this look?  Sorry, the text had padding on top causing the image to show a bottom padding of larger than 30dp.

Comment 55

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8708050 [details]
Screenshot_2016-01-14-15-37-28.png

Looks good! nice job \o/

Comment 56

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 57

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 58

[Alex Johnson (:alex_johnson)]

Michael, I've got the "Sign in" dialog here, but should we wait till we figure out the about:accounts callback stuff till we implement the last part of Anthony's mock?

Comment 59

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey Alex – I'll get to this tomorrow. Sorry for the delay.

Comment 60

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

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

Comment 61

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

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

Comment 62

[Alex Johnson (:alex_johnson)]

(In reply to Alex Johnson (:alex_johnson) from comment #61)
> Comment on attachment 8702659 [details]
> MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity
> and send url parameter to about:accounts with the webpage the user wanted to
> send. r?mcomella
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/29169/diff/7-8/

Fixed a crash this was causing on one of my devices.

Comment 63

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson (:alex_johnson) from comment #58)
> Michael, I've got the "Sign in" dialog here, but should we wait till we
> figure out the about:accounts callback stuff till we implement the last part
> of Anthony's mock?

No. It was my impression that Anthony and I decided that the user would just have to click the shareplane icon again to move forward in the process since implementing the continuation would be difficult.

Anthony, do you agree?

Comment 64

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/29099/#review28445

mozreview tells me I need to review this again but I seem to have already r+'d it on bz!

Comment 65

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702659 [details]
MozReview Request: Bug 1217174 - Part 2: Created NoAccountsDialog Activity and send url parameter to about:accounts with the webpage the user wanted to send. r?mcomella

https://reviewboard.mozilla.org/r/29169/#review28447

I was hoping to re-use some existing code here – let's see how much work it is. If it's too much, maybe we should just move forward with this approach.

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/NoAccountsDialog.java:25
(Diff revision 8)
> +public class NoAccountsDialog extends Locales.LocaleAwareActivity {

Sorry if I was unclear but I was hoping to repurpose the `TabQueuePrompt` for this. Did you take a look into that and if so, is there a reason you chose not to? Would it be much work to move this code over there? It looks like there is a lot of tab queue specific code in there so maybe it'd be more trouble than it's worth...

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/NoAccountsDialog.java:34
(Diff revision 8)
> +        ((ImageView)findViewById(R.id.image)).setImageResource(R.drawable.mobile_to_pc);

Make sure you run the new png assets through an image compressor before `hg add`ing them. We typically use `trimage` on Linux or ImageOptim on OS X.

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java
(Diff revision 8)
> -

nit: You removed this whitespace line – try not to change unrelated whitespace!

::: mobile/android/base/locales/en-US/android_strings.dtd:144
(Diff revision 8)
> +<!-- Localization note: The following strings show in the no accounts dialog when

afaik, a localization note can only describe a single String. The localization tools automatically parse out the notes so they need to follow the format of the other notes in the file.

::: mobile/android/base/resources/layout/overlay.xml:3
(Diff revision 8)
> +   This Source Code Form is subject to the terms of the Mozilla Public

nit: can you follow the formatting for the other files with this comment?

::: mobile/android/base/resources/layout/overlay.xml:16
(Diff revision 8)
> +        style="@style/Overlay">

If styles aren't re-used, it's best practice to leave the style declarations within the xml file itself. Putting them outside (via `@style` references) causes the developer to jump to a different file in order to understand the structure when it could be simpler and flat. Similarly with inheritance – don't inherit unless you have to!

::: mobile/android/base/resources/layout/overlay.xml:24
(Diff revision 8)
> +            style="@style/Overlay.Title" />

Best practice with text is to put text attributes in `textAppearance` and other attributes in `style`.

::: mobile/android/base/resources/layout/overlay.xml:51
(Diff revision 8)
> +         entire Activity with an onClickListener. -->

Could you set the hit listener on the root view of the activity instead?  I'm not sure how the root view's children might intercept the presses though.

Or perhaps preferrably, override `onInterceptTouchEvent` (or whichever touch event it is that handles the initial touch)?

Comment 66

[Alex Johnson (:alex_johnson)]

(In reply to Michael Comella (:mcomella) from comment #63)
> (In reply to Alex Johnson (:alex_johnson) from comment #58)
> > Michael, I've got the "Sign in" dialog here, but should we wait till we
> > figure out the about:accounts callback stuff till we implement the last part
> > of Anthony's mock?
> 
> No. It was my impression that Anthony and I decided that the user would just
> have to click the shareplane icon again to move forward in the process since
> implementing the continuation would be difficult.
> 
> Anthony, do you agree?

Ah.  I think this makes more sense to me now.  I thought we were going to show the "Where do we send it?" dialog after the login has completed.  But, we wanna show it if the user taps the sendtab dialog and a sync device isn't found?  Am I understanding that correctly?

Comment 67

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson (:alex_johnson) from comment #66)
> But, we wanna show it if the user taps the sendtab dialog and a sync device isn't
> found?  Am I understanding that correctly?

We want to show it if the user taps the shareplane icon (the "send tab to device" button) and the user has an account but no other devices are found.

Comment 68

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also, if you were curious about more style best-practices, this is a really good blog post:
 http://blog.danlew.net/2014/11/19/styles-on-android/

Comment 69

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #63)
> (In reply to Alex Johnson (:alex_johnson) from comment #58)
> > Michael, I've got the "Sign in" dialog here, but should we wait till we
> > figure out the about:accounts callback stuff till we implement the last part
> > of Anthony's mock?
> 
> No. It was my impression that Anthony and I decided that the user would just
> have to click the shareplane icon again to move forward in the process since
> implementing the continuation would be difficult.
> 
> Anthony, do you agree?

Yep - what :mcomella said.

Comment 70

[Alex Johnson (:alex_johnson)]

Comment on attachment 8702208 [details]
MozReview Request: Bug 1217174 - Part 1: Always show 'Send to other devices' menu item. r?mcomella

I'm gonna redo this one.  Some recent changes broke my patch (won't let me rebase).

Comment 71

[Alex Johnson (:alex_johnson)]

I'm gonna unassign this because I will not have time to continue working on this until sometime.

Comment 72

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, are we still interested in this?

Comment 73

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hm, what's missing here?

I don't think this is prioritized in our road map. But this contextual hint seems close enough that it might be good to finish off.

Comment 74

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I spoke to Anthony IRL – he mentioned we still want this. I offered the possibility that we should try to prioritize our mentored bugs more readily and he said we can discuss it during the funnel meeting.

(In reply to Anthony Lam (:antlam) from comment #73)
> Hm, what's missing here?

I'm not sure how complete the functionality in the obsolete patches was but they were using an unideal approach – there was a lot of duplicated code. I think it'd be non-trivial to finish it off so I'd say it should remain mentorable.

Comment 75

[BMO Automation]

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.