Open Bug 1217164 Opened 4 years ago Updated Last year

Back button during sync setup/ should return to previous Activity

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

Tracking Status
fennec + ---

People

(Reporter: mcomella, Unassigned)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

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.
(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.
Flags: needinfo?(michael.l.comella)
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.
Depends on: 1213490
(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.
Flags: needinfo?(michael.l.comella)
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.
(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.
(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.
(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.
tracking-fennec: ? → 44+
(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.
Assignee: nobody → michael.l.comella
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
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).
(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.
Not actively working on this. Going to renom to see how much triage wants this.
Assignee: michael.l.comella → nobody
tracking-fennec: 44+ → ?
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.
tracking-fennec: ? → +
Duplicate of this bug: 1238086
Blocks: 1282382
Blocks: 1254585
No longer blocks: 1282382
Duplicate of this bug: 1344343
Blocks: 1398299
Whiteboard: [FxA]
Whiteboard: [FxA]
You need to log in before you can comment on or make changes to this bug.