907918 - Set parent tab when doing search from selected text

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[microrffr]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Select some text.
Open the context menu for the selection.
Choose Google Search.
Press back.


Actual results:

minimizes


Expected results:

closes tab and returns to the tab that had the selection

Comment 1

[Aaron Train [:aaronmt]]

Can't recall if this is expected behaviour or intentional; but I can reproduce. CC'ing Brian.

Comment 2

[Brian Nicholson (:bnicholson)]

Interesting, I didn't even know this feature existed. CC'ing Chris since he added it.

Comment 3

[microrffr]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Can't recall if this is expected behaviour or intentional; but I can
> reproduce. CC'ing Brian.

It kind of happens in desktop Firefox. If you close the search tab, it goes back to the tab that had the selection, instead of the next tab after the search tab.

Comment 4

[:Margaret Leibovic]

This feature was added in 25. We could uplift a fix.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Set parent tab and preserve private-tab status when doing search-with-selection

Ah! I never knew this was a thing.

The old version would also open a non-private tab if you did search-with from inside a private tab. This seems like something we also want to fix, and is included in this patch.

Comment 6

[Chris Kitching [:ckitching]]

Just to ensure I didn't break anything else:
https://tbpl.mozilla.org/?tree=Try&rev=920c56c9edb0

Comment 7

[:Margaret Leibovic]

Comment on attachment 795710 [details] [diff] [review]
Set parent tab and preserve private-tab status when doing search-with-selection

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

Nice catch with the private browsing fix.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +471,5 @@
> +      let parent = BrowserApp.selectedTab;
> +      let isPrivate = PrivateBrowsingUtils.isWindowPrivate(parent.browser.contentWindow);
> +      // Set current tab as parent of new tab, and set new tab as private if the parent is.
> +      BrowserApp.addTab(req.uri.spec, {parentId: parent.id,
> +                                       selected: true,

New tabs default to being selected, but adding this line is nice for making the behavior more explicit.

Comment 8

[:Margaret Leibovic]

Chris, request approval to uplift this to aurora.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6247e1eb8406

Comment 10

[Chris Kitching [:ckitching]]

Comment on attachment 795710 [details] [diff] [review]
Set parent tab and preserve private-tab status when doing search-with-selection

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature of 828254.
User impact if declined: Inconsistency of tab-parenting behaviour over search-with-selection contextual menu item between mobile and desktop.
Testing completed (on m-c, etc.): Landed on fx-team, successful try run completed: https://tbpl.mozilla.org/?tree=Try&rev=920c56c9edb0
Risk to taking this patch (and alternatives if risky): Low risk - change essentially swaps one call into long-established code to spawn a new tab with another, slightly different one. Given the absence of syntax errors and other obvious screwups, for this patch to introduce a regression would likely require the existence of a long-standing thus far undetected bug in the underlying API. This seems unlikely.
String or IDL/UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6247e1eb8406

Comment 12

[Chris Kitching [:ckitching]]

Note:
https://bugzilla.mozilla.org/show_bug.cgi?id=905340
Is the reason the XPCShell test on Android 4 on the linked try run is failing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f533e1cc7a8

Comment 14

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0b1 (2013-09-18)
Device: LG Nexus 4
OS: Android 4.2.2

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 26.0a2 (2013-09-18)
Device: Samsung Galaxy R
OS: Android 2.3