521828 - Search entries and urls are not saved to urlbar until page is loaded

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b1pre) Gecko/20091012 Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091012
Fennec/1.0b5pre

Steps to Reproduce:
1. Go to www.cnn.com
2. Before the page title is shown in the url bar, click on the url bar

1. Go to the url bar, and enter a search entry (I used bugzilla)
2. Click on the search button
3. Before the page title is shown in the url bar, click on the url bar

1. Go to www.ilikemyfacebooks.net (or any page that doesn't exist)
2. Before the "Page Load Error" is shown in the url bar, click on the url bar

Actual Results:
The url of the previous page is shown

Expected Results:
The url and/or search entry should be "saved" to the url bar. It's prevalence should not depend on when the page is loaded

Comment 1

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

Wow! Those are some serious steps to reproduce. Is this typical behavior? Yes, I'm asking you to defend the blocking? flag.

Comment 2

[Stuart Parmenter]

I've seen it, usually with the first set of steps to reproduce.

Comment 3

[Madhava Enros [:madhava]]

I've seen this too.

Comment 4

[Aakash Desai [:aakashd]]

Mark, I flagged it as blocking to get some eyes on it. I think its particularly nasty behavior, but not blocking 1.0 as it doesn't completely break the browser even though it degrades user experience...which would make it as part of the 1.1 release. Stuart, what do you think?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This happen because the onLocationChange has not fired yet so the getDisplayURI return the previous URI instead of the new one (which is not really know at this time).

This patch put the value entered by the user if the uri is not know when the user enter into "edit" mode of the location bar

Comment 6

[Aakash Desai [:aakashd]]

Is this patch still valid since it's been in review for 3 months now?

Comment 7

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

patch is still fine. I am just not sure I like it. It seems like a band-aid on the problem.

Comment 8

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/79b9f7226153

Comment 9

[Aakash Desai [:aakashd]]

I don't see this as fixed on build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100401 Namoroka/3.6.3pre Fennec/1.1a2pre

The search entries and urls are only saved to the urlbar once a server has responded back to the initial request (and the page title is loaded)

Comment 10

[Matt Brubeck (:mbrubeck)]

This patch (attachment 419292 [details] [diff] [review]) also causes some new problems.  For example, if you click the urlbar after the title appears but while the page is still loading, then the edit field will contain the title instead of the URL.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: additional patch

So, the original problem was that we were replacing the urlbar contents with browser.currentURI.  Since currentURI is updated only after load, it is incorrect while the page is still loading.

Next we tried grabbing the URL from BrowserUI._edit.value, but that doesn't always work because (A) _edit.value might contain the title instead, and (B) DOMTitleChanged fires before onLocationChange, so we still sometimes end up displaying the title of the new page and the URI of the old one.

Here's my proposal for solving these problems.  This patch gives each Tab a "loadingURI" field that contains the URI of the new page, and uses that instead of the edit value.

It also adds a call to updateURI during the "network start" progress event when the urlbar is empty - just like desktop Firefox.  (This part of the code will be cleaned up slightly by an upcoming patch for bug 557238.)

Comment 12

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

Comment on attachment 419292 [details] [diff] [review]
Patch

r+ for posterity

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: additional patch v2

Minor update based on feedback from mfinkle - remember to clear loadingURI when loading is done.

Comment 14

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

Attachment: alternate patch using loadgroup

This patch uses the browser loadgroup to tell if the load requests are still active and uses the loadgroup default request to return the loading URI

Comment 15

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

Comment on attachment 437754 [details] [diff] [review]
alternate patch using loadgroup

switching to vivien since he'll be awake earlier

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 437754 [details] [diff] [review]
alternate patch using loadgroup

This is an inventive approach and I prefer this one to the previous because there is less code change.

btw, I've been to bed just a few before your ping! :)

Comment 17

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/17a6781c1ebb

Comment 18

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

Attachment: sets urlbar text very early during a load

Matt found an edge case where the urlbar can be empty when a new tab is loading (examples are URL from command line or code that opens new tabs with preset URL).

This patch should fix that. Desktop Firefox does something similar

Comment 19

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

pushed early load fix:
http://hg.mozilla.org/mobile-browser/rev/a427a2bd4bc0

Comment 20

[Aakash Desai [:aakashd]]

I'm using build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.4pre) Gecko/20100412 Namoroka/3.6.4pre Fennec/1.1a2pre

and I still get the previous/current page's url when I do any of the steps to perform on comment #0

Comment 21

[Matt Brubeck (:mbrubeck)]

I think this is expected.  In desktop Firefox there is a (sometimes long) delay before the URL bar changes after clicking a link or performing a search.  According to https://developer.mozilla.org/en/nsIWebProgressListener this is because we don't know right away whether a new request will replace the current document.  (For example, it might be a download instead.)

These patches should reduce the times when outdate URIs are displayed.  In particular they should fix cases where an old URI is displayed in the urlbar *after* the new one has already been entered there (e.g. the third set of repro steps in comment 0).  The most recent patch also fixes the regression in comment 10.

Comment 22

[Tony Chung [:tchung]]

the responsiveness seems much snappier now with the patches and the testcase in comment 0.  going to mark verified in Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2.5) Gecko/20100603 Firefox/3.6.5pre Fennec/1.1