1130372 - Onboarding screen still showing behind externally opened url

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Steps to reproduce:
- clear data from fennec
- open & observe first run screen
- switch to twitter app
- open url in fennec
- notice page load
- press back button
- observe first run screen flash as fennec 

Expected: 
First run should hide when an external url is loaded

Comment 1

[:Margaret Leibovic]

This used to work. A regression from bug 1063844?

Comment 2

[Chenxia Liu [:liuche]]

Technically a regression, but the old version used Dialogs which would get dismissed by Android. Thanks for filing!

This should be as simple as adding another call to hideFirstrun() on page loads (and perhaps other places).

I'll mentor, or just fix this.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #2)

> This should be as simple as adding another call to hideFirstrun() on page
> loads (and perhaps other places).

We should still be getting into onNewIntent, so I would try to fix this in the "external URL being passed to Fennec" phase, and not the "when a pageload occurs" phase.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Finding a contributor would be fine, but let's make sure this is fixed soon.

Comment 5

[Bhargav Chippada [:bhargavch]]

Hi, I want to work on this bug. I was able to reproduce it using the steps given by Martyn in comment 0. I also observed that fennec is still in the recent activity stack after pressing the back button, it is a bug right? we except fennec to close after we press back button on loading an external url.

Comment 6

[Chenxia Liu [:liuche]]

Actually, I'm not sure I see this bug anymore. The bug as reported is that the first run screen (which you can see if you open up Fennec for the first time from the launch icon) was flashed before the external url is launched.

Martyn, do you still see this bug? I can't repro on a N9, N7, N4.

Comment 7

[Bhargav Chippada [:bhargavch]]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Actually, I'm not sure I see this bug anymore. The bug as reported is that
> the first run screen (which you can see if you open up Fennec for the first
> time from the launch icon) was flashed before the external url is launched.
> 
> Martyn, do you still see this bug? I can't repro on a N9, N7, N
The bug reciprocation steps say the first run screen is flashed after (not before) pressing back button on loading an external URL. Also fennec data has to be cleared every time to reciprocate it. Hope I was helpful.. Can u reciprocate it now?

Comment 8

[Bhargav Chippada [:bhargavch]]

Attachment: Bug_1130372_-_Onboarding_screen_still_showing_behind_externally_opened_url_r=mfinkle.patch

Hi, I created a patch based on the suggestions from comment 2 by chenxia and comment 3 by mark finkle. I am hiding the firstrun pane using hideFirstrunPager() method inside onNewIntent method when the user is loading an external url. The patch worked.

Comment 9

[Martyn Haigh (:mhaigh)]

Bug still showing in nightly - looks like Bhargav has this one :)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8571948 [details] [diff] [review]
Bug_1130372_-_Onboarding_screen_still_showing_behind_externally_opened_url_r=mfinkle.patch

Seems OK to me, but Chenxia is a better reviewer.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8571948 [details] [diff] [review]
Bug_1130372_-_Onboarding_screen_still_showing_behind_externally_opened_url_r=mfinkle.patch

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

Sorry for the delay!

I think this looks okay, but while trying to apply this patch and test it, I found that the patch is malformed. There seems to be a lot of metadata that is unnecessary, and the patch is using relative paths, not absolute ones. Since you already have the changes, try making a mercurial patch - take a look at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Also, for the commit message, it should have the bug number, the description, and the reviewer.

Bug 1130372 - Onboarding screen still showing behind externally opened url. r=liuche

Flag me for review again when you have an updated patch - I'll test and review it.

Thanks!

::: ../../../../mobile/android/base/BrowserApp.java
@@ +3313,5 @@
>              mBrowserToolbar.cancelEdit();
>  
> +            // Hide firstrun-pane if the user is loading a URL from an external app.
> +            if (hideFirstrunPager()) {
> +                Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, TelemetryContract.Method.INTENT, "firstrun-pane");

UITelemetry is meant to record events where the user interacts with the UI - we don't want to record that they cancelled the first run screen if it was automatically hidden. We shouldn't record a telemetry event for this.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #11)

> UITelemetry is meant to record events where the user interacts with the UI -
> we don't want to record that they cancelled the first run screen if it was
> automatically hidden. We shouldn't record a telemetry event for this.

Yeah, I guess that makes sense. I was thinking about whether logging telemetry makes snese or not, but I think you're right. Let's not log it.

Also, you made me ask a new question:

Instead of hiding the first-run pager when opening an external Intent, can we skip creating/showing the first-run pager altogether? Maybe we can't, but I thought avoiding the work of showing & then hiding would be better.

Comment 13

[Bhargav Chippada [:bhargavch]]

Attachment: Bug_1130372_Onboarding_screen_still_showing_behind_externally_opened_url_r=liuche.patch

I removed Telemetry like you suggested and the patch is in the right format too :)

Comment 14

[Bhargav Chippada [:bhargavch]]

Hi, Following are the reproduction steps for a potential bug:
- kill all fennec activities from recent activity stack
- Open twitter app
- Open url in fennec
- notice page load
- press back button
- We will return to twitter app
- U will see fennec app still in recent activity stack
  (On clicking that fennec activity it will open the fennec homepage)
Expected: fennec shouldn't be in the recent activity stack right?
Should i file this bug?

Comment 15

[Chenxia Liu [:liuche]]

Thanks for the patch, Bhargav - I'll take a look at it today.

I think there's a misunderstanding about this bug - we don't mind Fennec being in the recent activities list at all, that is completely fine and expected. This bug is for the introduction screen (you can see the mocks in bug 1063844) being displayed or flashed briefly when Fennec is being launched from another app. When a user opens Fennec from Twitter to read a link, they don't want to see the introduction screen, they want to see the link - so Fennec shouldn't display (or flash) that screen.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8574282 [details] [diff] [review]
Bug_1130372_Onboarding_screen_still_showing_behind_externally_opened_url_r=liuche.patch

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

I can't repro the original bug on any of my devices, and I don't think this is doing anything that isn't already being done, so I'm going to pass this review to Martyn - we already check for Intent action not being a VIEW action and don't show the firstrun screen ( http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#853 ). This patch is doing an explicit check in order to hide the firstrun in onNewIntent(), but under the same conditions, we shouldn't have shown it in the first place.

Martyn, can you repro and verify that this patch does fix the bug?

Bhargav, sorry for the back and forth!

Comment 17

[Martyn Haigh (:mhaigh)]

Comment on attachment 8574282 [details] [diff] [review]
Bug_1130372_Onboarding_screen_still_showing_behind_externally_opened_url_r=liuche.patch

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

No longer seeing the onboarding screen with this patch.

Comment 18

[Chenxia Liu [:liuche]]

Oh ****, this fell through the cracks - landing it now.

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

Comment 19

[Chenxia Liu [:liuche]]

Comment on attachment 8574282 [details] [diff] [review]
Bug_1130372_Onboarding_screen_still_showing_behind_externally_opened_url_r=liuche.patch

Approval Request Comment
[Feature/regressing bug #]: First run experience
[User impact if declined]: Opening Firefox for the first time through an external link will flash the first run screen.
[Describe test coverage new/current, TreeHerder]: local testing
[Risks and why]: very low, one additional call to hide the first run UI
[String/UUID change made/needed]: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9b8636d515b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30b2115bdb18

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: Nexus 4 (Android 4.4)
Builds: Firefox for Android 40.0a1 (2015-04-23), Firefox for Android 39.0a2 (2015-04-23) and Firefox for Android 38 Beta 6