Closed Bug 827208 Opened 7 years ago Closed 7 years ago

Tabs tray button hit area is too wide

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 22
Tracking Status
firefox19 --- unaffected
firefox20 + verified
firefox21 + verified
fennec 20+ ---

People

(Reporter: arky, Assigned: lucasr)

References

Details

(Keywords: regression)

Attachments

(2 files)

User often press tabs icon when they reach for Reader mode icon.

Capture the video of this behavior here:    http://youtu.be/J1lUV2CY4J0
I wonder if this might be related to bug 817828. When bug 817828 happens on my phone, tapping the black area opens the tabs tray, so the same hit area may be broken here (even though the black invalidation bug isn't visible in the video). I'm able to reproduce this hit area bug on Nightly, but not Beta.
Or rather, this was likely caused by bug 709433, which is responsible for bug 817828.
Blocks: 709433
Keywords: regression
Summary: Reader Mode Icon is too close to tabs icon → Tabs tray button hit area is too wide
tracking-fennec: --- → ?
This is a regression cause by the new view stacking order in the toolbar (implemented in bug 709433). Bug 817828 is unrelated though (it's likely caused by a bug the ShapedButton's drawing code).

The toolbar animation relies on the new stacking order. Need to think of a way to reduce the hit area of the tabs tray button.
Component: Reader Mode → General
Assignee: nobody → lucasr.at.mozilla
Sriram - Did we remove the click delegation code that fixed this before? We might have.
tracking-fennec: ? → 20+
Flags: needinfo?(sriram)
This might be related to stacking order as Lucas mentioned in comment 4. May be we can delegate the left half of the tail of tabs button to go to the url-bar, which will give it to whatever is there.
Flags: needinfo?(sriram)
This makes it very hard to hit the reader mode button.
Attached patch PatchSplinter Review
We are awesome! because we were the first to use TouchDelegate in Android and file an Android bug on it! :P

We are more awesome! because we are using 2 touch delegates on the same view that Android doesn't support! :P :P

Basically setTouchDelegate() can have only one touch delegate. So, we internally hold a list of TouchDelegates inside something that extends TouchDelegate! :P :P :P

(I promise that I passed Object-Oriented Programming class :( )
Attachment #719205 - Flags: review?(bnicholson)
Attachment #719205 - Flags: review?(bnicholson) → review+
Attached patch Patch 2: FixSplinter Review
Only the delegate that consumed ACTION_DOWN should consume the subsequent events. Now that there might be 2 or more delegates, ACTION_UP might be consumed by someone causing a stale state to persist in the delegated view.
Attachment #719353 - Flags: review?(bnicholson)
Attachment #719353 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/e2b69993b3f4
https://hg.mozilla.org/mozilla-central/rev/c0a0c8227fb2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
It's probably worth uplifting this one to aurora/beta btw.
Sounds like this has been around since we shipped Reader Mode, so no need to track but feel free to nominate the patch(es) for uplift if low risk enough.
(In reply to Lukas Blakk [:lsblakk] from comment #14)
> Sounds like this has been around since we shipped Reader Mode, so no need to
> track but feel free to nominate the patch(es) for uplift if low risk enough.

This is a regression from bug 709433 (Fx20). Any chance this can still make beta?
Comment on attachment 719205 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Animating browser-toolbar to awesombar.
User impact if declined: Reader-mode button is hard to hit.
Testing completed (on m-c, etc.): Landed in m-c on 02/28
Risk to taking this patch (and alternatives if risky): Very less. We've been using tail-touch-delegate to make sure Menu button gets a fair share. Menu button was on the right side. Now we are using the same logic on the left side.
String or UUID changes made by this patch: None.
Attachment #719205 - Flags: approval-mozilla-beta?
Attachment #719205 - Flags: approval-mozilla-aurora?
Comment on attachment 719353 [details] [diff] [review]
Patch 2: Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Animating browser-toolbar to awesombar.
User impact if declined: Reader-mode button is hard to hit.
Testing completed (on m-c, etc.): Landed in m-c on 02/28
Risk to taking this patch (and alternatives if risky): Very less. We've been using tail-touch-delegate to make sure Menu button gets a fair share. Menu button was on the right side. Now we are using the same logic on the left side.
String or UUID changes made by this patch: None.
Attachment #719353 - Flags: approval-mozilla-beta?
Attachment #719353 - Flags: approval-mozilla-aurora?
Attachment #719205 - Flags: approval-mozilla-beta?
Attachment #719205 - Flags: approval-mozilla-beta+
Attachment #719205 - Flags: approval-mozilla-aurora?
Attachment #719205 - Flags: approval-mozilla-aurora+
Attachment #719353 - Flags: approval-mozilla-beta?
Attachment #719353 - Flags: approval-mozilla-beta+
Attachment #719353 - Flags: approval-mozilla-aurora?
Attachment #719353 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.