1071882 - Browser landing page should not persist tapping on a history result

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Kevin Grandon :kgrandon]

This may be a dupe, but I can't find the original bug.

After tapping on a top site or history item in the browser, we should close the browser, or navigate it in place. Currently it shows up in cards view, and it should not.

Comment 1

[Kevin Grandon :kgrandon]

Changing the title here as this bug could also be solved by navigating the the current browser instance.

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Pull request - navigate open app

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

Alive - how do you feel about enabling appWindow.navigate() for apps -> browser transformation? Is it too crazy? Anything better or ways that we can swap out an appWindow?

Opening this up for feedback now while I perform some additional testing.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

Sorry, but this makes me uncomfortable.

Is the land page === search app index?
Could we only override navigate behavior in SearchWindow? Or a new Window?
Could we try to kill the LandingPageWindow when it's going background?

Comment 5

[Kevin Grandon :kgrandon]

> Is the land page === search app index?

The landing page is the search app. This should be identified with role="search" due to the desire for replaceable search apps in the future.


> Could we only override navigate behavior in SearchWindow? Or a new Window?

We could perform a check inside of navigate() to only allow app -> browser navigation if the role is of search.


> Could we try to kill the LandingPageWindow when it's going background?

I tried this, but it seemed like a much more complex patch to write, one that would probably be regression prone. The first problem I had was that the homescreen was being flashed. We could delay the kill of the landing page, but then we still have to deal with incorrect transitions. Also I noticed lots of bugs for things like quickly edge swiping back to it or entering the task manager. These could be things already happening in master during an OOM kill, or perhaps I needed to hook into more events. I can try this again though, and we can try to work through these problems. Again, I do feel that trying this is much more complex.


> Sorry, but this makes me uncomfortable.

Any specific details as to why this makes you uncomfortable? Just the chance that it might cause unforseen issues? If this is only going to 2.2 we have a long time to let this bake, and the code is not too complex. The marionette test with the patch is quite solid so I'm not really worried about this feature breaking.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon from comment #5)
> > Could we only override navigate behavior in SearchWindow? Or a new Window?
> 
> We could perform a check inside of navigate() to only allow app -> browser
> navigation if the role is of search.
> 

Please go with this. The navigate without protecting will lead the user from an app to a browser site some day.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

Thanks for the tips Alive. I've added a guard in the pull request, and the existing unit tests in app_window cover this. Please let me know what you think.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

r+ with nits, ask review again if you think necessary.

Comment 9

[Kevin Grandon :kgrandon]

Thanks for the review.

In master: https://github.com/mozilla-b2g/gaia/commit/4fe544de9e8f542e267d6ed9c62bc45972b186c6

This should bake on master for a few days before we request uplift.

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

Since this has been baking on master now, I would like to go ahead and see if we can uplift this. This is one of the top UX priorities, as it's quite awkward to use the browser without it.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Users will have a weird experience using the system browser as the landing page will always remain open. It's not very browser like.
[Testing completed]: Manual and unit tests.
[Risk to taking this patch] (and alternatives if risky): We're doing something new here which is swapping out an app window for a browser window, but we only do so in the search app. I can't really think of anything that this would regress though.
[String changes made]: None.

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8499189 [details] [review]
Pull request - navigate open app

Is that turning a <iframe mozbrowser mozapp="app://search..."> into a frame that loads a "normal" web page? or are we creating a new frame?

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Comment on attachment 8499189 [details] [review]
> Pull request - navigate open app
> 
> Is that turning a <iframe mozbrowser mozapp="app://search..."> into a frame
> that loads a "normal" web page? or are we creating a new frame?

We create a new <iframe> element.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/69702e91ed84196d2890e4b935716cbe6658879e