1122302 - Replace "Add to Firefox" with "Send to other devices" menu item

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

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

Attachment: prev_sharetofx_mock1.png

Related to bug 1100479, this share menu also seems like a good place to address concerns of "Add to Firefox when you're within Firefox is awkwardly hard to get to".

So, I'd like to propose that we do something like this. Floating the user's synced devices to the top of this list would offer a quick way to actively "send" a tab to another device. One issue I can see is that the icon is the same for share overlay as well as this new device specific action, but that could be solved.

This would give us more time to deal with bug 1100479 which has other separate issues that might require more thinking.

Comment 1

[Richard Newman [:rnewman]]

Nice!

I think showing a different background shade (or a thin divider), plus using the device icons, should do the trick for disambiguating.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm not so sure about this plan as-is. I think we could end up doing something based from these ideas though.

One issue: We do not use this menu for Gingerbread. We use the standard Android UI and we can't tweak it.
IMO: Not a big deal. We ignore Gingerbread. Those devices need to use the current long-way.

Since we do control the menu, I think we should use a single item in the list for "devices", and tapping it shows a list of devices. I have 8 devices in my Synced Tabs currently. Crowding those on the top of my Share list will upset me.

Also, We need to think a little longer term. Send to TV (Device) will be the next thing we add to this pattern. We can't create a UI that won't scale to hold more types of devices.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> Since we do control the menu, I think we should use a single item in the
> list for "devices", and tapping it shows a list of devices. I have 8 devices
> in my Synced Tabs currently. Crowding those on the top of my Share list will
> upset me.

That's incrementally better than we have now, for sure.

To play devil's advocate here, though: with < 3 devices (i.e., almost everyone) and quickshare, this is exactly the setup we have now. Menu > Firefox icon > tap a device name. I do this all the time, and it's great.

With >= 3 devices, you'd save one click.

So perhaps we should do just what we do for the share overlay itself: if you have < 3 devices, we show them all right in the share list (and hide Send to Firefox, once we address reading list).

If you have more than a couple of devices, we show a "Send to device" item instead.

We could also consider treating a device as a share target, which means it can end up as a quickshare icon itself, thus making send-to-desktop a two-tap proposition. Which would be neat.

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #2)
> I'm not so sure about this plan as-is. I think we could end up doing
> something based from these ideas though.
> 
> One issue: We do not use this menu for Gingerbread. We use the standard
> Android UI and we can't tweak it.
> IMO: Not a big deal. We ignore Gingerbread. Those devices need to use the
> current long-way.

I'm ok with that :)

> Since we do control the menu, I think we should use a single item in the
> list for "devices", and tapping it shows a list of devices. I have 8 devices
> in my Synced Tabs currently. Crowding those on the top of my Share list will
> upset me.

