Closed Bug 1304688 Opened 3 years ago Closed 3 years ago

Long Press of Android System Back button does not show lists on Android 7.0 Nougat

Categories

(Firefox for Android :: General, defect, P1)

49 Branch
All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Iteration:
1.5
Tracking Status
firefox49 --- wontfix
fennec 50+ ---
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: rigarash, Assigned: ahunt)

References

(Regression)

Details

(Whiteboard: [MobileAS])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Android 7.0; Mobile; rv:49.0) Gecko/49.0 Firefox/49.0
Build ID: 20160916104719

Steps to reproduce:

When I upgrade my Nexus 6P to Android 7.0 Nougat, long pressing Android system 'back' button does not result in showing history of the tab after browsing a few web pages.
When I did long pressing 'back' icon available on firefox menu on the top-left, it worked as expected even on Android 7.0 Nougat.

When on Android 6.0 Marshmallow, 


Actual results:

Long pressing Android system 'back' button results in nothing.


Expected results:

Long pressing Android system 'back' button results in showing history of the tab.
I can reproduce this on a Nexus 6P running Android 7.0. This is not listed in the behavior changes of Android 7.0 [1]. Ahunt, can you investigate?

I guess we are handling this here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3835

[1] https://developer.android.com/about/versions/nougat/android-7.0-changes.html
Flags: needinfo?(ahunt)
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Unspecified → Android
Priority: -- → P1
Hardware: Unspecified → All
Assignee: nobody → ahunt
Flags: needinfo?(ahunt)
We don't seem to be receiving onKeyLongPress at all - I'll have a dig in the Android sources to see if something has changed to confirm that this is in fact an Android change (which seems likely seeing as this appeared with 7.0).

