1217164 - Back button during sync setup/ should return to previous Activity

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

STR:
0) With no account activated
1) Settings -> Sync (sync setup tab opens)
2) Hit back

Expected: User returns to Sync Settings (or browser tab that occurred before Settings)
Actual: Browser closes

In particular, once I finish signing into an account, my temptation is to hit back to return to the previous tab I was working with because there's no button to dismiss sync setup then I get booted from the browser. As such, I've marked this for onboarding because it's an unintuitive experience.

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #0)
> STR:
> 0) With no account activated
> 1) Settings -> Sync (sync setup tab opens)

Is this the web flow, so you really have a tab?

> 2) Hit back
> 
> Expected: User returns to Sync Settings (or browser tab that occurred before
> Settings)
> Actual: Browser closes
> 
> In particular, once I finish signing into an account, my temptation is to
> hit back to return to the previous tab I was working with because there's no
> button to dismiss sync setup then I get booted from the browser. As such,
> I've marked this for onboarding because it's an unintuitive experience.

I think this is because we start a new Intent, like https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/firstrun/WelcomePanel.java#31.  That routes through an intent filter to https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountGetStartedActivityWeb.java, which opens a URL in the browser using https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/activities/ActivityUtils.java#67.  Is it possible the "new task" flag is at fault here?

I find these things very confusing.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that my work in bug 1213490 introduces a flow where the user can open the sync setup screen (when the user clicks the shareplane but doesn't have an account set up) – that should be fixed here.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #1)
> Is this the web flow, so you really have a tab?

Yep.

> I think this is because we start a new Intent, like
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/firstrun/
> WelcomePanel.java#31.  That routes through an intent filter to
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/
> activities/FxAccountGetStartedActivityWeb.java, which opens a URL in the
> browser using
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/
> setup/activities/ActivityUtils.java#67.  Is it possible the "new task" flag
> is at fault here?
> 
> I find these things very confusing.

Since we don't start a new activity, I don't think new task & the like do anything – we would handle back the usual way, with BrowserApp.onBackPressed. We might be able to fix this with a parent tab:
  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?rev=806d80510c21#2386

Perhaps we can add that to the Intent? It'd make for an easy fix for bug 1213490 but harder when we access it via the Settings screen.

Comment 4

[Richard Newman [:rnewman]]

IIRC this is the behavior when LOADURL_EXTERNAL is set, which is the case for new tabs opened from intents -- that is, hitting back takes you back out to the Twitter app.

We should probably not be triggering this via the intent flow; if we are, we should make sure that this isn't handled like a real external intent, because delegating to Android and moving our task to the back of the stack is the wrong thing to do here.

Setting a parent on the Java won't fix this -- we give Gecko a chance to handle the back, then we check whether we're external, and only then do we check for a parent tab.

We could invert those last two checks, under the assumption that 'real' external tabs will never have a parent tab ID.

The real fix for this is to allow BrowserApp to appear multiple times in the activity stack (Bug 964883), so that Back would take you back to Settings.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #4)

> Setting a parent on the Java won't fix this

Apologies: setting a parent on the Java _side_ -- that is, setting Tab.mParentId.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #4)
> IIRC this is the behavior when LOADURL_EXTERNAL is set, which is the case
> for new tabs opened from intents -- that is, hitting back takes you back out
> to the Twitter app.
> 
> We should probably not be triggering this via the intent flow; if we are, we
> should make sure that this isn't handled like a real external intent,
> because delegating to Android and moving our task to the back of the stack
> is the wrong thing to do here.

We need to trigger via an Intent at some level 'cuz this can happen when Gecko isn't running.  (E.g., the Status Activity can send you to accounts.firefox.com.)

Is it possible to tag the incoming Intent as "internal" in some way and route it differently?  To distinguish between "real" external intents and internal ones?

> Setting a parent on the Java won't fix this -- we give Gecko a chance to
> handle the back, then we check whether we're external, and only then do we
> check for a parent tab.
> 
> We could invert those last two checks, under the assumption that 'real'
> external tabs will never have a parent tab ID.
> 
> The real fix for this is to allow BrowserApp to appear multiple times in the
> activity stack (Bug 964883), so that Back would take you back to Settings.

Agree.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #6)

> We need to trigger via an Intent at some level 'cuz this can happen when
> Gecko isn't running.  (E.g., the Status Activity can send you to
> accounts.firefox.com.)

