1142528 - Tappable area is larger than the +/- buttons, overlaping "Aa" button.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Go to an article from news.google.com and enter reader mode
2. Tap the "Aa" layout from the reader mode toolbar
3. Tap on the "Aa" button, located between the "-" and "+" button. 
4. Tap also on the left and right of the button.

Actual results:
- Tapping the "Aa" button increases the text size.
- Tapping near left, will minimize the text size.
- Tapping near right, will minimize the text size.

Expected results:
- Text shouldn't change its size.
- Tappable area should be delimited between buttons

Comment 1

[:Margaret Leibovic]

I noticed this while developing this feature, but I failed to fix it before landing the patch. I think I just need to swap #font-size-sample from a button to a span.

Comment 2

[:Margaret Leibovic]

Looking into this a bit more closely, the actual issue is that I added negative margins to the "Aa" element so that the -/+ buttons would align more closely with the center. It's not immediately clear to me how to best fix this issue.

Comment 3

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

Margaret - Is this a priority for Fx38?

Comment 4

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Margaret - Is this a priority for Fx38?

If this is really a serious issue, a quick fix would just be to remove that negative margin, but then the -/+ buttons will be farther from the center.

I don't think this is a huge problem, but I can let antlam make the call.

Let's track for 38 and see if we can get a fix.

Comment 5

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

Since this fix isn't as straight forward as I thought, I wouldn't say that it's blocking. 

Talked to margaret about this a bit on IRC, hypothesis is adding a button class (that links to nowhere) for the Aa might take users's presses/input and not let them hit the + / -

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1142528.patch

* Increased the size of Aa button. 
* Added margin-left and margin-right to - and + buttons respectively to align them with the center.

The screenshots of the changes are available here. 
https://www.dropbox.com/sh/eaqns04dakr09sb/AABw3N-_wi40H48_Plwfq4W0a?dl=0

old*.png are screenshots of the current implementation while new*.png are for the patch.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8583291 [details] [diff] [review]
1142528.patch

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

Nice! I'm embarrassed that I didn't come up with this simple solution myself :)

I just think we should move these rules into some existing selectors we have, but then we should definitely land this!

::: mobile/android/themes/core/aboutReader.css
@@ +407,5 @@
> +  margin-left: 60px;
> +}
> +
> +#font-size-plus {
> +  margin-right: 60px;

We already have styles for .minus-button and .plus-button, so let's just put these in there.

Comment 8

[Vivek Balakrishnan[:vivek]]

Attachment: 1142528.patch

Patch updated as per the review commment

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe3c6243c728

Comment 11

[:Margaret Leibovic]

Comment on attachment 8584730 [details] [diff] [review]
1142528.patch

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: tapping decorative "Aa" in font style panel will toggle font sizes
[Describe test coverage new/current, TreeHerder]: landed on nightly
[Risks and why]: low-risk style change
[String/UUID change made/needed]: none

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8584730 [details] [diff] [review]
1142528.patch

Should be in 38 beta 2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c1c458a4108

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d883361e554

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Nexus 5 (Android 5.0) and Nexus 7 (Android 5.0)
Builds: Firefox for Android 38 Beta 2 and latest Nightly

Is this issue fixed?
If I tap on the space between the "-" and "Aa" button or "Aa" and "+" button will change the text size.

Comment 16

[:Margaret Leibovic]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #15)
> Tested with:
> Device: Nexus 5 (Android 5.0) and Nexus 7 (Android 5.0)
> Builds: Firefox for Android 38 Beta 2 and latest Nightly
> 
> Is this issue fixed?
> If I tap on the space between the "-" and "Aa" button or "Aa" and "+" button
> will change the text size.

Yes, the tap area for the buttons includes the space around the -/+.

I backed this out of aurora/beta for causing bug 1150251, since I think this issue is less severe, and I would rather ship with this than with bug 1150251. We can look into uplifting both if we get a fix for bug 1150251.

https://hg.mozilla.org/releases/mozilla-aurora/rev/0304d5ce841c
https://hg.mozilla.org/releases/mozilla-beta/rev/ff91cb79a7c8