Closed Bug 1142528 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Reader View, defect)

39 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- affected
firefox39 --- affected
firefox40 --- fixed
fennec 38+ ---

People

(Reporter: TeoVermesan, Assigned: vivek)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
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.
Assignee: nobody → margaret.leibovic
Blocks: readerv2, 1120004
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.
tracking-fennec: --- → ?
Margaret - Is this a priority for Fx38?
Flags: needinfo?(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.
tracking-fennec: ? → 38+
Flags: needinfo?(margaret.leibovic) → needinfo?(alam)
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 + / -
Flags: needinfo?(alam)
Attached patch 1142528.patch (obsolete) — Splinter Review
* 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.
Attachment #8583291 - Flags: review?(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.
Attachment #8583291 - Flags: review?(margaret.leibovic) → review+
Assignee: margaret.leibovic → vivekb.balakrishnan
Attached patch 1142528.patchSplinter Review
Patch updated as per the review commment
Attachment #8583291 - Attachment is obsolete: true
Attachment #8584730 - Flags: review?(margaret.leibovic)
Attachment #8584730 - Attachment is patch: true
Attachment #8584730 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/fe3c6243c728
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1150251
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
Attachment #8584730 - Flags: approval-mozilla-beta?
Attachment #8584730 - Flags: approval-mozilla-aurora?
Comment on attachment 8584730 [details] [diff] [review]
1142528.patch

Should be in 38 beta 2
Attachment #8584730 - Flags: approval-mozilla-beta?
Attachment #8584730 - Flags: approval-mozilla-beta+
Attachment #8584730 - Flags: approval-mozilla-aurora?
Attachment #8584730 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(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
Flags: needinfo?(margaret.leibovic)
Attachment #8584730 - Flags: approval-mozilla-beta+
Attachment #8584730 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.