Closed
Bug 1122302
Opened 10 years ago
Closed 10 years ago
Replace "Add to Firefox" with "Send to other devices" menu item
Categories
(Firefox for Android Graveyard :: Overlays, defect)
Tracking
(firefox38 fixed, firefox39 fixed, fennec38+)
RESOLVED
FIXED
Firefox 39
People
(Reporter: antlam, Assigned: mcomella)
References
Details
Attachments
(32 files, 5 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rnewman
:
review+
|
Details |
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•10 years ago
|
||
Nice!
I think showing a different background shade (or a thin divider), plus using the device icons, should do the trick for disambiguating.
Component: General → Overlays
Hardware: x86 → All
Comment 2•10 years ago
|
||
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•10 years ago
|
||
(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.
Reporter | ||
Comment 4•10 years ago
|
||
(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.
Reporter | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
(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...
Reporter | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
Google Docs application sharing entry point
Comment 9•10 years ago
|
||
Google Docs application split: Sharing and Export and Other
Comment 10•10 years ago
|
||
Pocket Share entry point: "Send to Friend" is custom and only shown when inside Pocket. Not available from outside Pocket.
Comment 11•10 years ago
|
||
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?
Reporter | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → michael.l.comella
Comment 13•10 years ago
|
||
(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.
Reporter | ||
Comment 14•10 years ago
|
||
(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 hidden (typo) |
Assignee | ||
Comment 16•10 years ago
|
||
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?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(alam)
Comment 17•10 years ago
|
||
(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.
Flags: needinfo?(mark.finkle)
Comment 18•10 years ago
|
||
(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".
Assignee | ||
Comment 19•10 years ago
|
||
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)?
Flags: needinfo?(mark.finkle)
Comment 20•10 years ago
|
||
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
Flags: needinfo?(mark.finkle)
Assignee | ||
Comment 21•10 years ago
|
||
(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
Assignee | ||
Comment 22•10 years ago
|
||
/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
Attachment #8561751 -
Flags: review?(wjohnston)
Assignee | ||
Comment 23•10 years ago
|
||
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.
Reporter | ||
Comment 24•10 years ago
|
||
(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.
Flags: needinfo?(alam)
Comment 25•10 years ago
|
||
Product meeting suggests that this is tracking 38.
tracking-fennec: --- → 38+
Assignee | ||
Updated•10 years ago
|
Summary: Show synced devices earlier in share menu → Reduce "Menu -> Share" click count by creating "Send tab to device" menu item (in lieu of "Add to Firefox")
Assignee | ||
Comment 26•10 years ago
|
||
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)?
Attachment #8562491 -
Flags: feedback?(alam)
Reporter | ||
Comment 27•10 years ago
|
||
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 ;)
Attachment #8562491 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Comment 28•10 years ago
|
||
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
Attachment #8561751 -
Flags: review?(rnewman)
Assignee | ||
Comment 29•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 30•10 years ago
|
||
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
Assignee | ||
Comment 31•10 years ago
|
||
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.
Assignee | ||
Comment 32•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8561751 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 33•10 years ago
|
||
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.
Flags: needinfo?(alam)
Assignee | ||
Comment 34•10 years ago
|
||
(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!
Assignee | ||
Comment 35•10 years ago
|
||
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
Attachment #8561751 -
Flags: review+
Assignee | ||
Comment 36•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8561751 -
Flags: review?(rnewman)
Assignee | ||
Comment 37•10 years ago
|
||
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
Flags: needinfo?(alam)
Comment 38•10 years ago
|
||
(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•10 years ago
|
||
(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.
Reporter | ||
Comment 40•10 years ago
|
||
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).
Attachment #8549946 -
Attachment is obsolete: true
Attachment #8551508 -
Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Reporter | ||
Updated•10 years ago
|
Attachment #8554075 -
Attachment is obsolete: true
Reporter | ||
Comment 41•10 years ago
|
||
Idea, WIP, but gives an idea of how the Shareplane might sit on this menu level.
Reporter | ||
Comment 42•10 years ago
|
||
Planes for Share menu, devices for overlay
Assignee | ||
Updated•10 years ago
|
Attachment #8572126 -
Attachment description: Screen Shot 2015-03-03 at 11.19.47 AM.png → Potential v2
Assignee | ||
Updated•10 years ago
|
Attachment #8572119 -
Attachment description: prev_sharetofx_mock3.png → v1 mock
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(michael.l.comella)
Summary: Reduce "Menu -> Share" click count by creating "Send tab to device" menu item (in lieu of "Add to Firefox") → Replace "Add to Firefox" with "Send to other devices" menu item
Assignee | ||
Updated•10 years ago
|
Attachment #8562491 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8561751 -
Flags: review?(rnewman)
Assignee | ||
Comment 43•10 years ago
|
||
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
Reporter | ||
Comment 44•10 years ago
|
||
Assignee | ||
Comment 45•10 years ago
|
||
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
Assignee | ||
Comment 46•10 years ago
|
||
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
Assignee | ||
Comment 47•10 years ago
|
||
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).
Assignee | ||
Comment 48•10 years ago
|
||
(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.
Assignee | ||
Comment 49•10 years ago
|
||
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.
Assignee | ||
Comment 50•10 years ago
|
||
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
Assignee | ||
Comment 51•10 years ago
|
||
Still TODO:
* Final share plane assets
* Final escape hatch String
Some blocked bugs might be useful too (e.g. bug 1139230).
Assignee | ||
Comment 52•10 years ago
|
||
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•10 years ago
|
||
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.)
Status: NEW → ASSIGNED
Comment 54•10 years ago
|
||
Also:
The "tap the fox" functionality still works... and it opens the same page in a new tab. Let's not do that.
Assignee | ||
Comment 56•10 years ago
|
||
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•10 years ago
|
||
(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.
Reporter | ||
Comment 58•10 years ago
|
||
(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.
Flags: needinfo?(alam)
Comment 59•10 years ago
|
||
(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•10 years ago
|
||
Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella
Clearing flag pending comments 53 and onward.
Attachment #8561751 -
Flags: review?(rnewman) → feedback+
Assignee | ||
Comment 61•10 years ago
|
||
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
Assignee | ||
Comment 62•10 years ago
|
||
(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.
Assignee | ||
Comment 63•10 years ago
|
||
(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.
Assignee | ||
Comment 64•10 years ago
|
||
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
Assignee | ||
Comment 65•10 years ago
|
||
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
Attachment #8561751 -
Flags: feedback+ → review?(rnewman)
Assignee | ||
Comment 66•10 years ago
|
||
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.
Assignee | ||
Comment 67•10 years ago
|
||
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•10 years ago
|
||
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!
Attachment #8561751 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 71•10 years ago
|
||
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.
Assignee | ||
Comment 72•10 years ago
|
||
Assignee | ||
Comment 73•10 years ago
|
||
Assignee | ||
Comment 74•10 years ago
|
||
NI self to let bake and uplift.
Flags: needinfo?(michael.l.comella)
Comment 75•10 years ago
|
||
(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.
Assignee | ||
Comment 76•10 years ago
|
||
(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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Assignee | ||
Comment 78•10 years ago
|
||
Note: bug 1127445 has to land first for this to land OR this lands, bug 1127445 lands, and then comment 73 lands.
Depends on: 1127445
Assignee | ||
Comment 79•10 years ago
|
||
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.
Flags: needinfo?(michael.l.comella)
Attachment #8561751 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 80•10 years ago
|
||
I'll handle the uplift in comment 79.
Assignee | ||
Comment 81•10 years ago
|
||
bug 1145892 and bug 1145897 should also be uplifted with this bug.
Comment 82•10 years ago
|
||
Michael, this is an important change. Can this wait for 39 ?
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 83•10 years ago
|
||
(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.
Flags: needinfo?(michael.l.comella)
Updated•10 years ago
|
status-firefox38:
--- → affected
Comment 84•10 years ago
|
||
Comment on attachment 8561751 [details]
MozReview Request: bz://1122302/mcomella
a+ for all patches.
Attachment #8561751 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 85•10 years ago
|
||
To be more official with this commit.
Assignee | ||
Updated•10 years ago
|
Attachment #8582613 -
Attachment is patch: true
Updated•10 years ago
|
Attachment #8582613 -
Flags: review+
Assignee | ||
Comment 86•10 years ago
|
||
(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.
Assignee | ||
Comment 87•10 years ago
|
||
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•10 years ago
|
||
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?
Assignee | ||
Comment 89•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(teodora.vermesan)
Comment 90•10 years ago
|
||
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.
Assignee | ||
Comment 91•10 years ago
|
||
(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?
Flags: needinfo?(teodora.vermesan)
Assignee | ||
Comment 92•9 years ago
|
||
Attachment #8561751 -
Attachment is obsolete: true
Attachment #8619142 -
Flags: review+
Attachment #8619143 -
Flags: review+
Attachment #8619144 -
Flags: review+
Attachment #8619145 -
Flags: review+
Attachment #8619146 -
Flags: review+
Attachment #8619147 -
Flags: review+
Attachment #8619148 -
Flags: review+
Attachment #8619149 -
Flags: review+
Attachment #8619150 -
Flags: review+
Attachment #8619151 -
Flags: review+
Attachment #8619152 -
Flags: review+
Attachment #8619153 -
Flags: review+
Attachment #8619154 -
Flags: review+
Attachment #8619155 -
Flags: review+
Attachment #8619156 -
Flags: review+
Attachment #8619157 -
Flags: review+
Attachment #8619158 -
Flags: review+
Attachment #8619159 -
Flags: review+
Attachment #8619160 -
Flags: review+
Attachment #8619161 -
Flags: review+
Attachment #8619162 -
Flags: review+
Attachment #8619163 -
Flags: review+
Assignee | ||
Comment 93•9 years ago
|
||
Assignee | ||
Comment 94•9 years ago
|
||
Assignee | ||
Comment 95•9 years ago
|
||
Assignee | ||
Comment 96•9 years ago
|
||
Assignee | ||
Comment 97•9 years ago
|
||
Assignee | ||
Comment 98•9 years ago
|
||
Assignee | ||
Comment 99•9 years ago
|
||
Assignee | ||
Comment 100•9 years ago
|
||
Assignee | ||
Comment 101•9 years ago
|
||
Assignee | ||
Comment 102•9 years ago
|
||
Assignee | ||
Comment 103•9 years ago
|
||
Assignee | ||
Comment 104•9 years ago
|
||
Assignee | ||
Comment 105•9 years ago
|
||
Assignee | ||
Comment 106•9 years ago
|
||
Assignee | ||
Comment 107•9 years ago
|
||
Assignee | ||
Comment 108•9 years ago
|
||
Assignee | ||
Comment 109•9 years ago
|
||
Assignee | ||
Comment 110•9 years ago
|
||
Assignee | ||
Comment 111•9 years ago
|
||
Assignee | ||
Comment 112•9 years ago
|
||
Assignee | ||
Comment 113•9 years ago
|
||
Assignee | ||
Comment 114•9 years ago
|
||
Updated•9 years ago
|
Flags: needinfo?(teodora.vermesan)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•