If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

Fennec Graveyard
General
--
major
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: aakashd, Assigned: mbrubeck)

Tracking

({qablocker})

Fennec 1.1
qablocker
Bug Flags:
in-litmus -

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
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.

Comment 2

8 years ago
I've seen it, usually with the first set of steps to reproduce.
I've seen this too.
(Reporter)

Updated

8 years ago
Keywords: qablocker
(Reporter)

Comment 4

8 years ago
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?
Created attachment 419292 [details] [diff] [review]
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
Assignee: nobody → 21
Attachment #419292 - Flags: review?(mark.finkle)
(Reporter)

Comment 6

8 years ago
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.

Updated

8 years ago
tracking-fennec: ? → 1.1+
pushed:
http://hg.mozilla.org/mobile-browser/rev/79b9f7226153
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

8 years ago
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 → ---
(Assignee)

Comment 10

8 years ago
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.
(Assignee)

Comment 11

8 years ago
Created attachment 437239 [details] [diff] [review]
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.)
Attachment #437239 - Flags: review?(mark.finkle)
Comment on attachment 419292 [details] [diff] [review]
Patch

r+ for posterity
Attachment #419292 - Flags: review?(mark.finkle) → review+
Assignee: 21 → mbrubeck
(Assignee)

Comment 13

8 years ago
Created attachment 437399 [details] [diff] [review]
additional patch v2

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)
Created attachment 437754 [details] [diff] [review]
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
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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Attachment #419292 - Attachment is obsolete: true
Attachment #437399 - Attachment is obsolete: true
Attachment #437399 - Flags: review?(mark.finkle)
Created attachment 437838 [details] [diff] [review]
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
Attachment #437838 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
Attachment #437838 - Flags: review?(mbrubeck) → review+
pushed early load fix:
http://hg.mozilla.org/mobile-browser/rev/a427a2bd4bc0
(Reporter)

Comment 20

8 years ago
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
(Assignee)

Comment 21

8 years ago
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.