Closed Bug 966654 Opened 7 years ago Closed 6 years ago

Close button tap target on snippet banner is too small

Categories

(Firefox for Android :: General, defect, P5)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 37
Tracking Status
fennec + ---

People

(Reporter: wenzel, Assigned: imjalpreet, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(4 files, 7 obsolete files)

Firefox for Android, latest nightlies, has a "Thank you for using Nightly!" banner on startup. Which is a nice sentiment.

Unfortunately, it shows up every single time you, say, click on a link in Facebook or Twitter and it sends you over to Firefox.

Issues:
- I really only need to see this once.
- it blocks part of the content I came here to see and never (?) disappears on its own
- the click target to close it is small
- when you miss that click target, which happens frequently, it takes you to the nightly.m.o -- I have no idea why, since I am already running Nightly.

May I suggest:
- only show this once. If you really want to, show it once per version (though remember, these are nightlies which would mean you show it daily).
- don't take me to nightly.m.o when I click the banner. Instead, make the entire thing the click target for closing it.
Few things in here, covering content (bug 966307). The never show again is bug 961523.

> - when you miss that click target, which happens frequently, it takes you to the nightly.m.o -- I have no idea why, since I am already running Nightly.

It's just a test right now; I should hope that more meaningful snippets in the future provide a more meaningful URL.

That leaves the tap close target issue, I suppose?

Morphing summary
tracking-fennec: --- → ?
Flags: needinfo?(margaret.leibovic)
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: "Thank you" banner on Nightly is shown too often → Close button tap target on snippet banner is too small
Everthing Aaron said was correct. I just landed a backout on fx-team that will fix bug 966307.
Flags: needinfo?(margaret.leibovic)
tracking-fennec: ? → +
Hardware: ARM → All
Whiteboard: [mentor=margaret][lang=java][good first bug]
The tap target is small and does not stretch to the edge of screen on tablets (tested on Nexus 7, see screenshot).
This creates a bad user experience, as you end up clicking on the banner when trying to close it by clicking on the right edge.
Hi, I would like to take this up if it's free.
(In reply to Anuj Sahai from comment #4)
> Hi, I would like to take this up if it's free.

Hi Anuj! First of all, you'll need a Firefox for Android build environment. If you don't have that set up you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

To fix this bug, you should look into increasing the size of this close button view, which is defined here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_banner_content.xml

We don't want to change the appearance of this view, but maybe we can add some left/right padding to make a wider tap area.
Assignee: nobody → sahai.anuj16
Can I take this up? It's been a while since the previous person responded. I've set up the build and I ran it on a tablet running android, and I found the place where the close button size needs to be increases. Is it sufficient to just add paddings on either sides or do I have to do more? Also when I run the build on my phone, the same banner is not appearing. If I can be assigned to this bug, I'd be glad to work more on it.
(In reply to ashwinswaroop93 from comment #6)
> Can I take this up?

Yes, releasing the previous assignee as there's been no progress. I am need-info'ing the mentor on this bug for those particular questions.
Assignee: sahai.anuj16 → ashwinswaroop93
Flags: needinfo?(margaret.leibovic)
(In reply to ashwinswaroop93 from comment #6)
> Can I take this up? It's been a while since the previous person responded.
> I've set up the build and I ran it on a tablet running android, and I found
> the place where the close button size needs to be increases. Is it
> sufficient to just add paddings on either sides or do I have to do more?
> Also when I run the build on my phone, the same banner is not appearing. If
> I can be assigned to this bug, I'd be glad to work more on it.

Yes, adding padding on both sides sounds like the right approach. 

Are you testing with a fresh profile on your phone? If not, if you ever hit the "x" button (or if you already have sync set up), you won't see the sync promo banner. You can also modify the "browser.snippets.updateUrl" and "browser.snippets.updateInterval" prefs if you want to test with your own snippets. You can modify these in about:config, or in mobile.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#809

Here's an example of a test JSON file I've used:
http://people.mozilla.org/~mleibovic/snippets.json
Flags: needinfo?(margaret.leibovic)
I tried to add left and right paddings by adding 2 lines of code to the xml file specified in the post above. The lines were:

    android:paddingLeft="30dip"
    android:paddingRight="26dip"

Now if I try to build the project again, the build is sucessful but it seems to only be building the linux version and not the android version(fennec). So when I run ./mach package, the apk file is no longer being created. I wanted to know if this is due to the changes I made, or if I have to do something else before building, after making changes. I just edited the xml file and then ran ./mach build in the terminal in the mozilla-central folder. Any idea on what the problem might be?
What does your mozconfig file look like? That's the place where you can specify your desired build target. It should look like the example here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Setup_Fennec_mozconfig

For more real-time help, feel free to join #mobile on irc.mozilla.org. People in there could definitely help you with your build problems.
Okay so I found out how to fix the problem thanks to the people in #mobile. Now I added left and right paddings and built the project again. Is this all that is required, or is there more to be done? Now a larger area can be clicked, but the actual close image is the same size.
(In reply to ashwinswaroop93 from comment #11)
> Okay so I found out how to fix the problem thanks to the people in #mobile.
> Now I added left and right paddings and built the project again. Is this all
> that is required, or is there more to be done? Now a larger area can be
> clicked, but the actual close image is the same size.

That sounds great! Now you can follow these directions to create a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Then attach your patch to this bug and set the review? flag to me, to request review from me. Once I review the patch and give it a review+, it can make its way into Nightly.
Attachment #8432213 - Flags: review?(margaret.leibovic)
Comment on attachment 8432213 [details] [diff] [review]
Bug 966654 - Made the close button larger and easier to press

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

It looks like something went wrong when generating your patch, since this patch adds a whole new file, but this file already exists in the tree. You should make sure that your clone of mozilla-central is up-to-date, then make your changes on top of that, and then generate a diff with the new changes. Feel free to hop in #mobile to ask more questions if you need mercurial help.
Attachment #8432213 - Flags: review?(margaret.leibovic)
Attached patch Patch for 966654 (obsolete) — Splinter Review
Attachment #8432213 - Attachment is obsolete: true
Attachment #8436468 - Flags: review?(margaret.leibovic)
This patch is going in the right direction! I just want to get some feedback from antlam (one of our UX designers) about this change.

This change makes the close button view wider, such that the hit area is almost a square (increasing the width to 70dp):

https://dl.dropboxusercontent.com/u/3358452/banner-before.png
https://dl.dropboxusercontent.com/u/3358452/banner-after.png

My main is that this limits the amount of space for text in the banner, which is more of a problem on low-res devices (and also an issue for snippets, which may vary in length).

Maybe we shouldn't increase the width this much on phones. Also, comment 3 mentioned an issue more specific to tablets, so maybe we should increase this width by more on tablets.
Flags: needinfo?(alam)
Comment on attachment 8436468 [details] [diff] [review]
Patch for 966654

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

::: mobile/android/base/resources/layout/home_banner_content.xml
@@ +25,5 @@
>           android:gravity="center_vertical"
>           gecko:ellipsizeAtLine="3"/>
>  
>      <ImageButton android:id="@+id/close"
> +                 android:layout_width="70dip"

Whatever direction we go in, I think we should move this value out into dimens.xml (name it something like home_banner_close_button_width). And we could use that to specify different widths for tablets/phones.

Here's an example of how that works:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/dimens.xml#101
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/gecko_app.xml#42
Attachment #8436468 - Flags: review?(margaret.leibovic) → feedback+
I almost always try to start with a square hit area so this is looking good. However, I do agree with Margaret that this is starting to limit the text a bit. 

Can we try having the same amount of padding on the left/right of the 'x' as the left/right of the Sync graphic? We can take these proportions to the Tablet as well. 

I realize the text wrap issue is another one altogether but let's give that a try because from a structural point of view (and how I've laid it out in my mocks) it works and we can fine tune from there.

Also, as an added bonus do we want to explore having a "pull down" to dismiss action? for these types of snippets? I can see it addressing a lot of UX issues that have come up in this bug but I can see implementation could be a problem.
Flags: needinfo?(alam)
Attached image specs_snippet_mock1.png (obsolete) —
Maybe this will help! Where blue is text, left orange is sync icon, and right orange is the 'x' icon.

If there still seems to be too much space around the 'x' we could adjust padding on the area accordingly (hit area doesn't have to stay completely square).
(In reply to Anthony Lam (:antlam) from comment #18)
> Also, as an added bonus do we want to explore having a "pull down" to
> dismiss action? for these types of snippets? I can see it addressing a lot
> of UX issues that have come up in this bug but I can see implementation
> could be a problem.

I think a swipe to the side (either side) would be in line with what you do for notifications in the notification bar.
(In reply to Fred Wenzel [:wenzel] from comment #20)
> I think a swipe to the side (either side) would be in line with what you do
> for notifications in the notification bar.

Fair point, but visually speaking this does not come across the same as notifications in the notifications bar. Also from a metaphoric stand point, this snippet's background matte is pinned to the bottom and so sliding it downwards completes that experience.

But we should definitely experiment with possibilities here.
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=java][good first bug] → [lang=java][good first bug]
Mentor: nalexander
filter on [mass-p5]
Priority: -- → P5
Hey Ashwin, are you still interested in working on this?
Flags: needinfo?(ashwinswaroop93)
Assignee: ashwinswaroop93 → nobody
Hi, I want to take this up as my first bug if it's free.
Assignee: nobody → jalpreetnanda
Status: NEW → ASSIGNED
Hey, I am facing problem in building Firefox for Android. I am getting these errors when I build. http://snag.gy/1f4Qd.jpg. Secondly, Do I just need to increase the close button tap target area larger or do I need to do something more than that? Waiting for a reply :)
Flags: needinfo?(margaret.leibovic)
(In reply to Jalpreet Singh Nanda from comment #25)
> Hey, I am facing problem in building Firefox for Android. I am getting these
> errors when I build. http://snag.gy/1f4Qd.jpg. Secondly, Do I just need to
> increase the close button tap target area larger or do I need to do
> something more than that? Waiting for a reply :)

The build errors are tracked in Bug 1107134; we'll fix those today.  I'll defer to margaret on the real work to be done here.
Thanks, nalexander, for addressing the build issues.

(In reply to Jalpreet Singh Nanda from comment #25)
> Secondly, Do I just need to
> increase the close button tap target area larger or do I need to do
> something more than that? Waiting for a reply :)

Yep, let's just make that tap target area larger! Let's try implementing the spec that antlam attached in comment 19.

We can also explore some of the other ideas mentioned in this bug, but I think just increasing the tap area of the close button would be the quickest and easiest win.
Flags: needinfo?(margaret.leibovic)
Attached patch Bug-966654-Proposed_Patch (obsolete) — Splinter Review
This is my proposed patch according to the changes suggested by you. I have tried to implement what :antlam was saying. I have checked it on multiple screen sizes and it looks good to me.
Attachment #8533688 - Flags: review?(margaret.leibovic)
Flags: needinfo?(alam)
Comment on attachment 8533688 [details] [diff] [review]
Bug-966654-Proposed_Patch

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

Looking good. Let's just update this patch to not add any margins, and make the new width a bit smaller.

::: mobile/android/base/resources/layout/home_banner_content.xml
@@ +29,5 @@
> +                 android:layout_width="@dimen/home_banner_close_width"
> +                 android:layout_marginLeft="8dp"
> +                 android:layout_marginRight="10dp"
> +                 android:layout_marginTop="6dp"
> +                 android:layout_marginBottom="6dp"

It looks like adding top margin here is creating a top border on the close button, which isn't what we want. Also, adding margin makes the button take up more room visually without increasing the tap area at all, so we shouldn't add any margin at all.

Although antlam's mock-up showed a close button area that didn't extend all the way to the edges of the home banner, we should make the touch area as large as possible, while ensuring that the text doesn't get too squished. Basically, for this bug I think we just need to make the close button a bit wider, which is what you're doing with the width change.

::: mobile/android/base/resources/values/dimens.xml
@@ +146,5 @@
>      <dimen name="page_action_button_width">32dp</dimen>
>  
>      <!-- Banner -->
>      <dimen name="home_banner_height">72dp</dimen>
> +    <dimen name="home_banner_close_width">50dp</dimen>

I didn't see antlam specify this value anywhere. Did you just come up with this from the mock? In the mock it actually looks like the close button should be a bit slimmer than the icon, so I think this value should be smaller. However, we can't make it *too* small, since that wouldn't address the bug. So maybe we should make it somewhere around 40dp? or 44dp?
Attachment #8533688 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Bug-966654-Proposed_Patch (obsolete) — Splinter Review
I Have made the changes as you suggested. Though, I have still kept the left and right margin as in the layout suggested by antlam there was left and right margin around the close button.
Attachment #8534070 - Flags: review?(margaret.leibovic)
Comment on attachment 8534070 [details] [diff] [review]
Bug-966654-Proposed_Patch

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

I'm clearing review for now, and I'll flag antlam for needinfo. In the meantime, you could also upload a screenshot for antlam to review when he looks at this bug again.

::: mobile/android/base/resources/layout/home_banner_content.xml
@@ +27,5 @@
>  
>      <ImageButton android:id="@+id/close"
> +                 android:layout_width="@dimen/home_banner_close_width"
> +                 android:layout_marginLeft="8dp"
> +                 android:layout_marginRight="10dp"

antlam is on vacation this week, so we'll have to wait until next week to ask him to clarify his mock-up, but I really don't think we want this extra margin. There icon itself does not take up the whole width of the view, so there is already the appearance of margin.
Attachment #8534070 - Flags: review?(margaret.leibovic)
Ok, I will attach the screenshot and I will remove that margin as soon as we get a clarification from antlam.
Attached image Screenshot of the banner (obsolete) —
Attachment #8534529 - Flags: feedback?(alam)
Hey Jalpreet!

Sorry, I've been away for the past week. 

It's looking great! Let me look through my files and get you those specs.
Attached image spec_snippet_mock2.png
It looks like the specs were in bug 1064461 (which you're also a part of), so largely nothing has changed, just cleaned up the values slightly to align more with the work that's been going on in the rest of the UI. 

Notes as follows: 
 - Text is unchanged (line-height is finicky, let's keep to what we're using ATM and tweak later)
 - Padding for the left icon is updated
 - Ignore the top and bottom padding of the copy

I've also just called out the values here for you so it should be good if we set these. :)

Hope this helps!
Attachment #8437155 - Attachment is obsolete: true
Flags: needinfo?(alam)
Attached patch Bug-966654-Proposed_Patch (obsolete) — Splinter Review
I have made the changes according to the suggestions you gave. But, I have made padding around home icon to be 11dp rather than the 18dp that is suggested as 18dp is too much according to me and it does not look good. Please give a feedback on what more changes are to be done. Thanks :)
Attachment #8534070 - Attachment is obsolete: true
Attachment #8536531 - Flags: feedback?(margaret.leibovic)
Attachment #8536531 - Flags: feedback?(alam)
Comment on attachment 8536531 [details] [diff] [review]
Bug-966654-Proposed_Patch

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

I'm glad we got a spec from antlam on the close button width. 42dp looks good! Let's just keep the scope of this bug about changing the close button, and not the other view in the home banner.

::: mobile/android/base/resources/layout/home_banner_content.xml
@@ +16,5 @@
>           android:id="@+id/text"
>           android:layout_width="0dip"
>           android:layout_height="match_parent"
>           android:layout_weight="1"
> +         android:layout_marginLeft="11dp"

I think we should leave these margin changes for bug 1064461. Please remove these two margin changes.
Attachment #8536531 - Flags: feedback?(margaret.leibovic) → feedback+
I have made the changes, i.e., removed those two margin changes. I think there were no more changes to be done.
Attachment #8536531 - Attachment is obsolete: true
Attachment #8536531 - Flags: feedback?(alam)
Attachment #8536687 - Flags: review?(margaret.leibovic)
Attachment #8536687 - Flags: feedback?(alam)
Comment on attachment 8536687 [details] [diff] [review]
Bug-966654-Proposed-Patch

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

Great, thanks!
Attachment #8536687 - Flags: review?(margaret.leibovic) → review+
Should I add the checkin-needed keyword?
Flags: needinfo?(margaret.leibovic)
(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #40)
> Should I add the checkin-needed keyword?

Let's wait to get a feedback+ from antlam, and then we can add the checkin-needed keyword.
Flags: needinfo?(margaret.leibovic)
OK! :)
Hey Jalpreet,

Thanks for your work! Can I see a screenshot of the changes?
Flags: needinfo?(jalpreetnanda)
Hey Anthony, I am not getting the banner again as I already have activated sync. So, I am not able to take a screenshot. Can you try it once?
Thanks!
Flags: needinfo?(jalpreetnanda) → needinfo?(alam)
Ok, Here's the screenshot!
Attachment #8534529 - Attachment is obsolete: true
Attachment #8536687 - Flags: feedback?(alam) → feedback+
Looks good, thanks Jalpreet!
Flags: needinfo?(alam)
Attachment #8436468 - Attachment is obsolete: true
Attachment #8533688 - Attachment is obsolete: true
Keywords: checkin-needed
Flags: needinfo?(ashwinswaroop93)
https://hg.mozilla.org/integration/fx-team/rev/4108095d488c

Thanks for the patch! One request, try to use a commit message in the future that describes what your patch is actually doing rather than restating the problem it's fixing. Thanks!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4108095d488c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.