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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

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

We fixed the tap through in 1001243, but the button hit targets still aren't perfect.

Comment 1

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

Attachment: Change the margins to padding in the browser toolbar.

Comment 2

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

Built on the patch for bug 1001243.

Comment 3

[Brian Nicholson (:bnicholson)]

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?

Comment 4

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

(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 5

[Brian Nicholson (:bnicholson)]

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/

Comment 6

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

Attachment: Change the margins to padding in the browser toolbar.

Fixed nits.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/3e2b3f5a87e2

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e2b3f5a87e2