965455 - Opening a link from snippet doesn't set parent tab correctly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

This is visible when creating a new tab, and also when tapping a snippet (which calls BrowserApp.addTab).

* Tap snippet.
* New tab opens with snippet contents.
* Hit back.

Expected: return to previous screen.
Actual: dumped into the launcher.

Tab change events need to be registered in the back handling that GeckoApp implements in onBackPressed.

Comment 1

[Wesley Johnston (:wesj)]

Tabs don't use the normal android back stack. We likely just want to make sure this tab has a parent:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#710

so that hitting back will take you back to the parent tab.

Comment 2

[:Margaret Leibovic]

That's what I was going to say!

Comment 3

[:Margaret Leibovic]

Attachment: Set parent id when adding a new tab from snippet

This fixes the issue for tapping on snippets.

There's a separate, similar issue with opening a new tab from the app menu. I think that hitting back in that case should also close the tab and go back to the parent tab, but I can/should do that in a separate patch.

Comment 4

[:Margaret Leibovic]

Attachment: Set parent id when opening a new tab from app menu

I'm less confident about this patch. I was considering just changing the logic in loadUrl to default to setting the parent id as the selected tab, but that seems like it could break other consumers, such as when you open a link from an external app. Right now the consumers of addTab/addPrivateTab are just the app menu items (and a keyboard shortcut), so I feel like this is fine, but possible future consumers will need to be aware of this behavior.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8367762 [details] [diff] [review]
Set parent id when opening a new tab from app menu

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

These are fired when you hit the newTab button (or basically when you do anything in java to create a tab), right? I don't think we want to always set a parent...

Comment 6

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #5)
> Comment on attachment 8367762 [details] [diff] [review]
> Set parent id when opening a new tab from app menu
> 
> Review of attachment 8367762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These are fired when you hit the newTab button (or basically when you do
> anything in java to create a tab), right? I don't think we want to always
> set a parent...

Yes, these are fired when you hit the new tab button. These methods are what we use to create new tabs directly from user actions, meaning the menu, or the button in the tabs panel:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsPanel.java#122

I also just found some more unused code :)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java#591

I wonder what ibarlow thinks about this... I can see that setting a parent when you open a new tab from the button in the tabs UI might not make sense, but the app menu feels like it's tied to the current page.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8367762 [details] [diff] [review]
Set parent id when opening a new tab from app menu

Clearing request until ibarlow responds :)

Comment 8

[Ian Barlow (:ibarlow)]

I'm actually not sure about how I feel here. I agree with Richard's original comment that for a snippet that launches a new tab, hitting back should close the tab that you opened. I am less confident about having that interaction be part of the "new tab" control in the menu. It might be nice, but it also might make users feel "stuck" in the browser. 

Got a build I could try?

Comment 9

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #8)
> I'm actually not sure about how I feel here. I agree with Richard's original
> comment that for a snippet that launches a new tab, hitting back should
> close the tab that you opened. I am less confident about having that
> interaction be part of the "new tab" control in the menu. It might be nice,
> but it also might make users feel "stuck" in the browser. 
> 
> Got a build I could try?

http://people.mozilla.org/~mleibovic/backstack.apk

Back over to ibarlow's court. I don't feel very strongly about the new tab button, sorry to scope creep this bug :) Unless you feel like this is an improvement, I can just land the first snippets-only patch.

Comment 10

[Ian Barlow (:ibarlow)]

Hm. I don't have a strong feeling either, having used this. Given that, I might just do the snippet part, and not change the other stuff just for the sake of changing it...

Comment 11

[:Margaret Leibovic]

Cool, updating the summary to reflect that this bug is just about the snippet.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/140bfa99d5c9

Comment 13

[:Margaret Leibovic]

Comment on attachment 8367757 [details] [diff] [review]
Set parent id when adding a new tab from snippet

[Approval Request Comment]
Bug caused by (feature/regressing bug #): snippets were added in bug 937820 (fx28), but we enabled them for fx29
User impact if declined: hitting the back button after opening a snippet link will exit the browser
Testing completed (on m-c, etc.): tested locally
Risk to taking this patch (and alternatives if risky): low-risk, only affects snippets (which don't always appear)
String or IDL/UUID changes made by this patch: none

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/140bfa99d5c9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01a65675640a