941035 - [User Story] Browser Chrome: Load URL

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Peter Dolanjski [:pdol]]

User Story:
As a user I want to navigate to a web page by typing its URL so that I can navigate to specific addresses.

Acceptance Criteria:
Functionality should match existing Browser functionality unless described otherwise in UX spec.

Comment 1

[Kevin Grandon :kgrandon]

I feel like the work in bug 948302 solves the description of this bug.

If it doesn't, please re-open with more information. Marking as fixed for now.

Comment 2

[Ben Francis [:benfrancis]]

Francis, the spec says that if you're currently in a browser window and click on a hyperlink or a Rocketbar history result, that URL should load in the current window rather than opening a new one. Does this also apply when you enter a URL?

The current behaviour in master is to always open a new browser window when you enter a URL, which seems wrong.

Kevin, I don't think we should be relying on the search app for a function as basic as navigating to a URL. Currently we send an inter-app 'submit' message to the search app and the search app opens a new window. Wouldn't it be better if the Rocketbar itself recognised that the content of the URL bar was a URL and set the src of the iframe or opened a new browser window (depending on whether you're already in a browser window)?

Comment 3

[Francis Djabri [:djabber]]

According to the latest spec (https://mozilla.box.com/s/3s7zbdga8opcgbw2iaam), entering in a URL should open a new browser page, except if the user is already in a browser window, in which case it should load within the current window.

Comment 4

[Ben Francis [:benfrancis]]

Let's keep this user story open until this feature is implemented to spec.

I'd also like to re-visit the way this feature is implemented because I don't think something as simple as navigating to a URL should be dependent on the search app to work.

Comment 5

[Ben Francis [:benfrancis]]

Note that this bug should also cover the "go" button and loading a URL in the current window by typing it (not a new window), as per the UX spec.

Comment 6

[Dale Harvey (:daleharvey)]

Theres a WIP patch for this @ https://github.com/daleharvey/gaia/commit/9a2b87e3252b7c537ac4b0544d26a68496000296, It works and does what its supposed to

Some things I want to improve though, our state tracking of onhomescreen and adding currentApp is fairly error prone, we generally want to know 1. are we currently on the homescreen, 2. the state of the current app (is it a wrapper window, is it fullscreen), Should we track this via events or just |AppWindowManager.getActiveApp|, I think the latter

And |this._currentApp.iframe.src = url;| to navigate the current app (we only do this for wrapper windows), is probably wrong, what would be the cleaner way to implement this?

Comment 7

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

(In reply to Dale Harvey (:daleharvey) from comment #6)
> Theres a WIP patch for this @
> https://github.com/daleharvey/gaia/commit/
> 9a2b87e3252b7c537ac4b0544d26a68496000296, It works and does what its
> supposed to
> 
> Some things I want to improve though, our state tracking of onhomescreen and
> adding currentApp is fairly error prone, we generally want to know 1. are we
> currently on the homescreen, 2. the state of the current app (is it a
> wrapper window, is it fullscreen), Should we track this via events or just
> |AppWindowManager.getActiveApp|, I think the latter

My opinion: if we could avoid direct access from other modules, we should avoid and use events.

> 
> And |this._currentApp.iframe.src = url;| to navigate the current app (we
> only do this for wrapper windows), is probably wrong, what would be the
> cleaner way to implement this?

I cannot figure out better solution too. change src makes sense.

I will suggest:
Implement
AppWindow.prototype.isBrowser (without manifestURL)
and
AppWindow.prototype.navigate(url) (change src if our isBrowser=true)

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/19168

Comment 9

[Dale Harvey (:daleharvey)]

So a 3rd approach to this, we couldnt implement this in the rocketbar as it meant the behaviour wouldnt be shared between links opened within the search application and urls entered in the rocketbar.

We now fire a mozActivity for all links except e.me.

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8421088 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19168

Search app part looks good, thanks!

Comment 11

[Ben Francis [:benfrancis]]

(In reply to Dale Harvey (:daleharvey) from comment #9)
> So a 3rd approach to this, we couldnt implement this in the rocketbar as it
> meant the behaviour wouldnt be shared between links opened within the search
> application and urls entered in the rocketbar.

Why do you think that matters? Can't we do the URL detection in the Rocketbar and fire a web activity in both places?

Comment 12

[Kevin Grandon :kgrandon]

Perhaps we should look at handling the activity in the system in a follow-up, or another bug? I don't have an opinion either way. It could be a bit awkward if third party search apps have to satisfy the use case to open a browser window, but I also think the current approach might be less code?

Comment 13

[Dale Harvey (:daleharvey)]

> Why do you think that matters? Can't we do the URL detection in the Rocketbar and fire a web activity in both places?

We can, but the duplication of logic is messy especially as we are still deciding / changing on how new windows are opened in the window app (the context menu patch might need to change this further), the logic needs to happen in the search app so it happening in the system app is just duplicated code for now

Comment 14

[Dale Harvey (:daleharvey)]

* in the system app

Comment 15

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

Comment on attachment 8421088 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19168

I have some concern, lemme know if you have trouble addressing the comments.

Comment 16

[Dale Harvey (:daleharvey)]

Comment on attachment 8421088 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19168

Addressed review comments

Comment 17

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

Comment on attachment 8421088 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19168

Thanks !

Comment 18

[Dale Harvey (:daleharvey)]

Thanks Alive / Kevin

https://github.com/mozilla-b2g/gaia/commit/c123449a625878bf69aa2b8a0d611db4484d4d5f

Comment 19

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.

Comment 20

[Kevin Grandon :kgrandon]

Why change 2.0 to disabled? Can we leave it as fixed so I don't catch it in my uplift queries?

Comment 21

[Ben Francis [:benfrancis]]

Re-opening as not implemented to spec, as noted in comment 2

Comment 22

[Dale Harvey (:daleharvey)]

Dessigning from meta bug

Comment 23

[Kevin Grandon :kgrandon]

Dependencies closed, this should be ready for review.

Comment 24

[Alissa（leave from Mozilla）]

Attachment: Verify_video.3gp

This issue has been successfully verified on Flame 2.1,2.0
See attachment: Verify_video.3gp
Reproducing rate: 0/5

Comment 25

[Alissa（leave from Mozilla）]

FLame 2.0 new build:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0

Flame 2.1 new build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0