Closed
Bug 1140048
Opened 9 years ago
Closed 3 years ago
Add "Send to device" Shareplane to top level menu
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Tracking
(firefox44 unaffected, firefox45 affected)
RESOLVED
INCOMPLETE
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | unaffected |
firefox45 | --- | affected |
People
(Reporter: antlam, Unassigned, Mentored)
References
Details
Attachments
(10 files, 7 obsolete files)
136.61 KB,
image/png
|
Details | |
106.86 KB,
image/png
|
Details | |
170.93 KB,
image/png
|
Details | |
43.57 KB,
image/jpeg
|
Details | |
6.88 KB,
patch
|
dnirm
:
review+
|
Details | Diff | Splinter Review |
8.08 KB,
patch
|
dnirm
:
review+
|
Details | Diff | Splinter Review |
3.97 KB,
patch
|
dnirm
:
review+
|
Details | Diff | Splinter Review |
10.65 KB,
patch
|
dnirm
:
review+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
dnirm
:
review+
|
Details | Diff | Splinter Review |
30.68 KB,
image/png
|
antlam
:
feedback+
|
Details |
Currently, our "Send to" action is under "share". I propose that we move it further up one level and place it next to the "Add to Reading List" icon. Goals: - help us save users an extra press - give this feature more exposure - further establish a cross platform use of the "Shareplane" - help with use-cases involving task continuity
Updated•9 years ago
|
Priority: -- → P1
Reporter | ||
Comment 1•9 years ago
|
||
Attaching empty state (and half full/empty state) mocks. Went through many options but this one gives us the benefit of a bit more context around what that button is. Still undecided if the share icon should end up on the right, or left (after the user shares an item). But otherwise, this is the design for the empty state.
via irc: 11:32 <mcomella> antlam: This is still something we want to do, right? [link to this bug] 11:34 <antlam> mcomella: I do, but not until we finish the confirmation & notification bugs 11:34 <antlam> The feedback ux needs to be complete
Anthony wants this prioritized.
Reporter | ||
Comment 4•9 years ago
|
||
Talked to mcomella, we should be able to get this to track Firefox 44. Along with the polish feedback bug 1147653 NI-ing Barbara to update her on teh status.
Flags: needinfo?(bbermes)
Comment 5•9 years ago
|
||
Updated the corresponding Aha card! thanks everyone.
Flags: needinfo?(bbermes)
Assignee: nobody → michael.l.comella
tracking-fennec: --- → 44+
Dipti is going to take care of this (via bug 1166385 comment 65). I mentioned that this bug is tracking 44 and thus on a deadline.
Assignee: michael.l.comella → dnirm.moz
Hey, Dipti! In order to test sync, you'll need a Firefox account. Luckily for you, you can easily make a test Firefox account. To do so: * When you go to the "Create account" screen, type in an email address of the form "<user>@mockmyid.com". mockmyid.com is a special ending for test accounts. Continue to fill out the remaining fields (I'd recommend using a different password from your personal accounts) * Go to mailinator.com and type in <user> and click "Check it" - you'll get into a temporary mail account mailbox, the one your test account is registered to. For example, if I use "michael@mockmyid.com" on create account, I'd type in "michael" at mailinator. * Note: you'll only need to create an account for a mockmyid.com address once. After that, log in. (i.e. it's as you'd expect) * Verify the account via the email sent to that account and you should be good to go! * Sync another device to this account (i.e. log into it). * You'll want to make sure both devices sync to the account (it leans on Android which may not do this immediately). Go to 3-dot menu, Settings -> Sync -> Sync Now on both devices. Once they're synced, you should be able to send a tab. * To send a tab, click the 3-dot menu, click the Android share icon, and find "Send to other devices". iirc, it should always be at in the top few items of the menu. Let me know if you have any questions!
Comment 8•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #7) > Hey, Dipti! In order to test sync, you'll need a Firefox account. Luckily > for you, you can easily make a test Firefox account. > > Let me know if you have any questions! Thanks Michael for this info. Is there any specific reason for using test account? Recently I started using my private account for firefox sync across devices. I was wondering if I can use that here. Do I need to share the account details used? I'm exploring the code for this feature and found [1] and ShareDialog as the starting point. [1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/service/sharemethods/SendTab.java#52 Should we have OverlayDialogButton for "send to device" like "add to reading list" and "bookmark" features?
Flags: needinfo?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #8) > Thanks Michael for this info. Is there any specific reason for using test > account? It separates personal data from test data, which some people like to do in case of corruption (which is unlikely in this case, but you can follow the better-safe-than-sorry approach). For example, there was a Firefox for Android bug that I don't think was ever fixed where devices can be added to the list multiple times (e.g. add device to sync, remove device from sync, re-add device and it appears twice). When you're testing sync, it's possible you'll do these STR and litter your personal account with unused devices. > Recently I started using my private account for firefox sync across > devices. I was wondering if I can use that here. Sure, if you like! Just mind the potential issues above. > Do I need to share the account details used? No. > Should we have OverlayDialogButton for "send to device" like "add to reading > list" and "bookmark" features? No – sorry if I was unclear. We want to move the plane icon, which is "Send to other devices" button, from the Android share menu to the top-level menu – see the mock in comment 1. > I'm exploring the code for this feature and found [1] and ShareDialog as the > starting point. We want to start from some of the code that opens ShareDialog. I'll try to summarize what I think needs to happen: 1) Add the share button to the quick share action bar [1] (3rd row). When you start digging, you'll notice the *same* share View is used twice – once to display the quick share bar and once to display the actual share button. They used to be in the same row, which is why they're the same view. When I split up the view, it was easier to re-use the View and hide the extraneous parts of it – essentially, you're to undo that work and put the share button in the same row as quick share. I believe I implemented this originally in [2]. 2) Replace the 2nd row share button in the menu with the send to other devices button. You can have it do nothing for now. The menu is defined in resources/menu*. 3) Move the relevant "Send to other devices" code from ActivityChooserModel, GeckoActionProvider, etc. into the "Send to other devices button". Remove the rest. Note that the menu is initialized in BrowserApp (look at the "Add to reading list" button as an example, perhaps). This is a pretty tricky set of changes! If there's something you don't understand, please let me know! This is a lot so focus on each part in isolation. If this seems like too much, I can help with some parts or provide further guidance. NI to make sure you understand! [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenu.java?rev=28d346b0142c#154 [2]: http://hg.mozilla.org/mozilla-central/rev/aadbc0a67a2c
Flags: needinfo?(michael.l.comella) → needinfo?(dnirm.moz)
Comment 10•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #9) > We want to start from some of the code that opens ShareDialog. I'll try to > summarize what I think needs to happen: > 1) Add the share button to the quick share action bar [1] (3rd row). When > you start digging, you'll notice the *same* share View is used twice – once > to display the quick share bar and once to display the actual share button. > They used to be in the same row, which is why they're the same view. When I > split up the view, it was easier to re-use the View and hide the extraneous > parts of it – essentially, you're to undo that work and put the share button > in the same row as quick share. I believe I implemented this originally in > [2]. > 2) Replace the 2nd row share button in the menu with the send to other > devices button. You can have it do nothing for now. The menu is defined in > resources/menu*. > 3) Move the relevant "Send to other devices" code from > ActivityChooserModel, GeckoActionProvider, etc. into the "Send to other > devices button". Remove the rest. Note that the menu is initialized in > BrowserApp (look at the "Add to reading list" button as an example, perhaps). > > This is a pretty tricky set of changes! If there's something you don't > understand, please let me know! This is a lot so focus on each part in > isolation. If this seems like too much, I can help with some parts or > provide further guidance. > > NI to make sure you understand! Thank you. This gives better understanding of what needs to be done. I'll let you know if I'm stuck somewhere. For now it looks good to start.
Flags: needinfo?(dnirm.moz)
Comment 11•9 years ago
|
||
Attachment #8665206 -
Flags: review?(michael.l.comella)
Hey, Dipti – I'll try to get to this tomorrow.
Reporter | ||
Comment 13•9 years ago
|
||
Wanted to quickly bring up the design changes for the default "share" item in the menu (in comment 1) after moving the Shareplane out. Could I get an estimate of how achievable that is? Should we split it off into a second bug?
Blocks: 1208577
(In reply to Anthony Lam (:antlam) from comment #13) > Could I get an estimate of how achievable that is? Should we split it off > into a second bug? T-shirt size small amount of work but I split it off into a separate bug for separation of concerns: bug 1208577. Dipti, if you're interested, you can follow up with that bug after this one gets fixed.
Comment 15•9 years ago
|
||
Share button is replaced by 'Send to device' and share is added on third row in the previously submitted patch. Image shows share menu with and without the quick share items. I couldn't understand how bug 1208577 is different. Also I am yet to work on 3rd point from Comment 9 (moving 'send to other devices' code). I found this part little tricky especially related to 'share - send to device'. Could you please point me to where I should look for?
Flags: needinfo?(michael.l.comella)
I'll get to your review Monday, Dipti. (In reply to Dipti Nirmale [:dnirm] from comment #15) > I couldn't understand how bug 1208577 is different. In Anthony's mocks, the Android system share icon is combined with the "Share" text with 0 quick share items, and on the left side of the menu with >= 1 item, whereas your screenshot does not include it with 0 items, and puts it on the right with >= 1 item (this is how I was expecting this bug to go – looks good! :). > Also I am yet to work on > 3rd point from Comment 9 (moving 'send to other devices' code). I found this > part little tricky especially related to 'share - send to device'. Could you > please point me to where I should look for? What do you mean by related to "share - send to device"? I'm having difficulty providing guidance without more specific information on where you're stuck – should I just be more specific with what I said in comment 9?
Flags: needinfo?(michael.l.comella) → needinfo?(dnirm.moz)
Comment 17•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #16) > (In reply to Dipti Nirmale [:dnirm] from comment #15) > > Also I am yet to work on > > 3rd point from Comment 9 (moving 'send to other devices' code). I found this > > part little tricky especially related to 'share - send to device'. Could you > > please point me to where I should look for? > > What do you mean by related to "share - send to device"? > > I'm having difficulty providing guidance without more specific information > on where you're stuck – should I just be more specific with what I said in > comment 9? Sorry. I should have been more specific. I spent some time looking for on click event handler for 'send to other device' through 'share'. Could you tell me which class handles this event?
Flags: needinfo?(dnirm.moz) → needinfo?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #17) > Sorry. I should have been more specific. I spent some time looking for on > click event handler for 'send to other device' through 'share'. Could you > tell me which class handles this event? If you know the text of a string that is on a button you want to look up, you can search for the text to find the string name [1] to find where it is used [2] and then go through the results. We actually just open the ShareDialog Activity [3] via a generic share menu callback listener. [1]: https://mxr.mozilla.org/mozilla-central/search?string=send+to+other+device&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central [2]: https://mxr.mozilla.org/mozilla-central/search?string=overlay_share_send_other&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central [3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java?rev=a9738e5f636a#174
Flags: needinfo?(michael.l.comella)
Comment on attachment 8665206 [details] [diff] [review] Replace share with 'send to device' in menu Review of attachment 8665206 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you're going in the right direction ^_^ – you need to add the hit target, as you mentioned, and I'd like to rip more unused code out too! If you're comfortable with version control, feel free to put these in separate commits. ::: mobile/android/base/BrowserApp.java @@ +3012,5 @@ > > GeckoActionProvider provider = GeckoActionProvider.getForType(GeckoActionProvider.DEFAULT_MIME_TYPE, this); > > + // For test. To be removed before commit > + provider.clearHistory(); If you have non-final code in your request, it should be a feedback request, not a review request. btw, this doesn't compile for me. ::: mobile/android/base/menu/GeckoMenu.java @@ +100,5 @@ > // Map of "ifRoom" action-items in action-bar and their views. > private final Map<GeckoMenuItem, View> mSecondaryActionItems; > > + // Map of "ifRoom|withText" action-items in action-bar and their views. > + private final Map<GeckoMenuItem, View> mSecondaryTextActionItems; nit: I'd prefer if this was called, `mShareActionItems`, or something. Same for the associated items below. @@ +232,5 @@ > + setAdapter(null); > + addHeaderView((DefaultActionItemBar) mSecondaryTextActionItemBar); > + setAdapter(mAdapter); > + } > + if (added = mSecondaryTextActionItemBar.addActionItem(actionView)) { We should only add the header view if added is true here. @@ +233,5 @@ > + addHeaderView((DefaultActionItemBar) mSecondaryTextActionItemBar); > + setAdapter(mAdapter); > + } > + if (added = mSecondaryTextActionItemBar.addActionItem(actionView)) { > + mSecondaryTextActionItems.put(menuItem, actionView); nit: indentation. ::: mobile/android/base/menu/GeckoMenuItem.java @@ +30,1 @@ > public static final int SHOW_AS_ACTION_COLLAPSE_ACTION_VIEW = 8; I think this is unused now. @@ -140,5 @@ > @Override > public View getActionView() { > if (mActionProvider != null) { > - if (getActionEnum() == MenuItem.SHOW_AS_ACTION_IF_ROOM) { > - return mActionProvider.onCreateActionView(SECONDARY_ACTION_BAR_HISTORY_SIZE, These onCreateActionView(int, ActionViewType) methods are unused after your patch. @@ -144,5 @@ > - return mActionProvider.onCreateActionView(SECONDARY_ACTION_BAR_HISTORY_SIZE, > - GeckoActionProvider.ActionViewType.DEFAULT); > - } else { > - return mActionProvider.onCreateActionView(QUICK_SHARE_ACTION_BAR_HISTORY_SIZE, > - GeckoActionProvider.ActionViewType.QUICK_SHARE_ICON); You can also remove the code using QUICK_SHARE_ICON (e.g. QuickShareBarActionView). ::: mobile/android/base/resources/menu-large-v11/browser_app_menu.xml @@ +35,5 @@ > android:showAsAction="ifRoom"/> > > + <item android:id="@+id/send_to_device" > + android:icon="@drawable/overlay_send_tab_icon" > + android:title="Send to device" Use @string resources here – you can search for the entity name. ::: mobile/android/base/resources/menu-xlarge-v11/browser_app_menu.xml @@ +42,4 @@ > <item android:id="@+id/share" > android:icon="@drawable/ic_menu_share" > android:title="@string/share" > + android:showAsAction="ifRoom|withText"/> I wasn't aware you could OR them. Nice, I like that because we'll actually be showing text. :P ::: mobile/android/base/resources/menu/browser_app_menu.xml @@ +30,5 @@ > android:icon="@drawable/ic_menu_new_private_tab" > android:title="@string/new_private_tab"/> > > + <item android:id="@+id/send_to_device" > + android:icon="@drawable/overlay_send_tab_icon" Are these icons necessary? Have you tested on GB? I don't think these items appear in the top 5 items and thus don't have an icon.
Attachment #8665206 -
Flags: review?(michael.l.comella) → review+
btw, you're going to make Anthony very happy. :)
Comment 21•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #18) > (In reply to Dipti Nirmale [:dnirm] from comment #17) > If you know the text of a string that is on a button you want to look up, > you can search for the text to find the string name [1] to find where it is > used [2] and then go through the results. Thanks. I looked for 'send to device' instead 'send to other device'. I also got confused with the resource ids especially for overlay* though am getting familiar with them now :) > > We actually just open the ShareDialog Activity [3] via a generic share menu > callback listener. > [3]: > https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ > GeckoActionProvider.java?rev=a9738e5f636a#174 This is close to what I was looking for. Initially I had thought of ShareDialog as starting point (Comment 8) but somehow missed [1] which actually sends tab to other devices. I'm curious on how gecko events are handled and would like to explore it later. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java#395
Comment 22•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #19) > Comment on attachment 8665206 [details] [diff] [review] > Replace share with 'send to device' in menu > > Review of attachment 8665206 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like you're going in the right direction ^_^ – you need to add the hit > target, as you mentioned, and I'd like to rip more unused code out too! > > If you're comfortable with version control, feel free to put these in > separate commits. Should I add every class in separate commits? or should the code and resources be separated in different commits? > ::: mobile/android/base/BrowserApp.java > @@ +3012,5 @@ > > > > GeckoActionProvider provider = GeckoActionProvider.getForType(GeckoActionProvider.DEFAULT_MIME_TYPE, this); > > > > + // For test. To be removed before commit > > + provider.clearHistory(); > > If you have non-final code in your request, it should be a feedback request, > not a review request. > > btw, this doesn't compile for me. Sorry for this. I'll ensure the type of request onwards. > > ::: mobile/android/base/resources/menu/browser_app_menu.xml > @@ +30,5 @@ > > android:icon="@drawable/ic_menu_new_private_tab" > > android:title="@string/new_private_tab"/> > > > > + <item android:id="@+id/send_to_device" > > + android:icon="@drawable/overlay_send_tab_icon" > > Are these icons necessary? Have you tested on GB? I don't think these items > appear in the top 5 items and thus don't have an icon. I'm not sure on this. What is the criteria for adding the icons in menu? Could you also tell me what GB is?
Flags: needinfo?(michael.l.comella)
Comment 23•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #20) > btw, you're going to make Anthony very happy. :) I hope it makes into final release then :)
Comment 24•9 years ago
|
||
(In reply to Dipti Nirmale [:dnirm] from comment #21) > (In reply to Michael Comella (:mcomella) from comment #18) > > (In reply to Dipti Nirmale [:dnirm] from comment #17) > > If you know the text of a string that is on a button you want to look up, > > you can search for the text to find the string name [1] to find where it is > > used [2] and then go through the results. > > Thanks. I looked for 'send to device' instead 'send to other device'. I also > got confused with the resource ids especially for overlay* though am getting > familiar with them now :) Could you recommend any tool which maps elements on android ui to its id or layout? I generally do it intuitively which may not be the right way. I found [1] on similar lines. [1]: http://stackoverflow.com/questions/17354371/is-there-an-equivalent-to-dom-inspector-for-android-native-layouts
Reporter | ||
Comment 25•9 years ago
|
||
Awesome! Thank you Dipti! Two nits: - Can we resize the plane in the top menu now? I want to try and shrink it a bit cause it looks kinda large. 24dp max height should do it. - When there are no items in the quick share yet, we have > there right now, can we change that to the share icon so there are cross-state consistencies?
Flags: needinfo?(dnirm.moz)
Comment 26•9 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #25) > Awesome! Thank you Dipti! > > Two nits: > - Can we resize the plane in the top menu now? I want to try and shrink it > a bit cause it looks kinda large. 24dp max height should do it. Can you please confirm if you mean second row (for send to other device)? I'm somewhat naive to UI design :P. Is the plane changed because of the icon used? I see there are two icons (a)icon_shareplane.png and (b)overlay_send_tab_icon.png among which I've used the (b) for send to device. Should I use the other one? I have one query regarding the behaviour of this added menu. What should happen after tapping 'Send to other device' in case no sync account or device exists? I think this menu should be disabled in such case. The current behaviour seems not to display this option on 'share'. > - When there are no items in the quick share yet, we have > there right > now, can we change that to the share icon so there are cross-state > consistencies? I am not sure how feasible this is (from [1]). We might get some work-around after some digging. I see follow-up bug 1208577 for the same from Comment 16. Should we do it after this bug is fixed? [1] : http://ux.stackexchange.com/questions/57423/why-does-android-not-show-an-icon-in-the-overflow-menu
Flags: needinfo?(dnirm.moz)
Updated•9 years ago
|
Flags: needinfo?(alam)
Reporter | ||
Comment 27•9 years ago
|
||
(In reply to Dipti Nirmale [:dnirm] from comment #26) > (In reply to Anthony Lam (:antlam) from comment #25) > > Awesome! Thank you Dipti! > > > > Two nits: > > - Can we resize the plane in the top menu now? I want to try and shrink it > > a bit cause it looks kinda large. 24dp max height should do it. > > Can you please confirm if you mean second row (for send to other device)? > I'm somewhat naive to UI design :P. Is the plane changed because of the icon > used? I see there are two icons (a)icon_shareplane.png and > (b)overlay_send_tab_icon.png among which I've used the (b) for send to > device. Should I use the other one? No worries! Yes, I'm referring to the plane icon in your comment 15 attachment. Nope, it's the same plane icon and we're not changing it so we can just resize the current asset. > I have one query regarding the behaviour of this added menu. What should > happen after tapping 'Send to other device' in case no sync account or > device exists? I think this menu should be disabled in such case. The > current behaviour seems not to display this option on 'share'. > > > - When there are no items in the quick share yet, we have > there right > > now, can we change that to the share icon so there are cross-state > > consistencies? > > I am not sure how feasible this is (from [1]). We might get some work-around > after some digging. I see follow-up bug 1208577 for the same from Comment > 16. Should we do it after this bug is fixed? > > [1] : > http://ux.stackexchange.com/questions/57423/why-does-android-not-show-an- > icon-in-the-overflow-menu Sure, works for me
Flags: needinfo?(alam) → needinfo?(dnirm.moz)
Comment 28•9 years ago
|
||
Attachment #8668066 -
Flags: review?(michael.l.comella)
Comment 29•9 years ago
|
||
Do not display Send to other device as sub menu in share
Attachment #8668067 -
Flags: review?(michael.l.comella)
Comment 30•9 years ago
|
||
Attachment #8668070 -
Flags: review?(michael.l.comella)
Comment 31•9 years ago
|
||
Attachment #8665206 -
Attachment is obsolete: true
Attachment #8668072 -
Flags: review?(michael.l.comella)
Updated•9 years ago
|
QA Contact: teodora.vermesan
Comment 32•9 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #27) > (In reply to Dipti Nirmale [:dnirm] from comment #26) > > (In reply to Anthony Lam (:antlam) from comment #25) > > > - Can we resize the plane in the top menu now? I want to try and shrink it > > > a bit cause it looks kinda large. 24dp max height should do it. > > > > Can you please confirm if you mean second row (for send to other device)? > > I'm somewhat naive to UI design :P. Is the plane changed because of the icon > > used? I see there are two icons (a)icon_shareplane.png and > > (b)overlay_send_tab_icon.png among which I've used the (b) for send to > > device. Should I use the other one? > > No worries! Yes, I'm referring to the plane icon in your comment 15 > attachment. > > Nope, it's the same plane icon and we're not changing it so we can just > resize the current asset. I tried setting the height of the plane to 24dp in [1], [2] & [3] but it did not change the view. Probably Michael could help here. If it is icon design specific requirement, can we have a new bug for this? [1] : https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/menu_item_action_view.xml#24 [2] : https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/menu_secondary_action_bar.xml#13 [3] : https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml#95
Flags: needinfo?(dnirm.moz) → needinfo?(alam)
(In reply to Dipti Nirmale [:dnirm] from comment #22) > Should I add every class in separate commits? or should the code and > resources be separated in different commits? Whatever is most intuitive. I usually start by adding my png resources in a separate commit, and will separate by building small features. Where it goes from there is dependent on how I write the code. > Sorry for this. I'll ensure the type of request onwards. No need to apologize – that's how we learn! :) > Could you also tell me what GB is? Gingerbread, which is the codename for Android 2.3. > > Are these icons necessary? Have you tested on GB? I don't think these items > > appear in the top 5 items and thus don't have an icon. > > I'm not sure on this. What is the criteria for adding the icons in menu? On GB, it's the top five items, I believe. For example, see this menu [1]. Notably, item 6 on our menu is more, which brings up a list without icons. If you don't have a GB device, I can test for you – just NI me. [1]: https://encrypted.google.com/search?q=gingerbread%20menu&hl=en&client=firefox-a&hs=jN1&rls=org.mozilla:en-US:unofficial&prmd=imvns&source=lnms&tbm=isch&ei=OiInT-qcI8nygge53t3wCQ&sa=X&oi=mode_link&ct=mode&cd=2&ved=0CBMQ_AUoAQ&biw=1369&bih=967#imgrc=U4PIpJHZPtw1IM%3A&oi=mode_link
Flags: needinfo?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #24) > Could you recommend any tool which maps elements on android ui to its id or > layout? I generally do it intuitively which may not be the right way. I > found [1] on similar lines. Hierarchy Viewer would be the only one I know of, but it's not as fast as the DOM inspector. I generally read the code.
(In reply to Dipti Nirmale [:dnirm] from comment #32) > I tried setting the height of the plane to 24dp in [1], [2] & [3] but it did > not change the view. Probably Michael could help here. If it is icon design > specific requirement, can we have a new bug for this? I'm not sure what you mean by "icon design specific requirement" but I'll take a look into this and see what I can find.
Comment on attachment 8668066 [details] [diff] [review] Add 'send to device' and remove quick share from menu resource Review of attachment 8668066 [details] [diff] [review]: ----------------------------------------------------------------- It's good to have your patches stand on their own (e.g. this will fail to compile because the code tries to access R.id.quickshare) so someone can build each part of your code or check out any revision and still compile, but it's okay to do it this way occasionally. Thanks for breaking this apart! r+ w/ nits. ::: mobile/android/base/resources/menu-v11/browser_app_menu.xml @@ +42,5 @@ > <item android:id="@+id/share" > android:icon="@drawable/ic_menu_share" > android:title="@string/share" > + android:showAsAction="ifRoom|withText"> > + </item> Use `/>` rather than a separate closing tag. ::: mobile/android/base/resources/menu/browser_app_menu.xml @@ +36,2 @@ > <item android:id="@+id/share" > + android:icon="@drawable/ic_menu_share" I don't think this icon is necessary either.
Attachment #8668066 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8668067 [details] [diff] [review] Remove QUICK_SHARE_ICON Review of attachment 8668067 [details] [diff] [review]: ----------------------------------------------------------------- This patch does a lot more than "Remove QUICK_SHARE_ICON" – you should try to make the patches bite-sized enough to properly represent whatever the commit asks for. I'm not fine leaving it as is now to prevent wasting time reorganizing working commits. r+ w/ nits. ::: mobile/android/base/widget/ActivityChooserModel.java @@ -781,5 @@ > - */ > - if (shareDialogClassName.equals(resolveInfo.activityInfo.name) && > - channelToRemoveLabel.equals(resolveInfo.loadLabel(packageManager))) { > - // Don't add the menu item if there are no devices to share to. > - if (!hasOtherSyncClients()) { This method is unused. ::: mobile/android/base/widget/GeckoActionProvider.java @@ -102,5 @@ > case DEFAULT: > view = new MenuItemActionView(mContext, null); > break; > > - case QUICK_SHARE_ICON: Don't forget to remove this from the enum. @@ -156,1 @@ > final String sendTabLabel = mContext.getResources().getString(R.string.overlay_share_send_other); This and shareDialogClassName are unused. Nice find with removing this!
Attachment #8668067 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8668070 [details] [diff] [review] Replace quick share view with share in menu Review of attachment 8668070 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits. ::: mobile/android/base/menu/GeckoMenu.java @@ +233,5 @@ > + setAdapter(null); > + addHeaderView((DefaultActionItemBar) mShareActionItemBar); > + setAdapter(mAdapter); > + } > + mShareActionItems.put(menuItem, actionView); nit: indentation
Attachment #8668070 -
Flags: review?(michael.l.comella) → review+
Mentor: michael.l.comella
(In reply to Dipti Nirmale [:dnirm] from comment #26) > I have one query regarding the behaviour of this added menu. What should > happen after tapping 'Send to other device' in case no sync account or > device exists? I think this menu should be disabled in such case. The > current behaviour seems not to display this option on 'share'. Anthony and I will get back to you regarding this – we've discussed this before but misplaced the notes we wrote.
Comment on attachment 8668067 [details] [diff] [review] Remove QUICK_SHARE_ICON Review of attachment 8668067 [details] [diff] [review]: ----------------------------------------------------------------- You also have to remove QuickShareBarActionView.java from m/a/b/moz.build.
moz.build lists all of the files we include in our builds via `./mach` (as opposed to Intellij which unfortunately uses a different build system at the moment).
Comment on attachment 8668067 [details] [diff] [review] Remove QUICK_SHARE_ICON Review of attachment 8668067 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/widget/ActivityChooserModel.java @@ -778,5 @@ > - * > - * Note: we check both the class name and the label to ensure we only change the > - * label of the current channel. > - */ > - if (shareDialogClassName.equals(resolveInfo.activityInfo.name) && Actually, instead of removing this code, you should just `continue` if there's a match – we want to remove the item from the share menu as well. Maybe there's a better way to do this via intent filters, however.
Attachment #8668067 -
Flags: review+ → review-
Comment on attachment 8668072 [details] [diff] [review] Add 'Send to other device' Shareplane to menu Review of attachment 8668072 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits. Before this lands, we should also get a summary on how we should handle no account, no other synced devices, & one other synced devices. ::: mobile/android/base/BrowserApp.java @@ +3395,5 @@ > + Intent sendToDeviceIntent = GeckoAppShell.getShareIntent(getContext(), url, > + "text/plain", tab.getDisplayTitle()); > + sendToDeviceIntent.setClass(getContext(), ShareDialog.class); > + startActivity(sendToDeviceIntent); > + Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.MENU, "sendtab"); We already send a telemetry event for selecting the menu item in this method so this probe seems unnecessary. Good thinking though!
Attachment #8668072 -
Flags: review?(michael.l.comella) → review+
The push in comment 44 has a locally fixed build because these patches don't compile through mach.
Comment 46•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #34) > (In reply to Dipti Nirmale [:dnirm] from comment #24) > > Could you recommend any tool which maps elements on android ui to its id or > > layout? I generally do it intuitively which may not be the right way. I > > found [1] on similar lines. > > Hierarchy Viewer would be the only one I know of, but it's not as fast as > the DOM inspector. I generally read the code. Thanks for the info Michael.
Comment 47•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #35) > (In reply to Dipti Nirmale [:dnirm] from comment #32) > > I tried setting the height of the plane to 24dp in [1], [2] & [3] but it did > > not change the view. Probably Michael could help here. If it is icon design > > specific requirement, can we have a new bug for this? > > I'm not sure what you mean by "icon design specific requirement" but I'll > take a look into this and see what I can find. "icon design specific requirement" - Do we need to resize the icon (for send to device) which means changing the icon specific details? I'm not sure if it can be done within time frame since I'm naive to this.
Comment 48•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #33) > (In reply to Dipti Nirmale [:dnirm] from comment #22) > > > Are these icons necessary? Have you tested on GB? I don't think these items > > > appear in the top 5 items and thus don't have an icon. > > > > I'm not sure on this. What is the criteria for adding the icons in menu? > > On GB, it's the top five items, I believe. For example, see this menu [1]. > Notably, item 6 on our menu is more, which brings up a list without icons. > > If you don't have a GB device, I can test for you – just NI me. I don't have GB device. I downloaded 2.3.3 sdk but my AVD manager is not showing GB option for emulator. I'll dig into more to make it work; meanwhile it would be great if you could test it on your device.
Flags: needinfo?(michael.l.comella)
Comment 49•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #43) > Comment on attachment 8668072 [details] [diff] [review] > Add 'Send to other device' Shareplane to menu > > Review of attachment 8668072 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ w/ nits. > > Before this lands, we should also get a summary on how we should handle no > account, no other synced devices, & one other synced devices. > > ::: mobile/android/base/BrowserApp.java > @@ +3395,5 @@ > > + Intent sendToDeviceIntent = GeckoAppShell.getShareIntent(getContext(), url, > > + "text/plain", tab.getDisplayTitle()); > > + sendToDeviceIntent.setClass(getContext(), ShareDialog.class); > > + startActivity(sendToDeviceIntent); > > + Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.MENU, "sendtab"); > > We already send a telemetry event for selecting the menu item in this method > so this probe seems unnecessary. Good thinking though! For rest of the 'r+ w/ nits' - should I flag "r?" or "r+" (carry forward r+) after uploading patches with corrections?
Comment 50•9 years ago
|
||
Attachment #8668067 -
Attachment is obsolete: true
Attachment #8670017 -
Flags: review?(michael.l.comella)
Comment 51•9 years ago
|
||
Attachment #8670018 -
Flags: review?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #47) > "icon design specific requirement" - Do we need to resize the icon (for send > to device) which means changing the icon specific details? We should be able to resize the icon in code via scaleType and ImageView bounds. What do you mean by icon specific details? > I'm not sure if it can be done within time frame since I'm naive to this. If we were to change the asset (not preferred), it's generally pretty quick. If Anthony has the assets (e.g. some were from the previous UX designer), he can quickly export from SVG and so the icons are high quality pngs. (In reply to Dipti Nirmale [:dnirm] from comment #48) > I don't have GB device. I downloaded 2.3.3 sdk but my AVD manager is not > showing GB option for emulator. We actually support running the emulator from the command line now! https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#Running_tests_on_the_Android_emulator > meanwhile it would be great if you could test it on your device. I'll have a device to test on tomorrow. (In reply to Dipti Nirmale [:dnirm] from comment #49) > For rest of the 'r+ w/ nits' - should I flag "r?" or "r+" (carry forward r+) > after uploading patches with corrections? You can carry forward the r+ on all patches that are "r+ w/ nits".
Flags: needinfo?(michael.l.comella)
Attachment #8670017 -
Flags: review?(michael.l.comella) → review+
Attachment #8670018 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8670017 [details] [diff] [review] Remove quick share bar and set the action view to Default Review of attachment 8670017 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/menu/GeckoMenuItem.java @@ -140,5 @@ > @Override > public View getActionView() { > if (mActionProvider != null) { > - if (getActionEnum() == MenuItem.SHOW_AS_ACTION_IF_ROOM) { > - return mActionProvider.onCreateActionView(SECONDARY_ACTION_BAR_HISTORY_SIZE, You can also remove SECONDARY_ACTION_BAR_HISTORY_SIZE & QUICK_SHARE_ACTION_BAR_HISTORY_SIZE.
Dipti, I applied your patches and this looks great! Thanks for your hard work! I think we still have to: * Test on GB * Figure out the expected behavior when the user is not logged in, when there are no other synced devices, and when there is 1 other synced device. Anthony, here's a build if you want to test it out: https://people.mozilla.com/~mcomella/apks/dnirm-1140048_01.apk I find that the "I'm sending this tab to another device" context is lost... What do you think? Note: I added a try push in comment 53. Note2: Since these are attached as individual changesets, it's good to label their order. For example, the first patch would be, "Part 1 – Add 'send to device' and remove quick share from menu resource"
(In reply to Michael Comella (:mcomella) from comment #55) > I think we still have to: > * Test on GB > * Figure out the expected behavior when the user is not logged in, when > there are no other synced devices, and when there is 1 other synced device. * Fix remaining nits * ? – Resize the share plane in the menu
Reporter | ||
Comment 57•9 years ago
|
||
Looks good! Yes, the size of the shareplane would be the last thing to clean up here. Also, I'm seeing this in the "Share" list on my N6. Anyone else?
Flags: needinfo?(alam)
Reporter | ||
Comment 58•9 years ago
|
||
Wanted to call out the various scenarios/flows here: Not signed in: - Proceed to FxA sign up flow User is signed in, but has no other connected devices: - Display toast with messaging - - "Send this tab to another connected device" - At a later point, possibly experiment with using our "in-product promo sheet" UX (bug to-be-filed for this V2 polish) User is signed in, has 1 other connected device: - Just send the tab, confirmation UI will follow (being handled in bug 1147653) User is signed in, has 2+ other connected device: - Show device list, then send the tab to selected device, followed by confirmation UI. For context, this button sends the current tab to a Firefox Sync connected device. But, we need to display a message for users that don't have another device to send to. Essentially, we want the users to understand that they need to sign in on _another_ device to fully benefit from this feature.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(dnirm.moz)
Comment 59•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #36) > r+ w/ nits. > > ::: mobile/android/base/resources/menu-v11/browser_app_menu.xml > @@ +42,5 @@ > > <item android:id="@+id/share" > > android:icon="@drawable/ic_menu_share" > > android:title="@string/share" > > + android:showAsAction="ifRoom|withText"> > > + </item> > > Use `/>` rather than a separate closing tag. > > ::: mobile/android/base/resources/menu/browser_app_menu.xml > @@ +36,2 @@ > > <item android:id="@+id/share" > > + android:icon="@drawable/ic_menu_share" > > I don't think this icon is necessary either. Carry forward r+
Attachment #8668066 -
Attachment is obsolete: true
Attachment #8670622 -
Flags: review+
Comment 60•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #54) > ::: mobile/android/base/menu/GeckoMenuItem.java > @@ -140,5 @@ > > @Override > > public View getActionView() { > > if (mActionProvider != null) { > > - if (getActionEnum() == MenuItem.SHOW_AS_ACTION_IF_ROOM) { > > - return mActionProvider.onCreateActionView(SECONDARY_ACTION_BAR_HISTORY_SIZE, > > You can also remove SECONDARY_ACTION_BAR_HISTORY_SIZE & > QUICK_SHARE_ACTION_BAR_HISTORY_SIZE. Carry forward r+
Attachment #8670017 -
Attachment is obsolete: true
Attachment #8670623 -
Flags: review+
Comment 61•9 years ago
|
||
Added label. Carry forward r+
Attachment #8670018 -
Attachment is obsolete: true
Attachment #8670624 -
Flags: review+
Comment 62•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #38) > r+ w/ nits. > > ::: mobile/android/base/menu/GeckoMenu.java > @@ +233,5 @@ > > + setAdapter(null); > > + addHeaderView((DefaultActionItemBar) mShareActionItemBar); > > + setAdapter(mAdapter); > > + } > > + mShareActionItems.put(menuItem, actionView); > > nit: indentation Carry forward r+
Attachment #8668070 -
Attachment is obsolete: true
Attachment #8670625 -
Flags: review+
Comment 63•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #43) > r+ w/ nits. > ::: mobile/android/base/BrowserApp.java > @@ +3395,5 @@ > > + Intent sendToDeviceIntent = GeckoAppShell.getShareIntent(getContext(), url, > > + "text/plain", tab.getDisplayTitle()); > > + sendToDeviceIntent.setClass(getContext(), ShareDialog.class); > > + startActivity(sendToDeviceIntent); > > + Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.MENU, "sendtab"); > > We already send a telemetry event for selecting the menu item in this method > so this probe seems unnecessary. Good thinking though! Carry forward r+
Attachment #8668072 -
Attachment is obsolete: true
Attachment #8670626 -
Flags: review+
Comment 64•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #55) > Dipti, I applied your patches and this looks great! Thanks for your hard > work! > > Note2: Since these are attached as individual changesets, it's good to label > their order. For example, the first patch would be, "Part 1 – Add 'send to > device' and remove quick share from menu resource" Fixed remaining nits with labeling. Thanks Michael for all the help. I learned through the process. (In reply to Michael Comella (:mcomella) from comment #56) > (In reply to Michael Comella (:mcomella) from comment #55) > > I think we still have to: > > * Test on GB > > * Figure out the expected behavior when the user is not logged in, when > > there are no other synced devices, and when there is 1 other synced device. > * ? – Resize the share plane in the menu (In reply to Anthony Lam (:antlam) from comment #58) > Wanted to call out the various scenarios/flows here: > > Not signed in: > - Proceed to FxA sign up flow > > User is signed in, but has no other connected devices: > - Display toast with messaging > - - "Send this tab to another connected device" > > - At a later point, possibly experiment with using our "in-product promo > sheet" UX (bug to-be-filed for this V2 polish) > > User is signed in, has 1 other connected device: > - Just send the tab, confirmation UI will follow (being handled in bug > 1147653) > > User is signed in, has 2+ other connected device: > - Show device list, then send the tab to selected device, followed by > confirmation UI. > > For context, this button sends the current tab to a Firefox Sync connected > device. But, we need to display a message for users that don't have another > device to send to. > > Essentially, we want the users to understand that they need to sign in on > _another_ device to fully benefit from this feature. All this looks challenging and I'd have loved to work on all the pieces. Unfortunately I'm going on a month long vacation as I told Michael last week in IRC conversation. I apologize that due to lack of bandwidth I will not be able to work on the follow-up tasks.
Flags: needinfo?(dnirm.moz)
(In reply to Dipti Nirmale [:dnirm] from comment #64) > Fixed remaining nits with labeling. Thanks Michael for all the help. I > learned through the process. Yay – I'm glad! :) > (In reply to Michael Comella (:mcomella) from comment #56) > All this looks challenging and I'd have loved to work on all the pieces. > Unfortunately I'm going on a month long vacation as I told Michael last week > in IRC conversation. I apologize that due to lack of bandwidth I will not be > able to work on the follow-up tasks. No worries – thanks for all the help you've done thus far! I'll take this across the finish line, but be sure to let us know when you're back and if you're hungry for more! Enjoy your time off! Anthony, in order to separate concerns, I'd like to land this as is (provided it works on GB), and take care of the rest in a follow-up. Work for you?
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
NI self to land & file follow-ups.
Flags: needinfo?(michael.l.comella)
Blocks: 1213486
I think the Share should be over "Send to other devices" – what do you think, Anthony?
Attachment #8672166 -
Flags: feedback?(alam)
Works on GB, landing...
https://hg.mozilla.org/integration/fx-team/rev/13a6c5fb2f44fada9cc3dfe832641e0dcb968de7 Bug 1140048 - Add 'send to device' and remove quick share item in toolbar menu. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/a9d3499bbec176606b4e7c1b8ba2ff91a4c81621 Bug 1140048 - Remove quick share bar and set the action view to Default. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/35336ca96de213a697277b5c33ba300c553ee1bb Bug 1140048 - Remove 'Send to other device' menu item from share menu. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/64e311403f05ef109a5cfbe59babe0b929f4a2b9 Bug 1140048 - Move 'share' button down, replacing quick share view in menu. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/8b8598ed040e6343da1be02b8bab835362ba1355 Bug 1140048 - Add "Send to other device" Shareplane to top level menu. r=mcomella
Blocks: 1213490
Landed – thanks for all your help Dipti! :) I filed bug 1213486 for the shareplane size and bug 1213490 for the UX when < 2 devices are synced.
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 72•9 years ago
|
||
Comment on attachment 8672166 [details]
GB screenshot
wfm
Attachment #8672166 -
Flags: feedback?(alam) → feedback+
Comment 73•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/13a6c5fb2f44 https://hg.mozilla.org/mozilla-central/rev/a9d3499bbec1 https://hg.mozilla.org/mozilla-central/rev/35336ca96de2 https://hg.mozilla.org/mozilla-central/rev/64e311403f05 https://hg.mozilla.org/mozilla-central/rev/8b8598ed040e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Blocks: 1216307
Blocks: 1216312
Blocks: 1210989
tldr; backout of 44 inbound. The shareplane does what we want it to do when there are devices to share to (and the user is logged in) but we couldn't deliver a compelling experience when the user is 1) not signed into a Firefox Account or 2) the Firefox Account they're signed into has no other synced devices. bug 1213490 and bug 1217174 originally covered the implementation. We decided to develop a helper UI that will walk the user through signing up for a Firefox Account and informing them they may need to sync other devices to send a tab to them. We don't have enough time to get this into 44 so we're going to aim for 45. btw, Dipti, if you're interested in working on the helper UI, let me know! :)
tracking-fennec: 44+ → 45+
We don't actually need to land bug 1183659 first.
No longer depends on: 1183659
Bug 1140210 prevented a clean backout of this bug so I'm going to back it out too.
Blocks: 1140210
NI self to complete the backout (tree is closed). Update this bug's metadata as well as: bug 1140210 bug 1213490 bug 1213486
Flags: needinfo?(michael.l.comella)
Comment 78•9 years ago
|
||
Verified as fixed in build 44.0a2 2015-11-02; Devices: - Motorola Razr (Android 4.4.4); - Samsung Galaxy R (Android 2.3.4); - Asus Transformer Pad (Android 4.2.1).
Comment hidden (typo) |
Backout of 44, as per comment 74: https://hg.mozilla.org/releases/mozilla-aurora/rev/5c90a56aed40
Blocks: 1220910
Comment 81•9 years ago
|
||
Verified as fixed using: Device: nexus 9 (android 6.0) Build: Firefox for Android 45.0a1 (2015-11-11) "Shareplane" icon is present at the top level menu: - if the user is not signed in and he taps the icon: the FxA sign up flow is displayed - if the user is signed in, but has no other connected devices: "Send this tab to another connected device" toast notification is displayed - if the user is signed in and has 1 other connected device: a dialog with the single device is shown - if the user is signed in, has 2+ other connected device: the device list is shown
To double check, Margaret, we're backing this out, and the related work that landed on top, right?
Flags: needinfo?(margaret.leibovic)
Comment 83•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #82) > To double check, Margaret, we're backing this out, and the related work that > landed on top, right? Yes, let's back this out, and revisit it in the future when we have a better story around the no-accounts flow. I think this could be more important in the near future, when there's a better send tab experience from desktop. I would be fine to leave this in Nightly, but since this isn't actually behind a Nightly flag, and keeping it on Nightly would require maintenance work at every merge, it seems easier to just back it out. That being said, another idea we brought up in the funnel meeting would be not hiding the current shareplane option in the share menu when the user doesn't have an account, and using that to launch the helper UI. While this is less discoverable than a top-level menu, it does increase discoverability of this feature, while avoiding an unused icon in the top-level menu. We could file a bug about this, but I don't want us to invest too much time in this right now.
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
(In reply to :Margaret Leibovic from comment #83) > That being said, another idea we brought up in the funnel meeting would be > not hiding the current shareplane option in the share menu when the user > doesn't have an account, and using that to launch the helper UI. While this > is less discoverable than a top-level menu, it does increase discoverability > of this feature, while avoiding an unused icon in the top-level menu. We > could file a bug about this, but I don't want us to invest too much time in > this right now. If we spent the time to develop the helper UI while the feature was still in the share menu, it'd be pretty trivial to reapply these patches to get the whole thing in the top-level menu. Dipti, I apologize for this getting backed out. As Margaret mentioned we don't have a good experience when the user doesn't have an account or has no other devices to share to in that account (e.g. we can hide the menu item but then it sucks for those users because the menu is strictly worse than it was before). That being said, if you'd like to invest in that experience, we should be able to land this again! :) I've got the patches ready for backout but I'm waiting on the closed tree.
Comment 85•9 years ago
|
||
Thank you Dipti and Mike for all your hard work on this feature. I do think this is something that would be great to revisit in the future, but we just need to invest more in the no-account situation, and I think it would be best to do that without the pressure of needing to ship this in a certain release. Mike, can we use the existing bugs to work on the helper UI from the regular share menu, or do we need to file a new bug? Can you garden the bug tree here to make sure we have open bugs that reflect the outstanding work? We usually reopen bugs that were backed out, but we could also file new bugs if it makes it easier to follow the work.
https://hg.mozilla.org/integration/fx-team/rev/1e789580b2065698615e61665dcfca42028f8d17 Bug 1140048 - Backout 5 changesets to backout top level shareplane. Note: I mis-labeled this in the patch as bug 1140448.
Blocks: 1217174
Sure, we can re-use the bugs. The following bugs should be relanded and will force this bug to be rebased on them (hence depends on): * bug 1210989 * bug 1140210 The following bugs will need to land on top of this when we reland (I don't think order matters): * bug 1213490 * bug 1213486 * bug 1216307 The helper UI can be implemented in bug 1217174.
Status: RESOLVED → REOPENED
tracking-fennec: 45+ → ---
Resolution: FIXED → ---
bug 1216312 should also be relanded once this lands.
Since this is reopened and Dipti is, afaik, not actively working on this, unassigning. Dipti, let me know if you want to work on this further! We should start by implementing the helper UI (bug 1217174).
Assignee: dnirm.moz → nobody
Anthony, do we still want to do this?
Flags: needinfo?(alam)
Reporter | ||
Comment 92•8 years ago
|
||
I'm gonna loop in Barbara here for her Product expertise. Barbara, let's revisit this given the landscape of all the other work we're doing around the task continuity stories.
Flags: needinfo?(bbermes)
Comment 93•8 years ago
|
||
I think we should still consider this, and it's currently parked as a P3 aha card and hence should not be closed yet, but not worked on (actively) yet either.
Flags: needinfo?(bbermes)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(alam)
Comment 94•8 years ago
|
||
May I suggest that rather than convolute the process of sharing between Firefox Account registered devices, we instead use the real estate to add a Sync Now button given the removal of the Synced Tabs page from Firefox Home? Also if some work is to be done to Send To Device, may I suggest adding making it so that we can directly send to devices with Direct Share.
QA Contact: teodora.vermesan
QA Contact: teodora.vermesan
Comment 96•3 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 3 years ago
Resolution: --- → INCOMPLETE
Assignee | ||
Updated•3 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
•