1155860 - Unclear how to directly open a shared link in Add to Firefox

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: Screenshot_2015-04-17-12-56-17.png

I quite often run into problems where an app that uses an embedded web view fails to load the site. When this happens what I want to do is then open the site in Firefox - and until today I did not realize this was possible.

See attached screenshot - the overlay has 3 clear user actions:

* send to other devices
* add to reading list
* bookmark

Missing from this list is 'open now'. It turns out that I can open the page immediately by touching in the transparent area that displays the title of the page, but I don't think personally that this is very discoverable. I think it would be much more consistent to have an additional action icon+label that says something like 'Open right now, dammit'

Comment 1

[Richard Newman [:rnewman]]

There used to be a big honkin' Firefox logo at the top that was clickable.

I don't know why Anthony got rid of that, or what the motivation was for moving to a faint black overlay rather than the opaque 'drawer' we used to have.

Comment 2

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

(In reply to Richard Newman [:rnewman] from comment #1)
> There used to be a big honkin' Firefox logo at the top that was clickable.

Yeah, the original redesign had that along the top (Action bar area) but ultimately we punted on that for this first iteration. 

I think neither approach is rather clear. Perhaps the best option here would be a 4th item in that list to just "Open in Firefox". 

> I don't know why Anthony got rid of that, or what the motivation was for
> moving to a faint black overlay rather than the opaque 'drawer' we used to
> have.

Got rid of it because it wasn't obvious as a "button" and I wanted to replace it with a new "action bar" along the top that would animate in. I also questioned how valuable this might be for users "in" another app.

As I pointed out in the other bug (bug 1059554), the black overlay is designed to give more visibility to the actual content. Originally, we were covering up a lot and it made the actions of "task continuity" (recall, etc) harder to perform since most of your screen was cluttered with the menu item.

That being said, we should probably add a "Open in Firefox" 4th item in this list now just to call that out if users really do need it that much in this stage.

I th

Comment 3

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

(In reply to Anthony Lam (:antlam) from comment #2)
> Got rid of it because it wasn't obvious as a "button" and I wanted to
> replace it with a new "action bar" along the top that would animate in.

I contested the action bar because its intent seemed to be to cover the existing app's action bar which I disliked.

However, if the bar was a more obvious, "Open in Firefox right now!" option, I could like it. A Firefox logo is not intuitive, imo.

Comment 4

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

via IRL: NI antlam for updated designs.

Comment 5

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

Attachment: prev_shareoverlay_mock5.png

We talked about using the Action Bar area for this purpose before, I think that would make a good starting point.

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Anthony Lam (:antlam) from comment #2)
..
> I think neither approach is rather clear. Perhaps the best option here would
> be a 4th item in that list to just "Open in Firefox". 

This, +1000. My thinking is, if there are multiple actions a user can take at this point, they should all be presented in the same way to prevent confusion.

Comment 7

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

Not actively working on this.

Comment 8

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

The place to start looking in this code is the "ShareDialog" class.

Comment 9

[Prateek Arora]

Hi, I would like to take up this bug. I have updated the xml files for extra button and modified JAVA code accordingly. Basically calling launchBrowser onclick of new button instead of onclick of title and subtitle.

I will share a patch soon

Comment 10

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

Sounds good! Thanks Prateek!

Comment 11

[Prateek Arora]

Attachment: Screenshot_2015-09-12-00-42-48.png

Comment 12

[Prateek Arora]

Hi Michael,

I created a patch. Here is how it looks. I had couple of remarks about the Patch I am going to submit in next comment :

[1] For the icon I copied "icon_search_empty_firefox" and renamed it to overlay_openbrowser_icon with dimensions as (45X45 HDPI, 60X60 XHDPI, 90X90 XXHDPI). Is this the correct way, I am not that good with icons and studd.

[2] I notices 2 redundant initializations in ShareDialog (bookmarkButton and readingList button being initialized their xml resource twice.) I have removed them. Let me know if I misunderstood something here.

Personally, I feel the icon could be a little darker so that it is in the league with icons below it.

Comment 13

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

re the attachment in comment 11 – I apologize if I was unclear! We wanted to match the mock in comment 5, which has the open in Firefox button at the top of the screen. Let's check with Anthony to see if he still wants to do that:

Anthony, do you still want to match the mock?

(In reply to Prateek Arora from comment #12)
> [1] For the icon I copied "icon_search_empty_firefox" and renamed it to
> overlay_openbrowser_icon with dimensions as (45X45 HDPI, 60X60 XHDPI, 90X90
> XXHDPI). Is this the correct way

We're trying to reduce our APK size so it'd be good to reuse the asset, rather than copy it to a new file. You can just use the asset directly, and use android attributes to scale down the asset to the appropriate size. Scaling up causes noticeable artifacts so if you need to scale up, you should ask for a larger asset.

> [2] I notices 2 redundant initializations in ShareDialog (bookmarkButton and
> readingList button being initialized their xml resource twice.) I have
> removed them. Let me know if I misunderstood something here.

I'll take a look and see what you mean. In general, for ease of review, it's good to separate different changes into different patches (or bugs!), like this one. I'll take a look in this patch and let you know if I think it should be a different patch/bug/etc.

> Personally, I feel the icon could be a little darker so that it is in the
> league with icons below it.

In the general case, you can use android:tint to do this, but since we may have to change the design to match the mock, it may not apply here.

Comment 14

[Prateek Arora]

Attachment: bug1155860.patch

Oh ! I misunderstood then. I thought general opinion was in favour of adding 4th option.

I will include the following in the next iteration of patch then :
[1] Actionbar (depending on Anthony's opinion)
[2] Remove duplicated png icons I made for overlay list


This patch is for adding 4th option. You can see here what I meant by redundant code in ShareDialog.java

Thanks

Comment 15

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

NI self to check out redundant code.

Comment 16

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

Attachment: share_trigger_mov1.mov

(In reply to Michael Comella (:mcomella) from comment #13)
> re the attachment in comment 11 – I apologize if I was unclear! We wanted to
> match the mock in comment 5, which has the open in Firefox button at the top
> of the screen. Let's check with Anthony to see if he still wants to do that:
> 
> Anthony, do you still want to match the mock?

Thanks for checking in! Yes, I'd like to give this a try. Let's get this in a build and see what feels better.

When we worked on bug 1059554, I did want to keep that "open in firefox" along the top. To me, this is almost like a counter proposition to most "in-app" browsers that have the same action bar area reserved for page information and a "back" button to the original app. But also, Send to other devices, Add to Reading List, and Bookmark are arguably subsets of the "Opening in Firefox" action. So, I think it makes sense to pull it out. 

I'm attaching a .MOV of the transition I'm hoping for. I'd think we this slide in will be important. In terms of specifics, I'm looking for the same speed and everything as we do for the bottom "slide up" interaction.

Comment 17

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

^ in terms of visuals/UI though, let's follow comment 5 and not the old UI in the video mock.

Build icon: 30dp 
Build icon centered, left padding: 15dp
Action bar background: #363B40
"Open in Firefox": white, Roboto regular 14sp
> icon size: 10 dp wide (proportional) 
> icon tint: #AFB1B3
> icon padding: left - 10dp, right - 15dp

Comment 18

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

Prateek, if you're not interested in prototyping (e.g. to avoid doing non-final work), let me know and we'll see if we can work around this.

If you are interested, great! :) In order to reduce the burden the build in comment 16 will take, I'd recommend cutting out the action bar image from Anthony's mock, throwing the image directly into an ImageButton, putting a click handler (which you should already have from your patch) on it to open in Firefox, and try that out. If you already know how to do animations well, you can throw in the animation downwards too, but we'll see how much that's needed after the first build is up.

And remember the code doesn't need to be clean here (though it will in the final version).

Let me know if you have questions about this! I don't want you to spend too much time doing work that might end up getting thrown out.

Comment 19

[Prateek Arora]

Sure Michael, I would like to give it a try. Its fine with me if we need to try couple of things here. Gets me familiar with codebase, and thats always good. So I am in.

Will get started on what you suggested. Thanks !

Comment 20

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

(In reply to Prateek Arora from comment #12)
> [2] I notices 2 redundant initializations in ShareDialog (bookmarkButton and
> readingList button being initialized their xml resource twice.) I have
> removed them. Let me know if I misunderstood something here.

lol, yep, looks redundant - you did the right thing! :)

Comment 21

[Prateek Arora]

Attachment: Approach_Action_Bar.mp4

I don't know if this much size of attachment is allowed or not !

Comment 22

[Prateek Arora]

Attachment: bug1155860_ABar_approach.patch

Hi Michael,

The video above describes the patch. Some keypoints :

[1] I tried to do actual ActionBar, but simple getActionBar wont work. Also, for actionbar I would have to take care of different versions of android etc.

So I went with your method, slightly modified, instead of whole image button ( Again I am not really good with images, didnt want to scale up down that image for various dpis), I made a linear layout, with build icon as image view and textview on the right with caret

[2] The build icon on the left, I guess Anthony wanted 15dp left padding ? does it seem too much ? To me it did. Is this how it will appear if we used actual actionbar ?

[3] Anthony also mentioned center for build icon ? I am assuming he meant center vertical not center of screen right ? That would imply following UI from mock vid (with FF logo in center)

[4] I did translation animation upto -200 to go up. This was done to have smooth effect. Otherwise, since we have short area to animate on the top as compared to overlay share buttons area below, it was producing some unwanted lag.

Comment 23

[Prateek Arora]

Attachment: open_in_external.png

Just thinking out loud, but, going to top of the screen to open in firefox, for big screens.. I dont know ... may be inconvenient.

Here's a thought, we are already displaying title and link.. why not have a cool "open in external" icon near link... That is intuitive, easily understood. I am talking about standard icon, every one knows stands for "opening in external"  [attachment is just off the top google image search, real icon will be mozified or in material style .. we can discuss that ]

Also since user already clicked "add in firefox" previously, its obvious that open in external button will open the link in firefox

Comment 24

[Prateek Arora]

Attachment: bug1155860_ABar_Approach.patch

A file was missing in previous patch. Added it now. I know there are many improvements for code here like not using hardcoded strings etc. But we can get to that once we confirm an approach.
Thanks.

Comment 25

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

Anthony, thoughts on comment 23? Also see the question below.

(In reply to Prateek Arora from comment #22)
> [1] I tried to do actual ActionBar, but simple getActionBar wont work. Also,
> for actionbar I would have to take care of different versions of android etc.

I think ActionBar makes assumptions about how it will be used and how it looks so I'd agree with avoiding it.

> So I went with your method, slightly modified, ...

Sure, whatever is fastest. :)

> [2] The build icon on the left, I guess Anthony wanted 15dp left padding ?
> does it seem too much ? To me it did. Is this how it will appear if we used
> actual actionbar ?

Anthony?

> [3] Anthony also mentioned center for build icon ? I am assuming he meant
> center vertical not center of screen right ? That would imply following UI
> from mock vid (with FF logo in center)

The video mock is there to demonstrate the animations, but we should be using the static visuals from comment 5. Does that make sense?

Comment 26

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

Here is the action bar build: https://people.mozilla.com/~mcomella/apks/prateek-1155860-ab.apk

Prateek, your first patch doesn't apply. It fails with:

... ShareDialog.java:186: error: cannot find symbol
 0:10.53         openBrowserButton = (OverlayDialogButton) findViewById(R.id.overlay_share_open_browser_btn);
 0:10.53   symbol:   variable overlay_share_open_browser_btn

Can you fix up the patch so I can get antlam a sample build?

Comment 27

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

(In reply to Prateek Arora from comment #23)
> Created attachment 8660342 [details]
> open_in_external.png
> 
> Just thinking out loud, but, going to top of the screen to open in firefox,
> for big screens.. I dont know ... may be inconvenient.

I'm not too worried about this since at this point, the user has chosen to "Add to Firefox" through a "Share with/via" option in another app rather than just opening a link with Firefox. So, we're not really prioritizing for them to easily open with Firefox here.

> Here's a thought, we are already displaying title and link.. why not have a
> cool "open in external" icon near link... That is intuitive, easily
> understood. I am talking about standard icon, every one knows stands for
> "opening in external"  [attachment is just off the top google image search,
> real icon will be mozified or in material style .. we can discuss that ]
>
> Also since user already clicked "add in firefox" previously, its obvious
> that open in external button will open the link in Firefox

Perhaps, but I'd like to clearly define and separate the hit areas here. For example, top is to "open in Firefox", and the bottom is where we group "Add to Firefox" features, while the middle is reserved for easy dismissal and simple visual verification ("what was I sharing again?"). 

Also, this button is the most important thing on the screen at this stage in the experience. So, I'm hesitant to offer it such prominence in the UI.

Comment 28

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

(In reply to Michael Comella (:mcomella) from comment #25)
> (In reply to Prateek Arora from comment #22)
> > [1] I tried to do actual ActionBar, but simple getActionBar wont work. Also,
> > for actionbar I would have to take care of different versions of android etc.
> 
> I think ActionBar makes assumptions about how it will be used and how it
> looks so I'd agree with avoiding it.
> 
> > So I went with your method, slightly modified, ...
> 
> Sure, whatever is fastest. :)
> 
> > [2] The build icon on the left, I guess Anthony wanted 15dp left padding ?
> > does it seem too much ? To me it did. Is this how it will appear if we used
> > actual actionbar ?
> 
> Anthony?

Yes, 15dp. This is consistent with all our left-most and right-most margins so let's try that.

> > [3] Anthony also mentioned center for build icon ? I am assuming he meant
> > center vertical not center of screen right ? That would imply following UI
> > from mock vid (with FF logo in center)
> 
> The video mock is there to demonstrate the animations, but we should be
> using the static visuals from comment 5. Does that make sense?

Yes, follow comment 5.

Comment 29

[Prateek Arora]

Attachment: bug1155860_Btn_Approach.patch

Michael, I don't get it why that patch didn't work. Try this one, maybe ! I am able to build successfully and have "to-try" apk with me. Can I share the link for that ? (through GDrive or something, because dont have people.mozilla account). Let me know what else is needed.

Comment 30

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

(In reply to Prateek Arora from comment #29)
> and have "to-try" apk with me. Can I share the link for that ?
> (through GDrive or something,

Sure! I was hosting the APKs because I don't have any bandwidth/storage considerations and generally people do. But if you've got a solution, you're more than welcome to host it! Let me know if you need any help.

Comment 31

[Prateek Arora]

https://drive.google.com/file/d/0B8Ygm1nYMNJ3azZCQW8tMGRHVGc/view?usp=sharing

Actually I don't have bandwidth or storage considerations per say, I wasn't sure if it is allowed to share external links or only the ones hosted at people.mozilla.org/ is allowed :)

Let me know, which of 2, (or a suggestion for 3rd) works out ! I will clean the code and attach corresponding patch

Thanks :)

Comment 32

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

Anthony, can you compare the build in comment 31 and the build in comment 26?

Comment 33

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

Thanks guys! It's always nice to be able to compare builds like this. Seeing how these things feel on an actual device really helps.

Mcomella, for your build, I'm concerned with discover-ability. Even if we fix the animation to be exactly the same speed and animation. Prateek, what're your thoughts? I know you mentioned concerns for tap areas before :)

It seems like on smaller devices, mcomella's build would be fine but if we're putting this in now. Perhaps its best to be more obvious at first. Then, we can get some more telemetry to help inform how best to optimize this list of items as it grows.

From a UX point of view, I'd like to go with what Prateek has in his build (adding it as another list item). But, I'd like to add it to the bottom of the list. I think it makes the most sense in this context to have it closer to the bottom.

Prateek, can we make sure the icon is the right size and color? 30dp wide and #5F6368 (Toolbar icon grey)

Comment 34

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

Attachment: prev_shareoverlay_mock6.png

Comment 35

[Prateek Arora]

(In reply to Anthony Lam (:antlam) from comment #33)
 
> Mcomella, for your build, I'm concerned with discover-ability. Even if we
> fix the animation to be exactly the same speed and animation. Prateek,
> what're your thoughts? I know you mentioned concerns for tap areas before :)

I think you are right. When the overlay screen pops up, the eyes directly wander to the menu below the Title, making it a tad harder from discovery perspective. Also, were the animations not correct ? Tried to adjust the numerics to have same speed (Again try n hit adjustments I am afraid)


> Prateek, can we make sure the icon is the right size and color? 30dp wide
> and #5F6368 (Toolbar icon grey)

Patch coming up in next comment

Comment 36

[Prateek Arora]

Attachment: bug1155860_btn.patch

PS: In ShareDialog.java, alot is hardcoded depending on which item is first in the list. If we ever need to change the list order, that code needs to be changed again. (in this case readingList background resource etc.)

Comment 37

[Prateek Arora]

Attachment: bug1155860_btn_NO_EXTRA_PNG.patch

So, previous patch adds a png resource. As discussed with Mike, because of bug942609 we try to keep apk size minimum. So, in order to reuse icon_search_empty_firefox.png, I had to put in hardcoded 30dp height and a tint.

Comment 38

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

(In reply to Prateek Arora from comment #35)
> (In reply to Anthony Lam (:antlam) from comment #33)
>  
> > Mcomella, for your build, I'm concerned with discover-ability. Even if we
> > fix the animation to be exactly the same speed and animation. Prateek,
> > what're your thoughts? I know you mentioned concerns for tap areas before :)
> 
> I think you are right. When the overlay screen pops up, the eyes directly
> wander to the menu below the Title, making it a tad harder from discovery
> perspective. Also, were the animations not correct ? Tried to adjust the
> numerics to have same speed (Again try n hit adjustments I am afraid)

Don't worry about this if we're just adding another item to the list at the bottom. 

I was referring to Mcomella's build with the top action bar animating in.

Comment 39

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

(In reply to Prateek Arora from comment #36)
> PS: In ShareDialog.java, alot is hardcoded depending on which item is first
> in the list. If we ever need to change the list order, that code needs to be
> changed again. (in this case readingList background resource etc.)

That's unfortunate – can you point me at the code you're referring to? If it's somewhat-easily fixed, you can file a bug instead.

Comment 40

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

Comment on attachment 8666869 [details] [diff] [review]
bug1155860_btn_NO_EXTRA_PNG.patch

Review of attachment 8666869 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +116,5 @@
>  
>  <!-- Localization note (overlay_share_bookmark_btn_label) : This string is
>       used in the share overlay menu to select an action. It is the verb
>       "to bookmark", not the noun "a bookmark". -->
> +<!ENTITY overlay_share_open_browser_btn_label "Open in Firefox">

I think we'd want `Open in Nightly` in the appropriate builds. You can use `&brandShortName;` instead of `Firefox`.

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +183,5 @@
>  
>          bookmarkButton = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
>          readingListButton = (OverlayDialogButton) findViewById(R.id.overlay_share_reading_list_btn);
> +        openBrowserButton = (OverlayDialogButton) findViewById(R.id.overlay_share_open_browser_btn);
> +        

nit: ws.

@@ +201,5 @@
>              public void onClick(View view) {
>                  addToReadingList();
>              }
>          });
> +        

nit: ws

@@ +280,5 @@
>          if (state == State.DEVICES_ONLY) {
>              bookmarkButton.setVisibility(View.GONE);
>              readingListButton.setVisibility(View.GONE);
> +            openBrowserButton.setVisibility(View.GONE);
> +            

nit: ws

::: mobile/android/base/resources/layout/overlay_share_button.xml
@@ +13,1 @@
>          android:scaleType="center"/>

I think you need centerInside here.

Comment 41

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aebfea5a1cea

Comment 42

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

Attachment: Incorrect scaling

Why I think scaleType="centerInside" is necessary.

Comment 43

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

By the way Prateek, if you want to make your own try pushes, I'd vouch for level 1 commit access for you! See [1] about our commit access policy and [2] for a sample commit access bug.

[1]: http://www.mozilla.org/hacking/committer/
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175275

Comment 44

[Prateek Arora]

(In reply to Michael Comella (:mcomella) from comment #39)
> (In reply to Prateek Arora from comment #36)
> > PS: In ShareDialog.java, alot is hardcoded depending on which item is first
> > in the list. If we ever need to change the list order, that code needs to be
> > changed again. (in this case readingList background resource etc.)
> 
> That's unfortunate – can you point me at the code you're referring to? If
> it's somewhat-easily fixed, you can file a bug instead.

In "handleSendTabUIEvent"
> // The first item in the list has a unique style. If there are no items
> // in the list, the next button appears to be the first item in the list.
> //
> // Note: a more thorough implementation would add this
> // (and other non-ListView buttons) into a custom ListView.
> if (remoteClientRecords == null || remoteClientRecords.length == 0) {
>     readingListButton.setBackgroundResource(
>             R.drawable.overlay_share_button_background_first);
> }

We are setting the background for readingListButton, because it is the first item. And in onCreate, we are saving its state,

> readingListButtonDrawable = readingListButton.getBackground();

and re-assigning it in onResume(),

> readingListButton.setBackgroundDrawable(readingListButtonDrawable);

My doubt is, if my first item in list, is other than readingList, say "Open in Firefox", will all of this not change to openBrowserButton instead ?

Comment 45

[Prateek Arora]

Attachment: Screenshot (03-Oct-2015 8_17_54 pm).png

So, centerInside kind of turned the icons upside down. 

Besides, I don't think I can re-use the icon_search_empty_firefox.png because I just realized, we cannot used hardcoded tint, since "Already bookmarked" and "Added to reading list" icons have to be of different colour. The hardcoded tint will make them appear like this, which I don't think is desirable.

I think we are left with one option, but to create a separate resource for this. The resource will be appropriately sized and tinted, and since we have been using scaleType:center till now, I would like to give that a try again with appropriately sized additional resource.

Comment 46

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

(In reply to Prateek Arora from comment #44)
> My doubt is, if my first item in list, is other than readingList, say "Open
> in Firefox", will all of this not change to openBrowserButton instead ?

That's correct – the fix, imo, would be very low priority though.

Comment 47

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

(In reply to Prateek Arora from comment #45)
> So, centerInside kind of turned the icons upside down.

lol, I'm not sure how this is possible. Do you have any other devices you can test on because I didn't see this problem locally? Where did you add centerInside?

> Besides, I don't think I can re-use the icon_search_empty_firefox.png
> because I just realized, we cannot used hardcoded tint, since "Already
> bookmarked" and "Added to reading list" icons have to be of different
> colour.

We can dynamically tint by ID – see DrawableUtil, which relies on some support library APIs to add tint.

> I think we are left with one option, but to create a separate resource for
> this.

Because of my previous idea, I don't think this is necessary.

Comment 48

[Prateek Arora]

Sorry for delays, was on PTO (festival time in India)..

So, the centerInside was done in overlay_share_button.xml for ImageView. It contained center as android:scaleType before.

Actually, ImageView is kind of weird in overlay_share_button.xml

>    <ImageView
>        android:layout_width="60dp"
>        android:layout_height="match_parent"
>        android:id="@+id/overlaybtn_icon"
>        android:padding="30dp"
>        android:scaleType="center"/>

But we cannot keep height as match_parent anymore because we want to use existing resources.
I think upside down thing is also related to this size issue. So, I tried width:45dp and paddingLeft:15dp because that makes sense. This is good except for rightPadding (Text is now touching the icon). I will try to fix this. Meanwhile can you help me comprehend, how android lays out width:60dp and padding:30dp ? How is that possible ? 

Basically, we want icon to be 30 dp right ? with adequate spaces around (I think that would be 15dp).
I will try a few more things and let you know

Comment 49

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

(In reply to Prateek Arora from comment #48)
> Sorry for delays, was on PTO (festival time in India)..

No worries – you're entitled to your time off too! I hope you enjoyed yourself! :)

> Meanwhile can you help me comprehend, how android lays out width:60dp
> and padding:30dp ? How is that possible ? 

ImageViews are weird in my experience – afaict, it ignores padding. I assume our current code works because our assets are the desired size and centered in the view via scaleType. The padding is ignored and the width=60dp spaces out the image correctly, as if there was padding.

Rather, with ImageViews, layout_margin* is useful.

> Basically, we want icon to be 30 dp right ?

From the previous layouts, I would assume so. If it looks different than it did previously, you can find the size of the raw asset and use that value.

Comment 50

[Prateek Arora]

Attachment: Screenshot (18-Oct-2015 8_02_42 pm).png

Mike, thanks for the layout_margin tip. It worked. I set height and width to 30dp each that was needed, and layout_margin to 15dp. Its looking correct.

Now, the TINT is left. I need to apply "toolbar_icon_grey" tint when the state is enabled only. But before going into that, I feel "fr33styler's Firefox on p" and "Fennec fr33styler on Oneplus One" in the screenshot attached, they dont match with "toolbar_icon_grey" color of add to reading list or bookmark.

I tried some things for tintint, android:tint only working for 5.0 + versions. DrawableCompat.setTint is for drawable, not for imageview. ImageView.setColorFilter not working correctly. Still figuring out how to apply tint only when button is enabled. 

But would like your thoughts on above observation. If we need to change their color as well ? (Because if we are able to implement "Tint when enabled, I think it will also Tint first 2 buttons as well. Don't know if this is desired)

Comment 51

[Prateek Arora]

Attachment: Screenshot (18-Oct-2015 11_06_33 pm).png

So, this is what it looks like with tinting. As I said, it will also tint the first 2 items, that appeared to be of a bit different color by default. FWIW, I think we should tint them, and this attachment as a end result looks good to me.
Let me know, and I'll post the patch soon

Comment 52

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

Anthony, comment 50?

As far as tint is concerned, since Android only supports tintLists in some higher API level, I made drawableTintList [1] for use with our ThemedImage* views which will apply tint colors given different tint. Use the ThemedImage* version of a View rather than the Android version and apply the drawableTintList attribute on the View.

I would imagine all the of the icons should be tinted the same color, but you'll have to talk to Anthony.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=drawabletintlist&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 53

[Prateek Arora]

(In reply to Michael Comella (:mcomella) from comment #52)
> Anthony, comment 50?
> 
> As far as tint is concerned, since Android only supports tintLists in some
> higher API level, I made drawableTintList [1] for use with our ThemedImage*
> views which will apply tint colors given different tint. Use the
> ThemedImage* version of a View rather than the Android version and apply the
> drawableTintList attribute on the View.
> 
> I would imagine all the of the icons should be tinted the same color, but
> you'll have to talk to Anthony.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/
> search?string=drawabletintlist&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24
> &hitlimit=&tree=mozilla-central

Thanks, I'll give that a try, but presently I have applied ColorFilter. I think that would work for pre-lollipop as well. Let me test for that. If it works for pre-lollipop, can I go with that approach or have to go with ThemedImage* ?

Comment 54

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

(In reply to Prateek Arora from comment #53)
> can I go with that approach or have to go with ThemedImage* ?

I'd prefer the ThemedImage* approach as defining colors in XML is more robust. In fact, my motivation for writing DrawableTintList was because the ColorFilters I was using were hard to manage, particularly when adding state (e.g. private *and* enabled *and* blah blah).

Comment 55

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

(In reply to Michael Comella (:mcomella) from comment #52)
> Anthony, comment 50?
> 
> As far as tint is concerned, since Android only supports tintLists in some
> higher API level, I made drawableTintList [1] for use with our ThemedImage*
> views which will apply tint colors given different tint. Use the
> ThemedImage* version of a View rather than the Android version and apply the
> drawableTintList attribute on the View.
> 
> I would imagine all the of the icons should be tinted the same color, but
> you'll have to talk to Anthony.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/
> search?string=drawabletintlist&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24
> &hitlimit=&tree=mozilla-central

Hey Prateek!

Awesome job, it's almost there!

It seems like there's some inconsistencies here with the color. These icons should all be "toolbar icon grey" since the menu is "toolbar grey". And the text should all be "text and tabs tray grey". Regardless of state.

That should make it easier :) thanks!

Comment 56

[Prateek Arora]

Attachment: Screenshot (22 Oct).png

How does this look ?

1) All icons toolbar_icon_grey
2) All text text_and_tabs_tray_grey
3) Used ThemedImageView for tint

Patch to follow. Thanks guys :)

Comment 57

[Prateek Arora]

Attachment: bug1155860_final2.patch

Mike, I fixed 2 out of 3 nit:ws previously mentioned. But 2nd nit mentioned, that whitespace isn't extra and needed to be there. 

Thanks for suggesting ThemedImageView.

Comment 58

[Prateek Arora]

Attachment: bug1155860_final2.patch

Sorry forgot to add  r?

Comment 59

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

(In reply to Prateek Arora from comment #56)
> Created attachment 8677308 [details]
> Screenshot (22 Oct).png
> 
> How does this look ?
> 
> 1) All icons toolbar_icon_grey
> 2) All text text_and_tabs_tray_grey
> 3) Used ThemedImageView for tint
> 
> Patch to follow. Thanks guys :)

looking good! Thanks Prateek!

But, shouldn't "Already bookmarked" be "disabled grey" (#BFBFBF) here? that should be the case for both the icon and the text.

Comment 60

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

(In reply to Prateek Arora from comment #57)
> Mike, I fixed 2 out of 3 nit:ws previously mentioned. But 2nd nit mentioned,
> that whitespace isn't extra and needed to be there.

There were spaces at the start of a line so rather than the newline being

""

it was:

"    "

Comment 61

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

Comment on attachment 8677318 [details] [diff] [review]
bug1155860_final2.patch

Review of attachment 8677318 [details] [diff] [review]:
-----------------------------------------------------------------

Holding off on full review until the patch meets UX expectations (r? me then, Prateek).

::: mobile/android/base/resources/layout/overlay_share_button.xml
@@ +9,5 @@
>          android:id="@+id/overlaybtn_icon"
> +        android:layout_width="30dp"
> +        android:layout_height="30dp"
> +        android:layout_margin="15dp"
> +        gecko:drawableTintList="@color/toolbar_icon_grey"

You should use a color state list here for the disabled state. Maybe you can re-use a file like:
  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/color/action_bar_secondary_menu_item_colors.xml

Comment 62

[Prateek Arora]

Attachment: Screenshot_org.mozilla.fennec_fr33styl3r_2015-10-27-21-29-51.png

> But, shouldn't "Already bookmarked" be "disabled grey" (#BFBFBF) here? that
> should be the case for both the icon and the text.

Yeah ! That was my understanding as well but comment 55 was a bit confusing

> These icons should all be "toolbar icon grey" since the menu is "toolbar grey". And the text should 
> all be "text and tabs tray grey". Regardless of state. 

Anyways, this attachment looks fine right ?

Mike, I recently pulled so that I have latest files to patch on, but building after latest pull is failing to build. There was clobber, so I mach clobbered. The error I am facing is 

> 0:20.90 configure:14602:26: error: conflicting types for 'malloc_usable_size'
> 0:20.90 In file included from configure:14600:0:
> 0:20.90 /home/fr33styl3r/Development/android-ndk-r8e/platforms/android-9/arch 
> arm/usr/include/malloc.h:36:15: note: previous declaration of
> 'malloc_usable_size' was here

Is this some ndk version fault ? sdk ver fault ? I am gonna hg pull again and try. Other than this the patch is ready

Comment 63

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

WFM! thanks Prateek!

Comment 64

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

It'd be nice to be able to get some telemetry on this item as well.

Comment 65

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

(In reply to Anthony Lam (:antlam) from comment #64)
> It'd be nice to be able to get some telemetry on this item as well.

We already do other telemetry here, for example:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java#403

Sounds like we'd put something here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java#416

I don't think we'd use Event.LOAD_URL here because we'd already use Event.LOAD_URL in Fennec. We could use Event.LAUNCH for this.

Comment 66

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

(In reply to Prateek Arora from comment #62)
> Mike, I recently pulled so that I have latest files to patch on, but
> building after latest pull is failing to build. There was clobber, so I mach
> clobbered. The error I am facing is 
> 
> > 0:20.90 configure:14602:26: error: conflicting types for 'malloc_usable_size'
> > 0:20.90 In file included from configure:14600:0:
> > 0:20.90 /home/fr33styl3r/Development/android-ndk-r8e/platforms/android-9/arch 
> > arm/usr/include/malloc.h:36:15: note: previous declaration of
> > 'malloc_usable_size' was here
> 
> Is this some ndk version fault ? sdk ver fault ? I am gonna hg pull again
> and try. Other than this the patch is ready

I run ndk-r10e. If you don't have bandwidth or hard drive space restrictions, nalexander built a tool that would download the Android SDK of the appropriate version into ~/.mozbuild and keep it up to date. You can access this by running `./mach bootstrap`. If you have hard drive space restrictions, you could 1) download the ndk manually or 2) delete your existing SDK and point everything at the .mozbuild SDK.

If you try updating your NDK, don't forget to update your mozconfig!

Comment 67

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

(In reply to Anthony Lam (:antlam) from comment #64)
> It'd be nice to be able to get some telemetry on this item as well.

Filed bug 1219444 and assigned Prateek.

Comment 68

[Prateek Arora]

Attachment: bug1155860_final.patch

Comment 69

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

Comment on attachment 8681404 [details] [diff] [review]
bug1155860_final.patch

Review of attachment 8681404 [details] [diff] [review]:
-----------------------------------------------------------------

Implementation looks good to me! Thanks Prateek!

However, it doesn't consistently handle tab queue (our "Open multiple links" feature). I spoke with Anthony and he mentioned it should ignore tab queue (i.e. open in Firefox directly). It does this but will display the tab queue prompt if you do it enough times.

To fix this, I think you should just have to add the SKIP_TAB_QUEUE flag to launchBrowser – see bug 1216333 for an example. You could fix it here or file a follow-up to take care of it.

Comment 70

[Prateek Arora]

I would like to have a separate follow-up for that. I was thinking of this order, bug115860 --> then bug1219444 -- and then handle tab queue. What do you think ?

Comment 71

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

(In reply to Prateek Arora from comment #70)
> I would like to have a separate follow-up for that. I was thinking of this
> order, bug115860 --> then bug1219444 -- and then handle tab queue. What do
> you think ?

Provided they all land in the same releases, this works for me. When this lands, can you add "tracking-fennec" flags equal to "?" and mention you'd like to it track the release this lands in? We're currently on the 45 cycle.

Comment 72

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

Any movement here, Prateek?

Comment 73

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

Reset assignee due to inactivity.

Comment 74

[Shubham]

Hi Michael,
Seems like only change that needs to be added to patch by Prateek is "launchBrowser" with SKIP_TAB_QUEUE flag. I can quickly create a patch for this.

Comment 75

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

(In reply to Shubham from comment #74)
> Seems like only change that needs to be added to patch by Prateek is
> "launchBrowser" with SKIP_TAB_QUEUE flag. I can quickly create a patch for
> this.

Sure, can you go ahead and post it for review?

Comment 76

[Shubham]

Attachment: Bug 1155860 - open a shared link -r?ahunt

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

Comment 77

[Shubham]

(In reply to Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] from comment #75)
> (In reply to Shubham from comment #74)
> > Seems like only change that needs to be added to patch by Prateek is
> > "launchBrowser" with SKIP_TAB_QUEUE flag. I can quickly create a patch for
> > this.
> 
> Sure, can you go ahead and post it for review?
Hi Michael,
I have posted it for the review. Please check. Thanks.

Comment 78

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

Comment on attachment 8800906 [details]
Bug 1155860 - open a shared link -r?ahunt

Grisha, do you mind taking a look at this? Thanks.

Comment 79

[Shubham]

Hi Grisha,
Any feedback on this. Thanks.

Comment 80

[:Grisha Kruglov]

Andrzej, would you mind taking a look at this?

Comment 81

[Andrzej Hunt :ahunt]

Comment on attachment 8800906 [details]
Bug 1155860 - open a shared link -r?ahunt

https://reviewboard.mozilla.org/r/85722/#review93704

Looks good to me. This patch does however introduce a crash, see review comment below - other than that this looks good to go!

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java
(Diff revision 1)
>          // Register ourselves as both the listener and the context for the Adapter.
>          final SendTabDeviceListArrayAdapter adapter = new SendTabDeviceListArrayAdapter(this, this);
>          sendTabList.setAdapter(adapter);
>          sendTabList.setSendTabTargetSelectedListener(this);
>  
> -        bookmarkButton = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);

Why was bookmarkButton removed here? That seems to cause an NPE in the line below (bookamrkButton.getBackground() will crash)?

Comment 82

[Shubham]

Comment on attachment 8800906 [details]
Bug 1155860 - open a shared link -r?ahunt

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

Comment 83

[Shubham]

(In reply to Andrzej Hunt :ahunt from comment #81)
> Comment on attachment 8800906 [details]
> Bug 1155860 - open a shared link -r?ahunt
> 
> https://reviewboard.mozilla.org/r/85722/#review93704
> 
> Looks good to me. This patch does however introduce a crash, see review
> comment below - other than that this looks good to go!
> 
> ::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java
> (Diff revision 1)
> >          // Register ourselves as both the listener and the context for the Adapter.
> >          final SendTabDeviceListArrayAdapter adapter = new SendTabDeviceListArrayAdapter(this, this);
> >          sendTabList.setAdapter(adapter);
> >          sendTabList.setSendTabTargetSelectedListener(this);
> >  
> > -        bookmarkButton = (OverlayDialogButton) findViewById(R.id.overlay_share_bookmark_btn);
> 
> Why was bookmarkButton removed here? That seems to cause an NPE in the line
> below (bookamrkButton.getBackground() will crash)?

Hi Andrzej,
I have fixed it. Can you please check.

Comment 84

[Andrzej Hunt :ahunt]

Comment on attachment 8800906 [details]
Bug 1155860 - open a shared link -r?ahunt

https://reviewboard.mozilla.org/r/85722/#review98658

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e328cabfbdf
open a shared link -r?ahunt

Comment 86

[Shubham]

(In reply to Andrzej Hunt :ahunt from comment #84)
> Comment on attachment 8800906 [details]
> Bug 1155860 - open a shared link -r?ahunt
> 
> https://reviewboard.mozilla.org/r/85722/#review98658

Hi Andrzej,
Can you help me finding a new bug to work on, it will be super helpful. Many Thanks.

Comment 87

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e328cabfbdf

Comment 88

[Andrzej Hunt :ahunt]

(In reply to Shubham from comment #86)
> (In reply to Andrzej Hunt :ahunt from comment #84)
> > Comment on attachment 8800906 [details]
> > Bug 1155860 - open a shared link -r?ahunt
> > 
> > https://reviewboard.mozilla.org/r/85722/#review98658
> 
> Hi Andrzej,
> Can you help me finding a new bug to work on, it will be super helpful. Many
> Thanks.

I'd recommend looking at bugs with "[good next bug]" in the whiteboard, I've created a search using:
https://bugzilla.mozilla.org/buglist.cgi?v21=bug%5D&o13=substring&list_id=13363913&v11=next&o9=substring&v10=next&f13=cf_crash_signature&o2=substring&f23=cf_crash_signature&status_whiteboard_type=allwordssubstr&f12=status_whiteboard&f25=CP&v9=next&o18=substring&status_whiteboard=%5Bgood%20next%20bug%5D&j17=OR&v18=bug%5D&o12=substring&f14=CP&o19=substring&o20=substring&f24=CP&v2=good&f21=short_desc&v13=next&f10=alias&f19=component&f22=status_whiteboard&f1=OP&o21=substring&o3=substring&f20=alias&f0=OP&f8=product&v3=good&o11=substring&f18=product&v19=bug%5D&f15=CP&f9=component&j7=OR&v20=bug%5D&f4=CP&o23=substring&query_format=advanced&o10=substring&v23=bug%5D&j1=OR&f3=status_whiteboard&f2=short_desc&v12=next&f11=short_desc&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&f5=CP&f17=OP&v8=next&o22=substring&f6=OP&product=Firefox%20for%20Android&f7=OP&v22=bug%5D&o8=substring&f16=OP

If you're interested in more UI bugs, a good one might be Bug 1200521 - issues without layouting our SSL icon.

Comment 89

[Oana Horvath [:ohorvath]]

Verified as fixed in Nightly build 53.0a1 2017-01-09;
Devices: 
LG G4 Android  5.1;
Lenovo A536 Android 4.4.2;
Asus ZenPad (Android 6.0.1).