Closed Bug 907918 Opened 6 years ago Closed 6 years ago

Set parent tab when doing search from selected text

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox24 --- unaffected
firefox25 --- verified
firefox26 --- verified
fennec 25+ ---

People

(Reporter: microrffr, Assigned: ckitching)

References

Details

(Keywords: reproducible)

Attachments

(1 file)

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
Can't recall if this is expected behaviour or intentional; but I can reproduce. CC'ing Brian.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: reproducible
OS: Windows 7 → Android
Hardware: x86_64 → ARM
Interesting, I didn't even know this feature existed. CC'ing Chris since he added it.
(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.
This feature was added in 25. We could uplift a fix.
Blocks: 828254
tracking-fennec: --- → ?
Assignee: nobody → ckitching
tracking-fennec: ? → 25+
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.
Attachment #795710 - Flags: review?(margaret.leibovic)
Just to ensure I didn't break anything else:
https://tbpl.mozilla.org/?tree=Try&rev=920c56c9edb0
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.
Attachment #795710 - Flags: review?(margaret.leibovic) → review+
Chris, request approval to uplift this to aurora.
Keywords: checkin-needed
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.
Attachment #795710 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6247e1eb8406
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
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.
Attachment #795710 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 25.0b1 (2013-09-18)
Device: LG Nexus 4
OS: Android 4.2.2
Verified fixed on:
Build: Firefox for Android 26.0a2 (2013-09-18)
Device: Samsung Galaxy R
OS: Android 2.3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.