Closed Bug 965455 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: rnewman, Assigned: Margaret)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
That's what I was going to say!
Assignee: nobody → margaret.leibovic
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.
Attachment #8367757 - Flags: review?(wjohnston)
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.
Attachment #8367762 - Flags: review?(wjohnston)
Attachment #8367757 - Flags: review?(wjohnston) → review+
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...
(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.
Flags: needinfo?(ibarlow)
Comment on attachment 8367762 [details] [diff] [review]
Set parent id when opening a new tab from app menu

Clearing request until ibarlow responds :)
Attachment #8367762 - Flags: review?(wjohnston)
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?
Flags: needinfo?(ibarlow)
(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.
Flags: needinfo?(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...
Flags: needinfo?(ibarlow)
Cool, updating the summary to reflect that this bug is just about the snippet.
Summary: addTab doesn't extend the back stack → Opening a link from snippet doesn't set parent tab correctly
Attachment #8367762 - Attachment is obsolete: true
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
Attachment #8367757 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/140bfa99d5c9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8367757 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.