Closed Bug 1009279 Opened 7 years ago Closed 7 years ago

Increase browser toolbar editing mode hit targets by changing margin to padding

Categories

(Firefox for Android :: Awesomescreen, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file, 1 obsolete file)

We fixed the tap through in 1001243, but the button hit targets still aren't perfect.
Comment on attachment 8421374 [details] [diff] [review]
Change the margins to padding in the browser toolbar.

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

I'm not sure we need this. When I wrote https://bugzilla.mozilla.org/show_bug.cgi?id=1001243#c6, I was under the misconception that margins were being used for the entire button, so clicking anywhere outside the small X would click the background. But padding is in fact already being used for the button; margins are used only to offset the button's position from the URL bar. Given that, I would think the hit area should already be correct, no?
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> But padding is in fact already being used for the button; margins are used only
> to offset the button's position from the URL bar.

I'm not sure what you mean by this - the button has margins, and when the margins are clicked, the `onClick` event for the button is not fired. By changing this to padding, we're increasing the size of the hit target, making it harder to have a screen tap that doesn't fire.

I also like this patch because it simplifies the translation code, and simplifies the layout by merging the margins and padding into just padding.

So, no, we don't need it, but I think it's nice.
Comment on attachment 8421374 [details] [diff] [review]
Change the margins to padding in the browser toolbar.

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

I don't think it's a big deal to have a sliver of non-touchable area between the button and the URL bar; if the user taps there and nothing happens, so be it -- they weren't close enough to either of the hit areas. It also seems weird that the hit area is now lopsided and not a balanced rectangle like it was before.

That said, it does simplify the code slightly, and I doubt anyone will complain either way, so this is fine with me.

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +106,4 @@
>                 android:contentDescription="@string/edit_mode_cancel"
>                 android:visibility="invisible"/>
>  
> +    <!-- The space to the left of the cancel button is larger than the right because

s/is/would be/
Attachment #8421374 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/3e2b3f5a87e2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.