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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Peter Retzer (:pretzer)]

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

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).

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 644041 [details] [diff] [review]
patch

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

r+ with comments addressed.

::: mobile/android/base/AboutHomeContent.java
@@ +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/GeckoApp.java
@@ +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/TouchEventHandler.java
@@ +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

Comment 5

[Matt Brubeck (:mbrubeck)]

(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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f37b38a34a

I'll request uplift after this has baked a few days.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45f37b38a34a

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 644041 [details] [diff] [review]
patch

[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.

Comment 9

[Aaron Train [:aaronmt]]

Paul, please add a smoke-test

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644041 [details] [diff] [review]
patch

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.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/031afbc61298
https://hg.mozilla.org/releases/mozilla-beta/rev/14eff47aa8c2

Comment 12

[Cristian Nicolae (:xti)]

Tab menu is closed when tapping anywhere on any about: page on the latest Firefox 16 Beta 5.

Comment 13

[Paul Feher]

Test case created and added to Smoke-test suite:
https://moztrap.mozilla.org/manage/cases/_detail/8357/