Closed Bug 1097193 Opened 5 years ago Closed 5 years ago

Increase close button hit area in tabs panel

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1086221 didn't seem to make the hit area bigger - we need to investigate
The dimensions of the hit area have been tweaked slightly and have moved the button to the front of the view hierarchy - this should help register close button presses better than before.
Attachment #8520825 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8520825 [details] [diff] [review]
Increase close button hit area in tabs panel

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

::: mobile/android/base/tabs/TabsLayoutItemView.java
@@ +87,4 @@
>          mThumbnail = (ImageView) findViewById(R.id.thumbnail);
>          mCloseButton = (ImageButton) findViewById(R.id.close);
>          mThumbnailWrapper = (TabThumbnailWrapper) findViewById(R.id.wrapper);
> +        

nit: remove trailing space.

@@ +87,5 @@
>          mThumbnail = (ImageView) findViewById(R.id.thumbnail);
>          mCloseButton = (ImageButton) findViewById(R.id.close);
>          mThumbnailWrapper = (TabThumbnailWrapper) findViewById(R.id.wrapper);
> +        
> +        mCloseButton.bringToFront();

Unless I'm missing something, this call isn't really doing anything. bringToFront() will simply re-add the given view as the last child in its container. But the close button is already the last child of its container. You can safely remove this call, unless I'm missing something here.

@@ +98,5 @@
>                  final Rect r = new Rect();
>                  mCloseButton.getHitRect(r);
> +                r.left -= 20;
> +                r.top -= 5;
> +                r.right += 5;

This is growing the hit area beyond the touch delegate container bounds. It's a no-op as far as I know. The actual problem here seems to be that you're using pixel dimensions here (25) which will be a lot smaller in xhdpi+ devices. I missed this in my original review.

I think the right thing to do here is to ensure the close button has a hit area of at least 40x40dp.
Attachment #8520825 - Flags: review?(lucasr.at.mozilla)
So, I've tweaked the hit rectangle to make it as close to 40x40dp as possible.  We don't want to / can't have the hit area over top of the thumbnail, so instead we set the height to match the parent container view and make the width 40dp.
Attachment #8520825 - Attachment is obsolete: true
Attachment #8522193 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8522193 [details] [diff] [review]
Increase close button hit area in tabs panel

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

Looks good with the suggested changes.

::: mobile/android/base/tabs/TabsLayoutItemView.java
@@ +99,4 @@
>  
> +                // Ideally we want the close button hit area to be 40x40dp but we are constrained by the height of the parent, so
> +                // we make it as tall as the parent view and 40dp across.
> +                final int TARGET_HIT_AREA = (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 40, getResources().getDisplayMetrics());;

nit: we don't usually name local variables with all caps. targetHitArea is fine here.

@@ +104,5 @@
> +
> +                hitRect.top = 0;
> +                hitRect.right = parent.getWidth();
> +                hitRect.left = hitRect.right - TARGET_HIT_AREA;
> +                hitRect.bottom = parent.getHeight();

Remember, the hitRect here represents coordinates within the top level TabsLayoutItemView container. So, you can probably just do this:

hitRect.left = getWidth() - targetHitArea;
hitRect.right = getWidth();
hitRect.top = 0;
hitRect.bottom = targetHitArea;
Attachment #8522193 - Flags: review?(lucasr.at.mozilla) → review+
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/d075a37f1c2a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.