Closed Bug 766858 Opened 12 years ago Closed 12 years ago

Tap on content area does not close the tabs menu on about:home


(Firefox for Android Graveyard :: General, defect)

15 Branch
Not set


(firefox15 verified, firefox16 verified, fennec15+)

Firefox 17
Tracking Status
firefox15 --- verified
firefox16 --- verified
fennec 15+ ---


(Reporter: pretzer, Assigned: mbrubeck)




(1 file)

Bug 763726 implemented a closing mechanism for the tabs menu when the content area is clicked. This does not seem to work for about:home. I tested other about: pages and they work as expected.
tracking-fennec: --- → ?
Assignee: nobody → mbrubeck
Version: unspecified → Firefox 15
Attached patch patchSplinter Review
This patch fixes both this bug and bug 775630 at the same time, since the solutions ended up sharing most of their code.  In both cases, we need a way to intercept touch events so that we can consume them before they are passed to the child views (for about:home) or the PanZoomController (for web content).
Attachment #644041 - Flags: review?(bugmail.mozilla)
Blocks: 775630
Comment on attachment 644041 [details] [diff] [review]

Review of attachment 644041 [details] [diff] [review]:

r+ with comments addressed.

::: mobile/android/base/
@@ +359,5 @@
> +    public boolean onInterceptTouchEvent(MotionEvent event) {
> +        if (mOnInterceptTouchListener.onInterceptTouchEvent(this, event))
> +            return true;
> +        return super.onInterceptTouchEvent(event);
> +    }

I assume you tested this code reasonably well. The docs for View.onInterceptTouchEvent sound scary.

::: mobile/android/base/
@@ +2845,3 @@
>              private PointF initialPoint = null;
>              public boolean onTouch(View view, MotionEvent event) {
>                  if (event == null)

Mark this onTouch method as @Override

@@ +2888,5 @@
> +        @Override
> +        public boolean onTouch(View view, MotionEvent event) {
> +            if (mIsHidingTabs) {
> +                // Keep consuming events until the gesture finishes.
> +                if (event.getActionMasked() == MotionEvent.ACTION_UP)

Also check for MotionEvent.ACTION_CANCEL

::: mobile/android/base/gfx/
@@ +11,5 @@
>  import android.os.SystemClock;
>  import android.util.Log;
>  import android.view.GestureDetector;
>  import android.view.MotionEvent;
>  import android.view.View.OnTouchListener;

You can take out the View.OnTouchListener import

@@ +149,5 @@
>              return true;
>          }
> +        if (mOnTouchListener.onInterceptTouchEvent(mView, event))
> +            return true;

Add braces
Attachment #644041 - Flags: review?(bugmail.mozilla) → review+
tracking-fennec: ? → 15+
(In reply to Kartikaya Gupta (:kats) from comment #4)
> I assume you tested this code reasonably well. The docs for
> View.onInterceptTouchEvent sound scary.

Yes, I read those docs carefully and made sure to try all branches of this code.  Fortunately onInterceptTouchEvent, while a bit complicated, seems designed exactly for this scenario of intercepting one whole gesture at a time.

Pushed with fixes for all review comments:

I'll request uplift after this has baked a few days.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Depends on: 776253
Comment on attachment 644041 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Polish bug in new tab menu (bug 739407)

User impact if declined: One of the gestures to close the tab menu does not work when about:home is visible.  Closing the tab menu may scroll content unexpectedly.

Testing completed (on m-c, etc.): On m-c since 7/20.  One regression found and fixed (bug 776253, approval requested).

Risk to taking this patch (and alternatives if risky): Android-only.  This patch adds a small amount of new code that is moderately complex.  I think it has baked long enough to find all critical regressions, and the usability gain is worth the small risk from uplift.

String or UUID changes made by this patch: None.
Attachment #644041 - Flags: approval-mozilla-beta?
Attachment #644041 - Flags: approval-mozilla-aurora?
Paul, please add a smoke-test
Flags: in-moztrap?(paul.feher)
Comment on attachment 644041 [details] [diff] [review]

just approved bug 776253, let's get this in and get users on it so we can make sure there aren't any other regressions before release.
Attachment #644041 - Flags: approval-mozilla-beta?
Attachment #644041 - Flags: approval-mozilla-beta+
Attachment #644041 - Flags: approval-mozilla-aurora?
Attachment #644041 - Flags: approval-mozilla-aurora+
Tab menu is closed when tapping anywhere on any about: page on the latest Firefox 16 Beta 5.
Test case created and added to Smoke-test suite:
Flags: in-moztrap?(paul.feher) → in-moztrap+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.