I think we could probably use onKeyUp() with event.getDownTime() instead, so this isn't too catastrophic.
(In reply to Andrzej Hunt :ahunt from comment #2)
> We don't seem to be receiving onKeyLongPress at all - I'll have a dig in the
> Android sources to see if something has changed to confirm that this is in
> fact an Android change (which seems likely seeing as this appeared with 7.0).
> 
> I think we could probably use onKeyUp() with event.getDownTime() instead, so
> this isn't too catastrophic.

Android bug report at:
https://code.google.com/p/android/issues/detail?id=213738
(In reply to Andrzej Hunt :ahunt from comment #3)
> Android bug report at:
> https://code.google.com/p/android/issues/detail?id=213738

Argh.

(In reply to Andrzej Hunt :ahunt from comment #2)
> I think we could probably use onKeyUp() with event.getDownTime() instead, so
> this isn't too catastrophic.

Do we get that for back?
(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> 
> Do we get that for back?

Fortunately yes (at least on my N6P).


I did dig into this yesterday:

TL;DR version: yes, back button handling has changed, so no apps will receive long-press events.

Long version:

It seems Android has changed the way long-presses are handled for system buttons: this looks like an unintentional oversight (possibly related to support for having multiple navigation bars on screen? I have no idea...).

The button-handling is in KeyButtonView, which AFAICT hasn't changed significantly:
http://androidxref.com/7.0.0_r1/xref/frameworks/base/packages/SystemUI/src/com/android/systemui/statusbar/policy/KeyButtonView.java

The key issue is at line 64: previously isLongClickable() was false (this is a property of View), so the second branch of the if/else is run, and a LONG_PRESS is emitted. This seems to not be broken.

In 7.0, some code was added to add a longClickListener to the back button, which causes KeyButtonView to no longer emit the long-press even (isLongClickable() returns true once there's a longClickListener).

Then I discovered that there's a new listener which always receives the long-press, but that this is only used for a "lock task mode" in the (also new) PhoneStatusBar:
http://androidxref.com/7.0.0_r1/xref/frameworks/base/packages/SystemUI/src/com/android/systemui/statusbar/phone/PhoneStatusBar.java#handleLongPressBack

This lock task mode doesn't seem to be new, but the way the back button is handled is new (I'm guessing in 6.0 and earlier, either the long-press to exit lock-task either didn't exist, or the long-press listener was added only if lock-task mode is running - whereas now the listener is always added).

More on lock-task:
https://developer.android.com/work/cosu.html
Whiteboard: [MobileAS]
Status: NEW → ASSIGNED
Comment on attachment 8795374 [details]
Bug 1304688 - Pre: add Versions.preN to AppConstants

https://reviewboard.mozilla.org/r/81444/#review80776
Attachment #8795374 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795375 [details]
Bug 1304688 - Implement long-press timer to workaround broken onKeyLongPress on Android N

https://reviewboard.mozilla.org/r/81446/#review80960

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:551
(Diff revision 1)
> +    private Runnable mCheckLongPress;
> +    {
> +        if (!Versions.preN) {
> +            mCheckLongPress = new Runnable() {
> +                public void run() {
> +                    handleBackLongPress();
> +                }
> +            };
> +        }
> +    }

This syntax is a bit confusing at first. :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3863
(Diff revision 1)
>      /**
> -     * This will detect if the key pressed is back. If so, will show the history.
> +     * Handle a long press on the back button
> +     * @return <code>null</code> if the press wasn't handled, otherwise a Boolean corresponding to
> +     *                           the return value of KeyEvent.Callback.onKeyLongPress
>       */
> -    @Override
> +    private Boolean handleBackLongPress() {

Do we really need a Boolean object here? Shouldn't be true/false for handled / not handled be enough?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3867
(Diff revision 1)
> -    public boolean onKeyLongPress(int keyCode, KeyEvent event) {
> -        if (keyCode == KeyEvent.KEYCODE_BACK) {
> -            // If the tab search history is already shown, do nothing.
> +        // If the tab search history is already shown, do nothing.
> -            TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
> +        TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
> -            if (frag != null) {
> +        if (frag != null) {
> -                return false;
> +            return true;

Why is this now 'true' instead of 'false'?
Attachment #8795375 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> 
> Do we really need a Boolean object here? Shouldn't be true/false for handled
> / not handled be enough?

Yup, makes sense. I got confused by the way the original code work, and ended up overcomplicating things...

> 
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3867
> (Diff revision 1)
> > -    public boolean onKeyLongPress(int keyCode, KeyEvent event) {
> > -        if (keyCode == KeyEvent.KEYCODE_BACK) {
> > -            // If the tab search history is already shown, do nothing.
> > +        // If the tab search history is already shown, do nothing.
> > -            TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
> > +        TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
> > -            if (frag != null) {
> > +        if (frag != null) {
> > -                return false;
> > +            return true;
> 
> Why is this now 'true' instead of 'false'?

We're supposed to return true if we've handled the event - in this case we specifically seem to want to ignore the long-press since we're showing the history list. It seemed more sane to return true (we've handled the event by ignoring it), instead of false (we've ignored the event by not handling it, but anyone else could handle it). Either way, I don't think this has any real effect since no one else handles the long-press, maybe I should just leave the code as is if it's already working.
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11cf2ce8a9bd
Pre: add Versions.preN to AppConstants r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8d8964596de0
Implement long-press timer to workaround broken onKeyLongPress on Android N r=sebastian
https://hg.mozilla.org/mozilla-central/rev/11cf2ce8a9bd
https://hg.mozilla.org/mozilla-central/rev/8d8964596de0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
After visiting a few pages, long tapping on the android back button, will display a history list, so:
Verified as fixed using:
Device: Nexus 6P (Android 7.0)
Build: Firefox for Android 52.0a1 (2016-10-02)
Comment on attachment 8795374 [details]
Bug 1304688 - Pre: add Versions.preN to AppConstants

Approval Request Comment
[Feature/regressing bug #]: Android Bug 213738 - https://code.google.com/p/android/issues/detail?id=213738
[User impact if declined]: Can't view history by long-pressing back button on  devices running Android 7.
[Describe test coverage new/current, TreeHerder]: manual testing, tested on nightly.
[Risks and why]: This patch: low-risk, only adds a version constant. (Second patch: low to medium risk: we've added a timer that is only run in limited circumstances, this code is based on Android's own long-press code that is already well tested.)
[String/UUID change made/needed]: none.
Attachment #8795374 - Flags: approval-mozilla-beta?
Attachment #8795374 - Flags: approval-mozilla-aurora?
Comment on attachment 8795375 [details]
Bug 1304688 - Implement long-press timer to workaround broken onKeyLongPress on Android N

Approval Request Comment
[Feature/regressing bug #]: Android Bug 213738 - https://code.google.com/p/android/issues/detail?id=213738
[User impact if declined]: Can't view history by long-pressing back button on  devices running Android 7.
[Describe test coverage new/current, TreeHerder]: manual testing, tested on nightly.
[Risks and why]: This patch: low to medium risk: we've added a timer that is only run in limited circumstances, this code is based on Android's own long-press code that is already well tested. (First patch: low-risk, only adds a version constant.)
[String/UUID change made/needed]: none.
Attachment #8795375 - Flags: approval-mozilla-beta?
Attachment #8795375 - Flags: approval-mozilla-aurora?
Comment on attachment 8795374 [details]
Bug 1304688 - Pre: add Versions.preN to AppConstants

Fix was verified on Nightly52, Aurora51+, Beta50+
Attachment #8795374 - Flags: approval-mozilla-beta?
Attachment #8795374 - Flags: approval-mozilla-beta+
Attachment #8795374 - Flags: approval-mozilla-aurora?
Attachment #8795374 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Attachment #8795375 - Flags: approval-mozilla-beta?
Attachment #8795375 - Flags: approval-mozilla-beta+
Attachment #8795375 - Flags: approval-mozilla-aurora?
Attachment #8795375 - Flags: approval-mozilla-aurora+
Verified as fixed on Nexus 6P (Android 7.0) on Fennec 50 Beta 4 and latest Aurora.
tracking-fennec: ? → 50+
Iteration: --- → 1.5
My device has just updated from android 6 to Nougat 7.0 and Im running Firefox 56.0.  This problem now exists for me as reported.  Maybe this patch never made it to v56?  Is there a way of raising this again or is this comment sufficient to raise the profile/review?
Regressed by: 1527666
You need to log in before you can comment on or make changes to this bug.