1023306 - Android homepage shortcuts don't work when the awesomescreen is open

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Add a webpage shortcut to the Android homescreen
Open a webpage with Firefox for Android
Click on the URL to get to the awesomescreen
Hit the Home button to minimze the app
Click on the webpage shortcut that is on the Android homescreen

ER:
The webpage should be opened in a new tab

AR:
The awesomescreen is shown and the webpage is never loaded

Probably a regression from whatever fixed bug 774280.

Comment 1

[Aaron Train [:aaronmt]]

Also 29 affected. I suspect this has always been the case.

Comment 2

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)

> Probably a regression from whatever fixed bug 774280.

Our awesomescreen code has totally changed since then, so I'm not sure whatever logic that fixed that bug is still around.

We should probably dismiss edit mode when attempting to load the shortcut URL. Somehow this works properly when loading a URL from an external app (e.g. Twitter).

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Always dismiss edit mode when opening external URLs (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8439132 [details] [diff] [review]
Always dismiss edit mode when opening external URLs (r=margaret)

We were only dismissing edit mode:
- For ACTION_VIEW intent action
- After the page starts loading (the super.onNewIntent() call in BrowserApp)

This means this bug also affects links opened from third-party apps. Furthermore, Homescreen bookmarks use a different intent action (GeckoApp.BOOKMARK).

We have to dismiss edit mode before opening any external URL.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)

> This means this bug also affects links opened from third-party apps.
> Furthermore, Homescreen bookmarks use a different intent action
> (GeckoApp.BOOKMARK).

Not in the scope of this bug, but why do we even have this custom intent action? Couldn't we just use the ACTION_VIEW intent action for these shortcuts? Maybe a follow-up bug to investigate?

Comment 6

[:Margaret Leibovic]

Comment on attachment 8439132 [details] [diff] [review]
Always dismiss edit mode when opening external URLs (r=margaret)

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

::: mobile/android/base/BrowserApp.java
@@ +2620,4 @@
>          String action = intent.getAction();
>  
> +        boolean externalUrl = Intent.ACTION_VIEW.equals(action) ||
> +                              GeckoApp.ACTION_BOOKMARK.equals(action);

Make these local variable final?

@@ +2625,5 @@
> +        if (mInitialized && externalUrl) {
> +            // Dismiss editing mode if the user is loading a URL from an external app.
> +            mBrowserToolbar.cancelEdit();
> +
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT);

This is changing the meaning of this telemetry probe, but perhaps we're okay with that. We could file a follow-up to add a new separate probe for this ACTION_BOOKMARK intent. It could be interesting to know how often people use these shortcuts (although perhaps creating the shortcut is the action we care about more).

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Always dismiss edit mode when opening external URLs (r=margaret)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8439835 [details] [diff] [review]
Always dismiss edit mode when opening external URLs (r=margaret)

(In reply to :Margaret Leibovic from comment #6)
> Comment on attachment 8439132 [details] [diff] [review]
> Always dismiss edit mode when opening external URLs (r=margaret)
> 
> Review of attachment 8439132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2620,4 @@
> >          String action = intent.getAction();
> >  
> > +        boolean externalUrl = Intent.ACTION_VIEW.equals(action) ||
> > +                              GeckoApp.ACTION_BOOKMARK.equals(action);
> 
> Make these local variable final?

Done.
 
> @@ +2625,5 @@
> > +        if (mInitialized && externalUrl) {
> > +            // Dismiss editing mode if the user is loading a URL from an external app.
> > +            mBrowserToolbar.cancelEdit();
> > +
> > +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT);
> 
> This is changing the meaning of this telemetry probe, but perhaps we're okay
> with that. We could file a follow-up to add a new separate probe for this
> ACTION_BOOKMARK intent. It could be interesting to know how often people use
> these shortcuts (although perhaps creating the shortcut is the action we
> care about more).

Good point. This new patch uses a different TelemetryContract.Method when you open a homescreen bookmark. Requesting feedback from liuche on the new Telemetry method.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8439835 [details] [diff] [review]
Always dismiss edit mode when opening external URLs (r=margaret)

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

Let's hear back from liuche about the telemetry change, but this looks good to me. Thanks.

::: mobile/android/base/BrowserApp.java
@@ +2620,4 @@
>          String action = intent.getAction();
>  
> +        final boolean isViewAction = Intent.ACTION_VIEW.equals(action);
> +        final boolean isBookmarkAction = GeckoApp.ACTION_BOOKMARK.equals(action);

Can we file a follow-up mentor bug to rename this intent ACTION_HOMESCREEN? Or even ACTION_HOMESCREEN_SHORTCUT. I find the "bookmark" name confusing, since this doesn't actually have anything to do with our bookmark system.

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8439835 [details] [diff] [review]
Always dismiss edit mode when opening external URLs (r=margaret)

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

Looks good to me!

::: mobile/android/base/BrowserApp.java
@@ +2620,4 @@
>          String action = intent.getAction();
>  
> +        final boolean isViewAction = Intent.ACTION_VIEW.equals(action);
> +        final boolean isBookmarkAction = GeckoApp.ACTION_BOOKMARK.equals(action);

+1

Comment 11

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/30460db75b09

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8439835 [details] [diff] [review]
> Always dismiss edit mode when opening external URLs (r=margaret)
> 
> Review of attachment 8439835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's hear back from liuche about the telemetry change, but this looks good
> to me. Thanks.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2620,4 @@
> >          String action = intent.getAction();
> >  
> > +        final boolean isViewAction = Intent.ACTION_VIEW.equals(action);
> > +        final boolean isBookmarkAction = GeckoApp.ACTION_BOOKMARK.equals(action);
> 
> Can we file a follow-up mentor bug to rename this intent ACTION_HOMESCREEN?
> Or even ACTION_HOMESCREEN_SHORTCUT. I find the "bookmark" name confusing,
> since this doesn't actually have anything to do with our bookmark system.

Filed bug 1026010.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30460db75b09