Yeah, I concur that GeckoApp/BrowserApp need to be launched, and the usual Gecko thread mechanics need to happen -- we just shouldn't do that by sending a usual VIEW intent that comes in through the front door.


> Is it possible to tag the incoming Intent as "internal" in some way and
> route it differently?  To distinguish between "real" external intents and
> internal ones?

That's one way to do it -- put an extra on the intent. One needs to make sure it's not something that could have sec or DoS implications if misused, because these intents could come from anywhere.

The other way is to expose a second intent handler, one that's private -- VIEW_WEB_CONTENT, say.

This would give us a trustworthy mechanism for doing things like, say, going back to Settings by re-pushing it onto the activity stack in onBackPressed.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #7)
> > Is it possible to tag the incoming Intent as "internal" in some way and
> > route it differently? ...
> 
> That's one way to do it -- put an extra on the intent. One needs to make
> sure it's not something that could have sec or DoS implications if misused,
> because these intents could come from anywhere.
> 
> The other way is to expose a second intent handler, one that's private --
> VIEW_WEB_CONTENT, say.

Option 2 makes the most sense and isn't much more effort than option 1. It'd also help us avoid problems like bug 1216333 and avoid creating security/spoofing problems in option 1 as Richard mentions.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Just to spell it out, the ACTION_FXA_GET_STARTED Intent gets sent to FxAccountGetStartedActivityWeb, which extends FxAccountWebFlowActivity [1], where it redirects to an ACTION_VIEW Intent in ActivityUtils.openURLInFennec.

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountWebFlowActivity.java?rev=70f459b00b88#54

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also, because this could come in handy later:

14:43 <mcomella> nalexander: Is there a way to have a callback once the user finishes signing into accounts via the new flow?
15:13 <@nalexander> mcomella: such functionality doesn't exist right now, but it could certainly be possible.
15:15 <@nalexander> mcomella: in Accounts.jsm, we could radiate account status changes.
15:15 <@nalexander> mcomella: or we could do it at the Java layer.
15:16 <@nalexander> mcomella: if you looka t https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/authenticator/AndroidFxAccount.java?case=true&from=AndroidFxAccount.java#568, we already radiate state changed.
15:16 <@nalexander> mcomella: I'd be happy to add "account added" and "account removed", to facilitate places in the App reacting.
15:17 <@nalexander> mcomella: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/authenticator/AndroidFxAccount.java?case=true&from=AndroidFxAccount.java#401 is the place for adding.
15:19 <@nalexander> mcomella: looks like we already broadcast removed, in a round-about way.  Making this more robust and tested would be a Good Thing.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(filed bug 1221217 for comment 10)

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I retested the flow for this from the onboarding screen and it's pretty awful – you get stranded on a tab with "Account signed in" and back closes the app rather than taking you back to the tour. When you open the app, you remain stranded. I think going back to top sites would be a better experience.

I've been having difficulty figuring out why back does not return to the previous activity (even though it does for external links) so I might just inject "about:home" in the back history instead (which doesn't really resonate with what "back" means but it's better than it is now).

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #12)
> I retested the flow for this from the onboarding screen and it's pretty
> awful – you get stranded on a tab with "Account signed in" and back closes
> the app rather than taking you back to the tour. When you open the app, you
> remain stranded. I think going back to top sites would be a better
> experience.
> 
> I've been having difficulty figuring out why back does not return to the
> previous activity (even though it does for external links) so I might just
> inject "about:home" in the back history instead (which doesn't really
> resonate with what "back" means but it's better than it is now).

Guess: we're starting a new task?  That was appropriate at times in the past, but probably isn't right everywhere I do it.  Happy to take guidance here.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Not actively working on this. Going to renom to see how much triage wants this.

Comment 15

[:Margaret Leibovic]

I think it would be good to fix the back stack behavior of tabs that open from settings, but this isn't an urgent matter.

Comment 18

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.