Fair point. I think although most users won't have above 3, it'd be nice to have some sort of responsiveness in the design to handle this. Any data we have (on how many devices user's sync) might help too. I feel like I've seen that somewhere... maybe I'm wrong.

> Also, We need to think a little longer term. Send to TV (Device) will be the
> next thing we add to this pattern. We can't create a UI that won't scale to
> hold more types of devices.

Agree. If we use Android standard menu's this should be pretty extendible. I.e. kinda like we currently do with the dark context menu of devices (side note: we could probably update that too? :)

(In reply to Richard Newman [:rnewman] from comment #3)
> So perhaps we should do just what we do for the share overlay itself: if you
> have < 3 devices, we show them all right in the share list (and hide Send to
> Firefox, once we address reading list).

I am ok with showing the individual devices like I've mocked up above in this (< 3 devices) situation, but will need to think more clearly about removing "Send to Firefox"/"Add to Firefox".

> If you have more than a couple of devices, we show a "Send to device" item
> instead.

I'm ok with this!

> We could also consider treating a device as a share target, which means it
> can end up as a quickshare icon itself, thus making send-to-desktop a
> two-tap proposition. Which would be neat.

Agree. This is a good idea and something I hoped to do with this design too. Maybe this should be filed and addressed as a "V2" feature? I can foresee a couple nuances that will probably come up.

Comment 5

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

Attachment: prev_sharetofx_mock2.png

Something like this? We could also use the Shareplane instead of the build icon for "Send to other devices" as we do in the Share overlay.

Note: empty items are just old builds that had "Send tab to devices" which was just confusing for mock purposes.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #5)
> Created attachment 8551508 [details]
> prev_sharetofx_mock2.png
> 
> Something like this? We could also use the Shareplane instead of the build
> icon for "Send to other devices" as we do in the Share overlay.

This still doesn't feel right to me. It seems like we are trying to make one menu do way too much.

Also, keep in mind that no other Share menu in Fennec will have this feature, only the main menu entry point. That's probably not a big deal, but it does have consistency issues for users. Some other screen shots coming up...

Comment 7

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

Attachment: Screen Shot 2015-01-23 at 12.38.22 PM.png

(In reply to Mark Finkle (:mfinkle) from comment #6)
> This still doesn't feel right to me. It seems like we are trying to make one
> menu do way too much.

From within Firefox though, this is the place users would ideally go to to share things. Adding these specific "devices" might seem a bit much but I'd like it if we could funnel user's to a consistent part of town to do all their sharing related activities (3-dot > share). I've considered pulling it even 1 step higher but it seemed more out of place.

Attaching diagram of current sending/receiving experience from within Firefox (share overlay and this menu)

> Also, keep in mind that no other Share menu in Fennec will have this
> feature, only the main menu entry point. That's probably not a big deal, but
> it does have consistency issues for users. Some other screen shots coming
> up...

I agree, the consistency with other menus is a legitimate concern but if I understand this correctly, it probably is something we have to deal with separately depending on the use case (coming from outside Firefox particularly, which Android "Share" menu said app decided to use, etc).

Will keep thinking about this too

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: docs-sharing.png

Google Docs application sharing entry point

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: docs-export-sharing.png

Google Docs application split: Sharing and Export and Other

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: pocket-share.png

Pocket Share entry point: "Send to Friend" is custom and only shown when inside Pocket. Not available from outside Pocket.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

I chatted with Richard a little about this and came to the realization that we might be trying to solve too much with the proposed design. The problem we are trying to solve: Sending to a device from inside Firefox means Menu > Share > Add to Firefox > Display an overlay (with Add Bookmark, Add to Reading List and Send to Device).

I'm hoping we just fix the very obtuse, undiscoverable situation we have right now. We could show a simple UI, maybe just add "Send to Device", in the Share menu list. That action would popup a list of devices. This experience alone is better than our current situation. And it does not create a new situation that doesn't scale. Is it ideal? No. I'm sure we could create a more slick approach, but do we need to?

Comment 12

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

Attachment: prev_sharetofx_mock3.png

(In reply to Mark Finkle (:mfinkle) from comment #11)
> I chatted with Richard a little about this and came to the realization that
> we might be trying to solve too much with the proposed design. The problem
> we are trying to solve: Sending to a device from inside Firefox means Menu >
> Share > Add to Firefox > Display an overlay (with Add Bookmark, Add to
> Reading List and Send to Device).
> 
> I'm hoping we just fix the very obtuse, undiscoverable situation we have
> right now. We could show a simple UI, maybe just add "Send to Device", in
> the Share menu list. That action would popup a list of devices. This
> experience alone is better than our current situation. And it does not
> create a new situation that doesn't scale. Is it ideal? No. I'm sure we
> could create a more slick approach, but do we need to?

I guess for me it seems like adding another item in that Share menu ("Send to devices" or whatever we call it) while keeping "Add to Firefox" seems like we're heading in the opposite direction of bug 1100479. I'm inclined to suggest that if we're adding something there, we should just directly add the devices (that the user's trying to get to anyways).

I agree, some edge cases might seem weird (Mark's 8 devices) but all that aside, because it's an edge case and not ideal either, I also don't think we need to worry about that just yet.

I'm attaching a mock of what I think we're suggesting right now (on the right) so that we can compare it to the "Less than 3 devices" mock in prev_sharetofx_mock2.

Note: ignore the old builds that add items with "Send Tab to Devices" to this list.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #12)
> (In reply to Mark Finkle (:mfinkle) from comment #11)
> > I chatted with Richard a little about this and came to the realization that
> > we might be trying to solve too much with the proposed design. The problem
> > we are trying to solve: Sending to a device from inside Firefox means Menu >
> > Share > Add to Firefox > Display an overlay (with Add Bookmark, Add to
> > Reading List and Send to Device).
> > 
> > I'm hoping we just fix the very obtuse, undiscoverable situation we have
> > right now. We could show a simple UI, maybe just add "Send to Device", in
> > the Share menu list. That action would popup a list of devices. This
> > experience alone is better than our current situation. And it does not
> > create a new situation that doesn't scale. Is it ideal? No. I'm sure we
> > could create a more slick approach, but do we need to?
> 
> I guess for me it seems like adding another item in that Share menu ("Send
> to devices" or whatever we call it) while keeping "Add to Firefox" seems
> like we're heading in the opposite direction of bug 1100479. I'm inclined to
> suggest that if we're adding something there, we should just directly add
> the devices (that the user's trying to get to anyways).

If we add a "Send to Device" item to the menu, we should hide the "Add to Xxx" item (where "Xxx" is Nightly, Aurora, Firefox Beta or Firefox).

> I agree, some edge cases might seem weird (Mark's 8 devices) but all that
> aside, because it's an edge case and not ideal either, I also don't think we
> need to worry about that just yet.

Even with 1 or 2 devices, I think the inconsistency in the items is a problem. Some are devices and some are Android activities. Some can be promoted to Quick Share and some can't. Some move up and down the list based on usage, and some don't.

> I'm attaching a mock of what I think we're suggesting right now (on the
> right) so that we can compare it to the "Less than 3 devices" mock in
> prev_sharetofx_mock2.

The only change I would suggest to the mock is to remove the "Add to Firefox" item. Also, "Send to Device" would only be added when in Firefox. It should not be available from external applications. The "Add to Firefox" is the external application entry point.

Anthony and I chatted a bit about this design. Moving to "mock3" is a good incremental step. It reduces the number of taps needed to send to a device. It makes the "Send to Device" more prominent from within the Firefox. It even allows increased usage to promote it to the Quick Share menu items. It should also be available from the "long tap on a link" context menu.

Comment 14

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

(In reply to Mark Finkle (:mfinkle) from comment #13)
> The only change I would suggest to the mock is to remove the "Add to
> Firefox" item. Also, "Send to Device" would only be added when in Firefox.
> It should not be available from external applications. The "Add to Firefox"
> is the external application entry point.
> 
> Anthony and I chatted a bit about this design. Moving to "mock3" is a good
> incremental step. It reduces the number of taps needed to send to a device.
> It makes the "Send to Device" more prominent from within the Firefox. It
> even allows increased usage to promote it to the Quick Share menu items. It
> should also be available from the "long tap on a link" context menu.

Though, talking about the possibility of adding this to "quick share", isn't it still possible regardless? We could also leverage an "undo" super toast if there are concerns around user expectation of what tapping a "device name" (with a build icon next to it) does. 

That being said, I'm OK with this as a starting point. I still think there's room for improvement here but I agree about the consistency issue.

Comment 15

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

My interpretation of what should be done (via comment 13 and comment 14):

Implement mock3 but:
  * Remove "Add to Firefox"
  * "Send tab to device" acts like other menu items (e.g. no special priority at the top, accessible via quick share)
  * (assumption) when "Send tab to device" is clicked 

Is this right, Anthony?

Comment 16

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

My interpretation of what should be done (via comment 13 and comment 14):

Implement mock3 but:
  * Remove "Add to Firefox"
  * "Send tab to device" acts like other menu items (e.g. no special priority at the top, accessible via quick share)
  * (assumption) when "Send tab to device" is clicked, a menu will appear similar to the "Share overlay" that lists the available devices.
  * (assumption) "Send tab to device" should not appear if there are no other devices to connect to

Is this right, Anthony & Finkle?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #16)
> My interpretation of what should be done (via comment 13 and comment 14):
> 
> Implement mock3 but:
>   * Remove "Add to Firefox"
>   * "Send tab to device" acts like other menu items (e.g. no special
> priority at the top, accessible via quick share)
>   * (assumption) when "Send tab to device" is clicked, a menu will appear
> similar to the "Share overlay" that lists the available devices.

By "Share overlay" I would expect the list of devices that the Share overly would show, but not the Share overlay itself.

>   * (assumption) "Send tab to device" should not appear if there are no
> other devices to connect to
> 
> Is this right, Anthony & Finkle?

Otherwise, seems correct.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #16)
> My interpretation of what should be done (via comment 13 and comment 14):
> 
> Implement mock3 but:
>   * Remove "Add to Firefox"

This assumes that "Send to Device" is not visible from outside Firefox too. I mean, it's not a Share target that appears when other apps attempt to share. Those apps would see "Add to Firefox".

Comment 19

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

Finkle, two questions:
  1) Should I remove "Add to Firefox" for the current channel only, or all channels?
  2) Are there any changes that should take place on the web context menus (e.g. sharing a link)?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

1. Let's start with removing just the same channel
2. I would focus on the main menu to start, and see what's possible with the web context menu. It could be a different beast and have it's own challenges.

I remember from when we were adding telemetry that some of the logic is in this code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java

Comment 21

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

(In reply to Mark Finkle (:mfinkle) from comment #20)
> I remember from when we were adding telemetry that some of the logic is in
> this code:

Sub-menu: GeckoActionProvider.onPrepareSubmenu [1]
Context menus: GeckoActionProvider.getSortedActivities [2]

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java?rev=1f9b1f477588#123
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java?rev=1f9b1f477588#163

Comment 22

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

Attachment: MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302: Remove "Add to Firefox" from the toolbar and context menu for all channels. r=wesj

Pull down this commit:

hg pull review -r 498a2ccd40a4675d4236dcc184705c26fac39dd3

Comment 23

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

Anthony, do you have an UX ideas here on a "Send tab to device" dialog? Should I just use the share overlay without "bookmark" and "reading list" for now?

---

The commit in comment 22 are not enough to solve this bug. Note that the commit there also does not affect sharing from the action bar (but does affect long-press link/image context menus).

The next step is to add a SendTabToDeviceDialog Activity and add it to the AndroidManifest (exported=false so it cannot be accessed by other applications).

We then could then:
  1) Allow Android to do it's own intent matching (which would show "Send Tab to Device" any time we use ACTION_SEND)
  2) Wrap Android's intent matching to manage our two sources (ActivityChooserModel & packageManager.queryIntentActivities) of Intent resolution (and thus only include our Activity under certain conditions such as when some extra is present, or by doing multiple Intent queries)
  3) Insert our activity directly into GeckoActionProvider.onPrepareSubmenu.

I'm thinking (3) because we don't want "Send Tab to Device" on non-URLs and it's a good baby step.

Code from the ShareDialog can be refactored out to a super class to be shared - how much depends on how much visual/animation content is shared between the Activities. The SendTabList and/or the SendTabDeviceListArrayAdapter can also be used to fill this dialog.

Comment 24

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

(In reply to Michael Comella (:mcomella) from comment #16)
> My interpretation of what should be done (via comment 13 and comment 14):
> 
> Implement mock3 but:
>   * Remove "Add to Firefox"
>   * "Send tab to device" acts like other menu items (e.g. no special
> priority at the top, accessible via quick share)
>   * (assumption) when "Send tab to device" is clicked, a menu will appear
> similar to the "Share overlay" that lists the available devices.
>   * (assumption) "Send tab to device" should not appear if there are no
> other devices to connect to
> 
> Is this right, Anthony & Finkle?

Sounds good to me. Just wondering, is there a reason why we're not floating this to the top though?

(In reply to Mark Finkle (:mfinkle) from comment #18)
> (In reply to Michael Comella (:mcomella) from comment #16)
> > My interpretation of what should be done (via comment 13 and comment 14):
> > 
> > Implement mock3 but:
> >   * Remove "Add to Firefox"
> 
> This assumes that "Send to Device" is not visible from outside Firefox too.
> I mean, it's not a Share target that appears when other apps attempt to
> share. Those apps would see "Add to Firefox".

Yep.

(In reply to Michael Comella (:mcomella) from comment #23)
> Anthony, do you have an UX ideas here on a "Send tab to device" dialog?
> Should I just use the share overlay without "bookmark" and "reading list"
> for now?

Sure, we could use our device icons on the left hand side too. Lets see how this feels. 

As a backup, we can use system default for these dialogs (E.g. http://www.google.com/design/spec/components/dialogs.html#dialogs-simple-dialogs for L). This makes it easier to wrangle all of these in the future and avoids UI awkwardness in the meantime.

Comment 25

[Richard Newman [:rnewman]]

Product meeting suggests that this is tracking 38.

Comment 26

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

Attachment: (Prototype) Screenshot

Anthony, what do you think?

Prototype build: https://people.mozilla.com/~mcomella/apks/mcomella-1122302_01.apk

Note that the interaction for not being signed into a sync account is not yet mocked out but I imagine it'd open the Sync Setup Activity. Also, not mocked for having no other devices - perhaps a tutorial page (contextual hints)?

Comment 27

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

Comment on attachment 8562491 [details]
(Prototype) Screenshot

Great! + for progress! This is coming along nicely. Can't wait to see how this feels with the visual changes in bug 1059554 ;)

Comment 28

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302: Remove "Add to Firefox" from the toolbar and context menu for all channels. r=wesj
/r/3765 - Bug 1122302 - Move send tab portions of ShareDialog into AbstractSendTabDialog. r=rnewman
/r/3767 - Bug 1122302 - Clean up ShareDialog & AbstractSendTabDialog. r=rnewman
/r/3769 - Bug 1122302 - Get TelemetryMethod from sub-class in AbstractSendTabDialog. r=rnewman
/r/3771 - Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman
/r/3773 - Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman

Pull down these commits:

hg pull review -r 9ba084c02bdde8fa588cb20061735ab3bc45653f

Comment 29

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

comment 28 is non-final. Still TODO:
  * Add the SendTabDialog, and customize the SendTabList to always display all devices in that dialog (rather than changing state to a button to open a sub-menu based on the number of devices).

Comment 30

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302: Remove "Add to Firefox" from the toolbar and context menu for all channels. r=wesj
/r/3765 - Bug 1122302 - Move send tab portions of ShareDialog into AbstractSendTabDialog. r=rnewman
/r/3767 - Bug 1122302 - Clean up ShareDialog & AbstractSendTabDialog. r=rnewman
/r/3769 - Bug 1122302 - Get TelemetryMethod from sub-class in AbstractSendTabDialog. r=rnewman
/r/3771 - Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman
/r/3773 - Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman
/r/3815 - Bug 1122302 - Add SendTabDialog. r=rnewman

Pull down these commits:

hg pull review -r 6c2586010891391b27e20a040f3ad48f6b3b5936

Comment 31

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

comment 30 is non-final. Still TODO:
  * The "Send tab to device" menu appears in both long press context menus and the app menu. This is tricky to add - what we really want is to union the results of multiple Intent actions and have an action exclusive to the SendTabDialog. However, we can't! I tried adding the SendTabDialog Activity to the returned list in ActivityChooserModel.loadActivitiesIfNeeded, but the history file is shared between the app menu and the long-press context menu (causing inconsistent state when the activities are loaded), so we need to keep two files in order to use this method. Because we have to take the SendTabDialog into account in the history file, this method may be the only way to do it.

Comment 32

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

Since we're tracking 38 (aurora), we'd have to uplift Strings but I checked with :antlam and he's fine using "Send tab to devices", which is already in the tree.

Comment 33

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

NI :antlam for toast string when you click "send tab to devices" and the current device is the only device set up in sync.

Plan, checked w/ antlam: If no devices are setup and "send tab to devices" is pressed, display the sync setup activity. If one device is synced (i.e. the current one), display a toast saying you can't share (see above). And proceed as normal if there is a device to share to. 

(In reply to Michael Comella (:mcomella) from comment #32)
> Since we're tracking 38 (aurora), we'd have to uplift Strings but I checked
> with :antlam and he's fine using "Send tab to devices", which is already in
> the tree.

We should fix the capitalization though (from Finkle) - it's "Send Tab to Devices" in the tree.

Comment 34

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

(In reply to Michael Comella (:mcomella) from comment #33)
> Plan, checked w/ antlam:

Nevermind - bad plan. Reconvene tomorrow with :antlam (and maybe rnewman) to determine a better UI flow. Simplest solution: exclude the menu item when no other devices are synced or sync is not setup. We can blank the cache to do this via the ActivityChooserModel.mReloadActivities and maybe attaching a sync account listener. Watch for perf!

Comment 35

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302: Remove "Add to Firefox" from the toolbar and context menu for all channels. r=wesj
/r/3765 - Bug 1122302 - Move send tab portions of ShareDialog into AbstractSendTabDialog. r=rnewman
/r/3767 - Bug 1122302 - Clean up ShareDialog & AbstractSendTabDialog. r=rnewman
/r/3769 - Bug 1122302 - Get TelemetryMethod from sub-class in AbstractSendTabDialog. r=rnewman
/r/3771 - Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman
/r/3773 - Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman
/r/3815 - Bug 1122302 - Add SendTabDialog. r=rnewman
/r/4293 - Bug 1122302 - Display SendTabDialog only in sub menu. r=rnewman

Pull down these commits:

hg pull review -r 4c1643421995aa33341ee6420a11a1f0929242cb

Comment 36

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

Still TODO:
  * Figure out flow for non-synced, or synced w/ no other devices (comment 34, followup?)
  * Remove "Add to Firefox" from reading list share menu (and wherever else share is used that is not the sub menu or context menu - don't forget to file a bug to unify our sharing access so we don't have to eliminate this on a case-by-case basis because it's hacky and it sucks)

Comment 37

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

Had a discussion w/ antlam and rnewman:

We decide that having a "Send tab to devices" menu option rather than an "Add to firefox" option isn't actually that useful - given that users with 2 (or is it 3?) or less devices see their devices directly in the "Add to firefox" menu, it's the same number of clicks as "Send tab to devices" would be (and thus "send tab to devices" just serves to remove the redundant "Add to Reading List" and "Add to bookmarks").

Instead, we decided on something akin to mock1 (left image) [1] where we have a sectioned-off list of devices floated to the top of the menu, which for most users (< 3 devices) won't be overwhelming. These do not move to quick share (and thus do not interact with the ActivityChooserModel). If there are no devices synced, this sectioned-off menu will not be shown.

Potential v2 ideas are to add quick share capability (maybe click for a list of devices in a style like the share overlay) and to allow this sectioned off menu to display different content based on synced state ("Sign up for sync!", "Sign into another device to share to other devices!"). For users with a large number of devices, we could also fold-close the tail end of their devices. 

[1]: https://bug1122302.bugzilla.mozilla.org/attachment.cgi?id=8549946

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #37)
> Had a discussion w/ antlam and rnewman:
> 
> We decide that having a "Send tab to devices" menu option rather than an
> "Add to firefox" option isn't actually that useful - given that users with 2
> (or is it 3?) or less devices see their devices directly in the "Add to
> firefox" menu, it's the same number of clicks as "Send tab to devices" would
> be (and thus "send tab to devices" just serves to remove the redundant "Add
> to Reading List" and "Add to bookmarks").
> 
> Instead, we decided on something akin to mock1 (left image) [1] where we
> have a sectioned-off list of devices floated to the top of the menu, which
> for most users (< 3 devices) won't be overwhelming. These do not move to
> quick share (and thus do not interact with the ActivityChooserModel). If
> there are no devices synced, this sectioned-off menu will not be shown.

I am against this idea, as previously stated in this bug. Creating a menu that is not homogenous list of items will be confusing to the user. Not only are the "device" items different from the "activity" items, they don't sort the way the "activity" items sort.

Also, it's a crime if we don't support "uplifting" items in this menu to Quick Share.

I don't see a way to style the "device" items in a way that allows people to get the gist of what we are trying to do. They just won't get it.

> Potential v2 ideas are to add quick share capability (maybe click for a list
> of devices in a style like the share overlay) and to allow this sectioned
> off menu to display different content based on synced state ("Sign up for
> sync!", "Sign into another device to share to other devices!"). For users
> with a large number of devices, we could also fold-close the tail end of
> their devices. 

I thought the next evolution of this concept was to add an "airplane" icon to the QuickShare area of the main menu, thereby removing the need put "devices" in the Share list altogether.

Comment 39

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #38)

> I am against this idea, as previously stated in this bug. Creating a menu
> that is not homogenous list of items will be confusing to the user.

Why do you think that?

I would imagine that

    ==============================
    = Your devices               =
    | [] Richard's desktop       |
    | [] Richard's tablet        |
    ------------------------------
    = Other apps                 =
    | Add to dropbox             |
    | Send email                 |
    | ...                        |

is pretty amazingly clear. Certainly more so than the user having to find and figure out what "Send Tab" means.

> Also, it's a crime if we don't support "uplifting" items in this menu to
> Quick Share.

That's planned for v2, but it involves touching ActivityChooserModel to aggregate different share devices into a single share activity (because we can't put individual devices there, because they're not labeled).
 

> I thought the next evolution of this concept was to add an "airplane" icon
> to the QuickShare area of the main menu, thereby removing the need put
> "devices" in the Share list altogether.

I see:

v1: add a "devices" section to the share menu.
v2: allow "send to device" as an aggregate action to earn itself a spot in QuickShare.

Comment 40

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

Attachment: v1 mock

Talked to mcomella about this more. 

Considering: 
 - that most users will not have more than 3 devices, 
 - that we want this to be able to move into quick share, 
 - that we have plans to move the shareplane icon out to the top level of the overflow menu, 
 - and our time constraint, 

I think that this original proposition is the best (attached). 

I've opted to use the "Shareplane" icon here (as opposed to simply the build icon) as part of the larger (cross-product) initiative to reserve this icon as our "sending something" icon. Keeping just 1 item in the "Share menu" will also allow for a more straight forward approach when this item appears in 1 of 2 (or 3) quick share spots for the user. 

Yes, this doesn't really save a click for users (compared to our idea of pulling out the individual device in this list) but I think the better, more thorough approach would be to address that UX issue in "V2" of these improvements (outlined below). Trying to "save a click/press" in this stage creates too many edge cases that our menu system just isn't built to handle and the trade-offs just don't seem to be worth it.

Pressing this "Send to other devices" button will simply trigger a list of devices (unless they have 1, in which case pressing it will just send to that device directly, with on-screen feedback) that looks like the Share overlay. This behaviour will be kept consistent (the same) when the Shareplane appears in the Quick share spots.

As part of V2 (similar to bug 1127445), pulling the Shareplane up to the first level of this menu will give it more exposure and allow us to save the user an extra click/press. By moving this icon out, we also give ourselves more opportunity to explore other possibilities to improving the UX here (probably V3 type work).

Comment 41

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

Attachment: Potential v2

Idea, WIP, but gives an idea of how the Shareplane might sit on this menu level.

Comment 42

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

Attachment: icon_device_plane.zip

Planes for Share menu, devices for overlay

Comment 43

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman

Pull down these commits:

hg pull review -r e9a53dfb4cbdef920da36678ae36a6cccc237c80

Comment 44

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

Attachment: icon_plane_padded.zip

Comment 45

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302: Remove "Add to Firefox" from the toolbar and context menu for all channels. r=wesj
/r/3765 - Bug 1122302 - Move send tab portions of ShareDialog into AbstractSendTabDialog. r=rnewman
/r/4577 - Bug 1122302 - Clean up ShareDialog & AbstractSendTabDialog. r=rnewman
/r/4579 - Bug 1122302 - Get TelemetryMethod from sub-class in AbstractSendTabDialog. r=rnewman
/r/4581 - Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman
/r/4583 - Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman
/r/4585 - Bug 1122302 - Add SendTabDialog. r=rnewman
/r/4587 - Bug 1122302 - Update "Send to other devices" to final String. r=rnewman

Pull down these commits:

hg pull review -r d6d7efc060f294fad427a1598ff45d9f40ab9f06

Comment 46

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman
/r/4577 - Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman
/r/4579 - Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman

Pull down these commits:

hg pull review -r b0e02debcec70e0715dc05154dc96bd5b851d0b0

Comment 47

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

Still TODO:
  * Get final share plane assets
  * Make ActivityChooserModel cache refresh after account sync (or after device count changes after account sync)
  * We've gotten into some inconsistent states with ^ and below where we have no devices to sync to but the option appears in the menu, so we should have a worst-case escape route where we show a toast rather a dialog with no devices.

Note: we rely on the ClientsDatabaseAccessor to get the client count, but this
database is not purged when an Account is removed from the device and can
contain stale information. I think there is a mentor bug open about this somewhere (where the share overlay's device list has this problem).

Comment 48

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

(In reply to Michael Comella (:mcomella) from comment #47)
> Note: we rely on the ClientsDatabaseAccessor to get the client count, but
> this
> database is not purged when an Account is removed from the device and can
> contain stale information. I think there is a mentor bug open about this
> somewhere (where the share overlay's device list has this problem).

bug 830270.

Comment 49

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

I attempted to refactor with Loaders but it adds a lot of complexity for little gain (particularly for an uplift) so I dropped that path.

Comment 50

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman
/r/4577 - Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman
/r/4579 - Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman
/r/4725 - Bug 1122302 - Reload activities in ActivityChooserModel on sync. r=rnewman
/r/4727 - Bug 1122302 - Add escape hatch in case clients list is empty in share overlay. r=rnewman

Pull down these commits:

hg pull review -r ea4ee26a4a4fe07912f878d5608eacffd91b87fd

Comment 51

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

Still TODO:
  * Final share plane assets
  * Final escape hatch String

Some blocked bugs might be useful too (e.g. bug 1139230).

Comment 52

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman
/r/4577 - Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman
/r/4579 - Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman
/r/4725 - Bug 1122302 - Reload activities in ActivityChooserModel on sync. r=rnewman
/r/4727 - Bug 1122302 - Add escape hatch in case clients list is empty in share overlay. r=rnewman

Pull down these commits:

hg pull review -r 8767a2f91bf50f699440fdf83050662fdd727e4c

Comment 53

[Richard Newman [:rnewman]]

So... this seems to function (haven't looked at the code yet), but I have some UX objections.


* One of the reasons we picked "Add to Firefox" is that it puts us near the top of the list. That applies here, too; I had to search for "Send to other devices", and I was looking for it!

Our share list is randomly ordered, and this option is hiding in the middle. Until you've used it, it doesn't appear in quickshare, and for me it's buried way below the fold.

* If you have only one device, the list is a long way away from your first press -- it shoots up from the bottom of your screen, so you need to reposition your hand to tap your device.

The existing layout made sense for two reasons:

  1. We also had bookmark and reading list, so you have at least three rows plus a header. With only one row you can mistake this for a toast.

  2. You enter the share overlay from another app, where you're looking at content.

I don't think either of these constraints apply now. Here you enter the share overlay from an almost-full-screen menu; we don't need to preserve context, and often you'll only have one row.


If we're committed to immediately doing better (without implying that this is bad work!), then I think this is a reasonable starting point. But I'd strongly suggest 'pinning' "Send to other devices" to the top of the Share menu for users who have Sync set up; without it the feature is even less discoverable than it is now. And I'd suggest considering whether we should do a top-down or left-right transition in this context.

(Yes, I consider "in-app" share destinations to be fundamentally separable from external share destinations, and I think we should help users to find them.)

Comment 54

[Richard Newman [:rnewman]]

Also:

The "tap the fox" functionality still works... and it opens the same page in a new tab. Let's not do that.

Comment 55

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

Anthony, thoughts on comment 53?

Comment 56

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

Note: found a bug where the share dialog displays the device list from external apps if "Send to other devices" is selected from within Firefox first.

Comment 57

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #53)

> * One of the reasons we picked "Add to Firefox" is that it puts us near the
> top of the list. That applies here, too; I had to search for "Send to other
> devices", and I was looking for it!

If you're making an argument based on alphabetical order, this won't hold for other locales.

> Our share list is randomly ordered, and this option is hiding in the middle.
> Until you've used it, it doesn't appear in quickshare, and for me it's
> buried way below the fold.

Can we change how we order the share list to special-case this to the top? I imagine we have the ability to do this. Follow-up bug?

> * If you have only one device, the list is a long way away from your first
> press -- it shoots up from the bottom of your screen, so you need to
> reposition your hand to tap your device.
> 
> The existing layout made sense for two reasons:
> 
>   1. We also had bookmark and reading list, so you have at least three rows
> plus a header. With only one row you can mistake this for a toast.
> 
>   2. You enter the share overlay from another app, where you're looking at
> content.
> 
> I don't think either of these constraints apply now. Here you enter the
> share overlay from an almost-full-screen menu; we don't need to preserve
> context, and often you'll only have one row.

I think these two points are valid, maybe we can bring this up in some sort of dialog in the middle of the screen, instead of at the bottom? But this sounds like an improvement we can make in a future patch.

Comment 58

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

(In reply to Richard Newman [:rnewman] from comment #53)
> So... this seems to function (haven't looked at the code yet), but I have
> some UX objections.
> 
> * One of the reasons we picked "Add to Firefox" is that it puts us near the
> top of the list. That applies here, too; I had to search for "Send to other
> devices", and I was looking for it!
> 
> Our share list is randomly ordered, and this option is hiding in the middle.
> Until you've used it, it doesn't appear in quickshare, and for me it's
> buried way below the fold.
> 
> * If you have only one device, the list is a long way away from your first
> press -- it shoots up from the bottom of your screen, so you need to
> reposition your hand to tap your device.

I agree, and a lot of these usability/ discoverability concerns seem to be great reasons we should take a step back and look at WHERE we should be placing this Shareplane. I think in the long run, we should pull out this "Send to device" button into the level above this "Share to" list. 

See bug 1140048 filed

> And I'd suggest considering whether we should
> do a top-down or left-right transition in this context.

I see what you're saying. But the current "top-down" transition is there to keep consistent with the other "dialogs" that come in as well as the "Share overlay" that the user will get to from outside Firefox. We're looking to brand the interactions and animations here, essentially. 

Consider the "from top" animations for things like doorhangers to be coming from Firefox, where as the "from/to bottom" animations are going out of Firefox. I've been trying to establish a cohesive system of metaphors and interactions that keep consistent with each other. Adding a third "from the side" space/metaphor needs to be considered thoroughly and used sparingly so we don't end up with many different spacial models for every interaction.

Comment 59

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #57)

> If you're making an argument based on alphabetical order, this won't hold
> for other locales.

We surveyed a bunch when we were deciding what to call "Add to Firefox", and in the majority of our translations, "add" comes in way before "send" -- e.g., es_MX "Añadir a…" versus "Enviar al…".

So no, not all locales, but most, and we did check.


> Can we change how we order the share list to special-case this to the top? I
> imagine we have the ability to do this. Follow-up bug?

That was my suggestion, yeah. We should be pinning our internal actions to the top of the share list.

But I'd do it now, not as a follow-up; we're removing Add to Firefox from the list, and making its replacement really hard to find, so I think of that as a regression that should be addressed when we land this, not a follow-up.

Comment 60

[Richard Newman [:rnewman]]

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

Clearing flag pending comments 53 and onward.

Comment 61

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

Consensus from vidyo meeting: We add the "Send tab to other devices" menu item (as discussed) to the share menu and pin it to the top-ish (at most item 3 so it can get into quick share but stay at the top - we won't pin it directly to the top due to ActivityChooserModel difficulties).

Note:
  * The share menu is not ordered alphabetically, but randomly, so the name doesn't actually matter.
  * We will potentially move internal share actions out from the share menu at a later date (e.g. share plane in bug 1140048), to a more versatile system

Comment 62

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

(In reply to Michael Comella (:mcomella) from comment #56)
> Note: found a bug where the share dialog displays the device list from
> external apps if "Send to other devices" is selected from within Firefox
> first.

I can only repro on L because of bug 1137928.

Comment 63

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

(In reply to Michael Comella (:mcomella) from comment #62)
> (In reply to Michael Comella (:mcomella) from comment #56)
> > Note: found a bug where the share dialog displays the device list from
> > external apps if "Send to other devices" is selected from within Firefox
> > first.
> 
> I can only repro on L because of bug 1137928.

To clarify, the dialog has to remain open in the first Firefox, i.e. the Activity is being shared.

Comment 64

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

Interesting alternative (but probably too late to go back): if we made "Send tab to other devices" a separate Activity, we might be able to use Menu.addIntentOptions [1].

[1]: https://developer.android.com/reference/android/view/Menu.html#addIntentOptions%28int,%20int,%20int,%20android.content.ComponentName,%20android.content.Intent[],%20android.content.Intent,%20int,%20android.view.MenuItem[]%29

Comment 65

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman
/r/4577 - Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman
/r/4579 - Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman
/r/4725 - Bug 1122302 - Reload activities in ActivityChooserModel on sync. r=rnewman
/r/4727 - Bug 1122302 - Add escape hatch in case clients list is empty in share overlay. r=rnewman
/r/5279 - Bug 1122302 - Handle reusing the share overlay. r=rnewman
/r/5281 - Bug 1122302 - Correct the size of the share plane in the quick share menu. r=rnewman
/r/5283 - Bug 1122302 - Make the device list in the share overlay always show up only for the current channel. r=rnewman
/r/5285 - Bug 1122302 - Pin internal actions to the top of the share menu. r=rnewman
/r/5287 - Bug 1122302 - Small perf improvement in ActivityChooserModel. r=rnewman

Pull down these commits:

hg pull review -r a6b1ecd806258d823862c657d92077b5ff56b09d

Comment 66

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

Note for changesets in comment 65: we discussed getting the "Send tab to other devices" action it the top 3 items but I think getting it to the top item was desired & I was able to get it to there.

Comment 67

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

/r/3635 - Bug 1122302 - Add share plane assets. r=rnewman
/r/3765 - Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman
/r/4577 - Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman
/r/4579 - Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman
/r/4725 - Bug 1122302 - Reload activities in ActivityChooserModel on sync. r=rnewman
/r/4727 - Bug 1122302 - Add escape hatch in case clients list is empty in share overlay. r=rnewman
/r/5279 - Bug 1122302 - Handle reusing the share overlay. r=rnewman
/r/5281 - Bug 1122302 - Correct the size of the share plane in the quick share menu. r=rnewman
/r/5283 - Bug 1122302 - Make the device list in the share overlay always show up only for the current channel. r=rnewman
/r/5285 - Bug 1122302 - Pin internal actions to the top of the share menu. r=rnewman
/r/5287 - Bug 1122302 - Small perf improvement in ActivityChooserModel. r=rnewman
/r/5291 - Bug 1139230 - Enable scrolling on the share overlay. r=rnewman

Pull down these commits:

hg pull review -r 8c36347bb2f3ce6c90722d002bf21d541c8d9282

Comment 70

[Richard Newman [:rnewman]]

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

https://reviewboard.mozilla.org/r/3633/#review4287

::: mobile/android/base/AndroidManifest.xml.in
(Diff revision 11)
> +             Setting launchMode="singleTop" ensure onNewIntent is called when the Activity is

ensures

::: mobile/android/base/locales/en-US/android_strings.dtd
(Diff revision 11)
> +<!ENTITY overlay_no_synced_devices "No Firefox Account connected devices found">

This should really be "No Firefox Account-connected devices found". That looks a little odd with the noun phrase, though.

This string is also not all that actionable.

If this is only something shown in extremis -- that is, we ordinarily hide the menu item -- then fine. If not, we should probably very quickly iterate on the wording to get something that's a little closer to giving the user a clue.

For example:

"You have no other devices syncing with your Firefox Account"

"No devices found that can receive sent tabs"

or something.

::: mobile/android/base/overlays/ui/ShareDialog.java
(Diff revision 11)
> +        if (state == State.DEVICES_ONLY && clientrecords.length == 0) {

Might wanna null-check clientrecords here.

::: mobile/android/base/overlays/ui/ShareDialog.java
(Diff revision 11)
> +                State.DEVICES_ONLY : State.DEFAULT);

No need for the enclosing parens, which will also fix the indentation nit on this line :)

::: mobile/android/base/overlays/ui/ShareDialog.java
(Diff revision 11)
> -        final String bookmarkEnabledLabel = resources.getString(R.string.overlay_share_bookmark_btn_label);
> +            foxIcon.setOnClickListener(null);

Thanks.

::: mobile/android/base/widget/ActivityChooserModel.java
(Diff revision 11)
> +                    if (getOtherSyncedClientCount() <= 0) {

We know that the activity will always be found in the list somewhere. We'll call getOtherSyncedClientCount once for sure.

So just compute this once, outside the loop, as a final boolean. Don't bother doing the two string comparisons inside the loop if it's false.

::: mobile/android/base/widget/ActivityChooserModel.java
(Diff revision 11)
> +    private int getOtherSyncedClientCount() {

Why not make this `hasOtherSyncClients`?

::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 11)
> +                    sendTabLabel.equals(activityLabel)) {

Align "send" under "share".

::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 11)
> +            subMenu.add(0, i, order, activityLabel)

Strictly speaking we should add `Menu.FIRST` to each order.

::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 11)
> +                order = i | Menu.CATEGORY_SECONDARY;

Make sure we test this!

Comment 71

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

https://reviewboard.mozilla.org/r/3633/#review4317

> No need for the enclosing parens, which will also fix the indentation nit on this line :)

I actually prefer hanging double indentations so that was fine. :P However, removed the parens to make us both happy.

> This should really be "No Firefox Account-connected devices found". That looks a little odd with the noun phrase, though.
> 
> This string is also not all that actionable.
> 
> If this is only something shown in extremis -- that is, we ordinarily hide the menu item -- then fine. If not, we should probably very quickly iterate on the wording to get something that's a little closer to giving the user a clue.
> 
> For example:
> 
> "You have no other devices syncing with your Firefox Account"
> 
> "No devices found that can receive sent tabs"
> 
> or something.

This is intended never to be shown so I think the wording is fine.

e.g. I'm plan to remove this escape hatch in the uplift to Aurora so we don't have to uplift strings.

> We know that the activity will always be found in the list somewhere. We'll call getOtherSyncedClientCount once for sure.
> 
> So just compute this once, outside the loop, as a final boolean. Don't bother doing the two string comparisons inside the loop if it's false.

This Activity is returned as a share list item by the system - if we don't have any sync clients, we want to skip adding it to the share menu so we need to check for this Activity in either case.

> Align "send" under "share".

Again, I prefer double-indent but wfm.

> Strictly speaking we should add `Menu.FIRST` to each order.

From the [docs](https://developer.android.com/reference/android/view/Menu.html#FIRST):

*First value for group and item identifier integers.*

So this seems unnecessary for order.

> Make sure we test this!

Tested locally.

It's just [an integer comparison under the hood](https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenu.java?rev=aadbc0a67a2c#870) and we're ORing (large values)[https://developer.android.com/reference/android/view/Menu.html#CATEGORY_SECONDARY] against our order integers.

Comment 72

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

https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=48a7bd3146e3

Comment 73

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

Fix bustage: https://hg.mozilla.org/integration/fx-team/rev/e1269f306413

Comment 74

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

NI self to let bake and uplift.

Comment 75

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #71)


> *First value for group and item identifier integers.*
> 
> So this seems unnecessary for order.

It means that order numbers start at 1 (FIRST), not at zero. Maybe that's not a constraint in GeckoMenu itself, but that's how this is supposed to work.

Comment 76

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

(In reply to Richard Newman [:rnewman] from comment #75)
> > *First value for group and item identifier integers.*
> > 
> > So this seems unnecessary for order.
> 
> It means that order numbers start at 1 (FIRST), not at zero. Maybe that's
> not a constraint in GeckoMenu itself, but that's how this is supposed to
> work.

public abstract MenuItem add (int groupId, int itemId, int order, int titleRes) [1]

^ It seems Menu.NONE [2] == 0 can be used for all of these values (groupId, itemId, order), so I agree with that though the docs are unclear. Filed bug 1143196.

[1]: https://developer.android.com/reference/android/view/Menu.html#add%28int,%20int,%20int,%20int%29
[2]: https://developer.android.com/reference/android/view/Menu.html#NONE

Comment 77

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7034af721c2f
https://hg.mozilla.org/mozilla-central/rev/54a918565359
https://hg.mozilla.org/mozilla-central/rev/872bbe891097
https://hg.mozilla.org/mozilla-central/rev/bee769cfe045
https://hg.mozilla.org/mozilla-central/rev/6b2ff209f5d9
https://hg.mozilla.org/mozilla-central/rev/13fa33b1876e
https://hg.mozilla.org/mozilla-central/rev/a8ff0289a763
https://hg.mozilla.org/mozilla-central/rev/cac8ee9abe86
https://hg.mozilla.org/mozilla-central/rev/b84463dd7b30
https://hg.mozilla.org/mozilla-central/rev/b27486859693
https://hg.mozilla.org/mozilla-central/rev/4dc8b8d5ab5f
https://hg.mozilla.org/mozilla-central/rev/48a7bd3146e3
https://hg.mozilla.org/mozilla-central/rev/e1269f306413

Comment 78

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

Note: bug 1127445 has to land first for this to land OR this lands, bug 1127445 lands, and then comment 73 lands.

Comment 79

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

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

This applies to this set of changesets and the bustage fix in comment 73 (provided bug 1127445 gets uplifted first).

This should be uplifted with bug 1143759.

Approval Request Comment
[Feature/regressing bug #]: Share overlay

[User impact if declined]:
  Users will use "Add to Firefox" from inside Firefox and outside. This patch makes it so the item is changed to "Send to other devices" inside Firefox and the resulting menu is more streamlined for internal use.

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
  We customize when the menu item is shown based on sync state so we could in theory get that wrong (and hit our this-should-never-happen error case). We're also using some infrequently used sync APIs to identify synced state so it's possible there is a bug in that code that causes this state to appear incorrectly. There were some issues with Android L changing the behavior of the task stack and thus reusing this dialog unexpectedly (bug 1137928), which I attempted to handle but we might have missed an edge case there (which is hit when the user hits the recent app switcher button while the dialog is open and then shares to Firefox again, using the same dialog).

[String/UUID change made/needed]: There is a string added for extreme, this-should-never-happen error conditions, but I plan to remove it for the uplift.

Comment 80

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

I'll handle the uplift in comment 79.

Comment 81

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

bug 1145892 and bug 1145897 should also be uplifted with this bug.

Comment 82

[Sylvestre Ledru [:Sylvestre]]

Michael, this is an important change. Can this wait for 39 ?

Comment 83

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #82)
> Michael, this is an important change. Can this wait for 39 ?

This is roadmapped for 38 because we anticipate the share dialog receiving a lot more usage in 38 - this cleans up the experience and makes it more intuitive so no, it shouldn't wait for 39.

Comment 84

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella

a+ for all patches.

Comment 85

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

Attachment: (38 patch) Remove use of non-uplifted string

To be more official with this commit.

Comment 86

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

(In reply to Michael Comella (:mcomella) from comment #85)
> Created attachment 8582613 [details] [diff] [review]
> (38 patch) Remove use of non-uplifted string
> 
> To be more official with this commit.

Assuming aurora approval on the changeset from comment 85 because uplifting is blocking other work. I previously mentioned this change in comment 79 without comment.

Comment 87

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bf04f6732526
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/909e80e07d18
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4dcad21fe5d1
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/fcbc7d7bf56e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9eee81158ae2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f9ecb2ff1fec

^ This is the commit where I added the strings originally. I folded the patch in comment 85 into it to erase the addition of the strings so I didn't have to provide l10n override authorization.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0873eaffa95f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/69cbe91e4d9e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2d8245566470
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c71969cda503
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c4ef508b97ef
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/6c950c12a283
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/87465191828f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/538f346c00b6

Comment 88

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2015-03-26 17:19:44.png

Tested using:
Device: Samsung S5 (Android 4.4)
Builds: Firefox for Android 38.0a1 and 39.0a1 (2015-03-26)

Tapping the notification from the android notification bar opens a dialog: "Complete action using:" and you can open the received tab with Nightly, Aurora etc. Is this expected?

Comment 89

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

(In reply to Teodora Vermesan (:TeoVermesan) from comment #88)
> Tapping the notification from the android notification bar opens a dialog:
> "Complete action using:" and you can open the received tab with Nightly,
> Aurora etc. Is this expected?

AFAIK, I didn't change this behavior - assuming some send tab functionality exists on 37 (maybe still as "Add to Firefox"?) , can you repro on 37? If not, please file and CC me.

Comment 90

[Teodora Vermesan (:TeoVermesan)]

Devices: Alcatel One Touch (Android 4.1.2) and Nexus 5 (Android 5.0) both using Firefox 37. 

On Alcatel One Touch open a page and go to Menu -> Share -> Send to other devices -> and choose "Firefox on Nexus 5" from the list. Tapping the notification from android notification bar on Nexus 5 opens a dialog: "Open with Aurora or use a different app": http://i.imgur.com/KzvlbxQ.png

I thought that if a tab is sent for example from Nightly to Beta, the tab should be opened in Beta, and no dialog (with suggested apps to open the tab with) should not be displayed.

Comment 91

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

(In reply to Teodora Vermesan (:TeoVermesan) from comment #90)
> I thought that if a tab is sent for example from Nightly to Beta, the tab
> should be opened in Beta, and no dialog (with suggested apps to open the tab
> with) should not be displayed.

I don't think I changed this. Is this a regression, Teodora?

Comment 93

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

Attachment: MozReview Request: Bug 1122302 - Add SendTabDialog. r=rnewman

Comment 94

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

Attachment: MozReview Request: Bug 1122302 - Handle reusing the share overlay. r=rnewman

Comment 95

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

Attachment: MozReview Request: Bug 1122302 - Correct the size of the share plane in the quick share menu. r=rnewman

Comment 96

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

Attachment: MozReview Request: Bug 1122302 - Make the device list in the share overlay always show up only for the current channel. r=rnewman

Comment 97

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

Attachment: MozReview Request: Bug 1122302 - Pin internal actions to the top of the share menu. r=rnewman

Comment 98

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

Attachment: MozReview Request: Bug 1122302 - Small perf improvement in ActivityChooserModel. r=rnewman

Comment 99

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

Attachment: MozReview Request: Bug 1139230 - Enable scrolling on the share overlay. r=rnewman

Comment 100

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

Attachment: MozReview Request: Bug 1122302 - Add share plane assets. r=rnewman

Comment 101

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

Attachment: MozReview Request: Bug 1122302 - Change "Add to Firefox" -> "Send to other devices" when sharing inside Firefox. r=rnewman

Comment 102

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

Attachment: MozReview Request: Bug 1122302 - Clean up ShareDialog & AbstractSendTabDialog. r=rnewman

Comment 103

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

Attachment: MozReview Request: Bug 1122302 - Get TelemetryMethod from sub-class in AbstractSendTabDialog. r=rnewman

Comment 104

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

Attachment: MozReview Request: Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman

Comment 105

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

Attachment: MozReview Request: Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman

Comment 106

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

Attachment: MozReview Request: Bug 1122302 - Display SendTabDialog only in sub menu. r=rnewman

Comment 107

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

Attachment: MozReview Request: Bug 1122302 - Only show the device list when opening "Send to other devices" in Firefox. r=rnewman

Comment 108

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

Attachment: MozReview Request: Bug 1122302 - Don't add "Send to other devices" if there are no other devices to send to. r=rnewman

Comment 109

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

Attachment: MozReview Request: Bug 1122302 - Rename ShareDialog -> ExternalShareDialog. r=rnewman

Comment 110

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

Attachment: MozReview Request: Bug 1122302 - Move the ExternalShareDialog specific layout elements to a ViewStub. r=rnewman

Comment 111

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

Attachment: MozReview Request: Bug 1122302 - Add SendTabDialog. r=rnewman

Comment 112

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

Attachment: MozReview Request: Bug 1122302 - Update "Send to other devices" to final String. r=rnewman

Comment 113

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

Attachment: MozReview Request: Bug 1122302 - Reload activities in ActivityChooserModel on sync. r=rnewman

Comment 114

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

Attachment: MozReview Request: Bug 1122302 - Add escape hatch in case clients list is empty in share overlay. r=rnewman