Closed Bug 521828 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect, major)

Fennec 1.1
defect
Not set
major

Tracking

(fennec1.1+)

VERIFIED FIXED
Tracking Status
fennec 1.1+ ---

People

(Reporter: aakashd, Assigned: mbrubeck)

Details

(Keywords: qablocker)

Attachments

(2 files, 3 obsolete files)

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
tracking-fennec: --- → ?
Flags: in-litmus?
Wow! Those are some serious steps to reproduce. Is this typical behavior? Yes, I'm asking you to defend the blocking? flag.
I've seen it, usually with the first set of steps to reproduce.
I've seen this too.
Keywords: qablocker
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?
Attached patch Patch (obsolete) — Splinter Review
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
Assignee: nobody → 21
Attachment #419292 - Flags: review?(mark.finkle)
Is this patch still valid since it's been in review for 3 months now?
patch is still fine. I am just not sure I like it. It seems like a band-aid on the problem.
tracking-fennec: ? → 1.1+
pushed:
http://hg.mozilla.org/mobile-browser/rev/79b9f7226153
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → REOPENED
Flags: in-litmus? → in-litmus-
Resolution: FIXED → ---
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.
Attached patch additional patch (obsolete) — Splinter Review
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.)
Attachment #437239 - Flags: review?(mark.finkle)
Comment on attachment 419292 [details] [diff] [review]
Patch

r+ for posterity
Attachment #419292 - Flags: review?(mark.finkle) → review+
Attached patch additional patch v2 (obsolete) — Splinter Review
Minor update based on feedback from mfinkle - remember to clear loadingURI when loading is done.
Attachment #437239 - Attachment is obsolete: true
Attachment #437399 - Flags: review?(mark.finkle)
Attachment #437239 - Flags: review?(mark.finkle)
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
Attachment #437754 - Flags: review?(mbrubeck)
Comment on attachment 437754 [details] [diff] [review]
alternate patch using loadgroup

switching to vivien since he'll be awake earlier
Attachment #437754 - Flags: review?(mbrubeck) → review?(21)
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! :)
Attachment #437754 - Flags: review?(21) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/17a6781c1ebb
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Attachment #419292 - Attachment is obsolete: true
Attachment #437399 - Attachment is obsolete: true
Attachment #437399 - Flags: review?(mark.finkle)
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
Attachment #437838 - Flags: review?(mbrubeck)
Attachment #437838 - Flags: review?(mbrubeck) → review+
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
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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.