Closed Bug 937253 Opened 6 years ago Closed 5 years ago

Android home screen shortcuts should switch-to-tab if the page is already open

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox37 --- verified
relnote-firefox --- 37+

People

(Reporter: jaws, Assigned: mfinkle, Mentored)

References

(Depends on 1 open bug)

Details

(Keywords: reproducible, Whiteboard: [lang=java])

Attachments

(2 files, 1 obsolete file)

STR:
Bookmark a page and add it to the Home Screen
Click on bookmark on Home Screen
Note the current tab count
Go back to the home screen
Click on same bookmark on Home Screen

ER:
The tab count remains the same and the selected tab is the previously opened bookmarked page. It could be reloaded or just shown as usual.

AR:
A new tab is created at the URL of the bookmarked page.

(Tested with Firefox Nightly 11/11/2013 on LG Nexus 5)
Keywords: reproducible
This will be covered by bug 933811. We need design for the switch-to-tab functionality in about:home's thumbnails.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 933811
I thought this bug was about the "Add to home screen" functionality, like the shortcuts we create on your Android launch screen. Bug 933811 is just about the about:home thumbnails.
(In reply to :Margaret Leibovic from comment #2)
> I thought this bug was about the "Add to home screen" functionality, like
> the shortcuts we create on your Android launch screen. Bug 933811 is just
> about the about:home thumbnails.

This is correct. I am talking about the "Add to home screen", as in the Android home screen, not the Firefox home screen. Should this bug be reopened?
(In reply to Jared Wein [:jaws] from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> > I thought this bug was about the "Add to home screen" functionality, like
> > the shortcuts we create on your Android launch screen. Bug 933811 is just
> > about the about:home thumbnails.
> 
> This is correct. I am talking about the "Add to home screen", as in the
> Android home screen, not the Firefox home screen. Should this bug be
> reopened?

Yep.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Bookmarks opened from the Home Screen should "switch to tab" if the bookmarked page is already open → Bookmarks opened from the Android Home Screen should "switch to tab" if the bookmarked page is already open
Status: REOPENED → NEW
Can this be a mentor bug?
(In reply to Aaron Train [:aaronmt] from comment #5)
> Can this be a mentor bug?

I think so.
Whiteboard: [mentor=lucasr][lang=java]
Duplicate of this bug: 945244
Attached patch bug937253v1.patch (obsolete) — Splinter Review
Hi,

I'd like to know what you think about this patch. I wasn't positive about where the logic for this should go, but from reading through the code GeckoApp.java seemed like the best place.

Phil
Attachment #8358405 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8358405 [details] [diff] [review]
bug937253v1.patch

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

Hi Phil, first of all, thanks for the patch. Your patch changes the code for when you add a page to Android's home screen. But this bug is about when you tap on the bookmark in Android's home screen. Here some extra information to help you find your way in the code that needs to be changed:

When you tap on a bookmark that was added to Android's home screen, we'll use a special type of intent (ACTION_BOOKMARK) that is defined when you add the shortcut:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#809

So, when you tap on a shortcut from Android's home screen, we'll handle it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1911

Which will make Gecko use a special -bookmark flag when loading the URL:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#673

Which will then be used by gecko to figure out how to load the page in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#69

But note how we don't really do anything with the bookmark flag in there. The actual call that triggers the page load is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#94

Note how we unconditionally use OPEN_NEWTAB in there.

A very first thing to try is to:
- Check if the -bookmark flag was passed around here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#69
- If the -bookmark flag was passed, use OPEN_SWITCHTAB (instead of OPEN_NEWTAB)

For more context, these flags are defined here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2624

And used in this _getBrowser() method here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2610

There's some old cruft there that uses appOrigin to handle OPEN_SWITCHTAB but we should just match based on tab's URL instead:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2615

I realize this is a lot to grasp at once :-) Ping us on IRC if you have any questions :-)
Attachment #8358405 - Flags: review?(lucasr.at.mozilla)
Mentor: lucasr.at.mozilla
Whiteboard: [mentor=lucasr][lang=java] → [lang=java]
Curiously, this is not an issue on new tablet.

NI to investigate and maybe mentor.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Summary: Bookmarks opened from the Android Home Screen should "switch to tab" if the bookmarked page is already open → Bookmarks opened from about:home should "switch to tab" if the bookmarked page is already open on phones
Flags: needinfo?(michael.l.comella)
Curiously, for the default bookmarks, this appears to work for "Firefox Marketplace" on all devices, but not for "Firefox: Customize with add-ons".

This sounds like it requires some digging but I'm definitely not knowledgeable enough on this part of the code to mentor.

Margaret, can you take a look and mentor if possible?

Also, I wonder how relevant comment 2 still is - can you speak to that?
Flags: needinfo?(michael.l.comella) → needinfo?(margaret.leibovic)
Summary: Bookmarks opened from about:home should "switch to tab" if the bookmarked page is already open on phones → about:home bookmarks don't always show "switch to tab" if the page is already open
(In reply to Michael Comella (:mcomella) from comment #11)

Please don't change the direction of this bug. As comment #3 states, this is not for about:home, but the Android home screen. I think you're looking for bug 933811.
Summary: about:home bookmarks don't always show "switch to tab" if the page is already open → Bookmarks opened from the Android Home Screen should "switch to tab" if the bookmarked page is already open
Let's avoid further confusion in the bug summary -- after all, home screen shortcuts don't need to be bookmarks.


(In reply to Michael Comella (:mcomella) from comment #11)

> Curiously, for the default bookmarks, this appears to work for "Firefox
> Marketplace" on all devices, but not for "Firefox: Customize with add-ons".

That's because the page redirects from the bookmark:

  https://addons.mozilla.org/android/

to the locale-specific page:

  https://addons.mozilla.org/en-US/android/

so from Fennec's perspective it's not the same page, and it won't offer switch-to-tab.

The same will be true in some circumstances for home screen shortcuts, and anywhere else we might offer switch-to-tab.
See Also: → 933811
Summary: Bookmarks opened from the Android Home Screen should "switch to tab" if the bookmarked page is already open → Home screen shortcuts should switch-to-tab if the page is already open
We don't even offer switch-to-tab for home screen shortcuts, which is why this bug was filed.

Most of lucasr's mxr links from comment 9 are out of date, but I don't think this logic has changed, so his general advice should still remain true.
Flags: needinfo?(margaret.leibovic)
Mentor: lucasr.at.mozilla → margaret.leibovic
Summary: Home screen shortcuts should switch-to-tab if the page is already open → Android home screen shortcuts should switch-to-tab if the page is already open
I started looking into this bug. It was annoying me too much to let it alone.

I added the concept of "pinned" sites [1] back in the old days when WebApps were just Homescreen shortcuts, like Bookmarks. WebApps went an entirely different (better) direction with Native APK wrappers and never use "pinned" anymore.

Most of the "pinned" code paths are still in place and almost work just fine. I will add some patches to:
1. Remove some vestiges of the old WebApps-as-shortcuts code
2. Updated the "pinned" code to work for Bookmarks and handle the new Tab stubs
Assignee: nobody → mark.finkle
(In reply to Mark Finkle (:mfinkle) from comment #15)

> I added the concept of "pinned" sites [1] back in the old days when WebApps

Where [1] is:
http://hg.mozilla.org/mozilla-central/rev/2ce1f809b765
This patch removes some old code that handled the original WEBAPP intent coming in from a homescreen shortcut. The new WebApp approach never uses this code path, I verified.

Also note that "pinned" doesn't need to be sent to browser.js startup anymore. Tab stubs are used, which mean the startup code never knows the URL being opened anymore.
Attachment #8358405 - Attachment is obsolete: true
Attachment #8543355 - Flags: review?(wjohnston)
This patch makes sure the "pinned" idea works for HOMESCREEN_SHORTCUT intents (which used to be BOOKMARK intents). The only real code change here is updating loadStartupTab to take flags. We need to be able to tell Gecko whether or not the External URL is supposed to be pinned or not.
Attachment #8543356 - Flags: review?(bnicholson)
Comment on attachment 8543356 [details] [diff] [review]
make-pinned-work v0.1

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

::: mobile/android/base/webapp/WebappImpl.java
@@ +183,3 @@
>          // Load a tab so it's available for any code that assumes a tab
>          // before the app tab itself is loaded in BrowserApp._loadWebapp.
> +        flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;

Might want to use "flags |=" instead so any additional flags passed in don't get wiped out and ignored.
Attachment #8543356 - Flags: review?(bnicholson) → review+
Comment on attachment 8543355 [details] [diff] [review]
remove-old-webapps v0.1

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

::: mobile/android/chrome/content/browser.js
@@ +464,3 @@
>      // The order that context menu items are added is important
>      // Make sure the "Open in App" context menu item appears at the bottom of the list
> +    this.initContextMenu();

This will make all our context menu actions work in webapps. I don't mind that, but note that we intentionally disabled it before (and apparently disabled it in SearchEngine startup too?)
Attachment #8543355 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #20)
> Comment on attachment 8543355 [details] [diff] [review]
> remove-old-webapps v0.1
> 
> Review of attachment 8543355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +464,3 @@
> >      // The order that context menu items are added is important
> >      // Make sure the "Open in App" context menu item appears at the bottom of the list
> > +    this.initContextMenu();
> 
> This will make all our context menu actions work in webapps. I don't mind
> that, but note that we intentionally disabled it before (and apparently
> disabled it in SearchEngine startup too?)

Not really. I verified that "pinned" is never true in BrowserApp.startup for webapps, or anything. We must disable context menus some other way. Wait, actually that's bug 1097261.
(In reply to Brian Nicholson (:bnicholson) from comment #19)
> Comment on attachment 8543356 [details] [diff] [review]
> make-pinned-work v0.1
> 
> Review of attachment 8543356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/webapp/WebappImpl.java
> @@ +183,3 @@
> >          // Load a tab so it's available for any code that assumes a tab
> >          // before the app tab itself is loaded in BrowserApp._loadWebapp.
> > +        flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;
> 
> Might want to use "flags |=" instead so any additional flags passed in don't
> get wiped out and ignored.

We totally ignore whatever is passed into WebappImpl.loadStartupTab, so we don't want the flags. I could have improved the comment to make the more clear.
https://hg.mozilla.org/mozilla-central/rev/66b114f7c038
https://hg.mozilla.org/mozilla-central/rev/5419d08cbe98
Status: NEW → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Verified as fixed in Firefox for Android 37.0a1 (2015-01-11)
Device: Samsung Galaxy S4 (Android 4.4.2)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1098199
Depends on: 1129935
Depends on: 1132591
This is a nice change. relnoted as "Changed: Android home screen shortcut now opens existing tab instead of new tab"
You need to log in before you can comment on or make changes to this bug.