Closed Bug 302575 Opened 19 years ago Closed 17 years ago

URL bar gets confused about what URI is loaded

Categories

(Firefox :: General, defect, P3)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 beta3

People

(Reporter: bzbarsky, Assigned: florian)

References

Details

(Keywords: fixed-seamonkey1.0)

Attachments

(3 files, 4 obsolete files)

Suite patch
2.29 KB, patch
bzbarsky
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
Testcase for address bar editing (hash)
988 bytes, text/html
Details
patch v6 (for checkin)
14.44 KB, patch
Details | Diff | Splinter Review 
STEPS TO REPRODUCE:

1)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=213946
2)  Click in the URL bar, add "#29" to the end of the URI, and hit enter
3)  Click on the link that says #30

EXPECTED RESULTS:  URL bar says
https://bugzilla.mozilla.org/show_bug.cgi?id=213946#30

ACTUAL RESULTS:  URL bar says https://bugzilla.mozilla.org/show_bug.cgi?id=213946#29

I get the same result in Seamonkey and Firefox, but that's probably because of
code copy/paste.  I'm guessing the issue here is that our code that decides
whether the URL bar was edited looks for progress events that are not fired for
anchor scrolls or something silly like that...
I assume you mean "#c29" and "#c30" for the URL hash items?
er, yeah.  ;)
Are we sure that nsIWebProgressListener::OnLocationChange is being fired?  If
so, then the UI must not be listening for that event.
The UI ignores OnLocationChange if it thinks the URI has been "edited by the
user".  My problem is with the way this last state is being determined.  Note
that if you never type anything in the URL bar, then it updates correctly when
scrolling to an anchor....
Do we also want the URL bar to update when a script executes an anchor scroll?
It does right now, no?
*** Bug 285622 has been marked as a duplicate of this bug. ***
So, when you type we set userTypedClear to 0, and increment it to say we can
change the displayed URI in startDocumentLoad, which only gets called for
STATE_START && STATE_IS_NETWORK - what else should we be listening for, to see,
um, state is scrolling to an anchor or maybe to the top of the document?

(Note to self: fixing this right ought to fix bug 227024 too)
I don't believe scrolling to an anchor fires anything other than onLocationChange.
Attached patch Suite patch — — Splinter Review 
My previous attempt at this patch dated back to the time when userTypedClear
was a boolean. Now that it's an integer the code is substantially simplified.
We temporarily bump userTypedClear in case an anchor load wants to reset the
location. Otherwise the document load starts, also bumping userTypedClear, so
that the URL bar will still get reset when the page eventually loads. If the
page is stopped before the location changes (e.g. errors with an alert) then
userTypedClear does not get reset thus avoiding dataloss.
Attachment #194185 - Flags: superreview?(darin)
Attachment #194185 - Flags: review?(bzbarsky)
Comment on attachment 194185 [details] [diff] [review]
Suite patch

So why does this help?
Comment on attachment 194185 [details] [diff] [review]
Suite patch

r=bzbarsky; I missed comment 10 somehow...
Attachment #194185 - Flags: review?(bzbarsky) → review+
Comment on attachment 194185 [details] [diff] [review]
Suite patch

sr=darin
Attachment #194185 - Flags: superreview?(darin) → superreview+
Comment on attachment 194390 [details] [diff] [review]
firefox patch

sr=darin
Attachment #194390 - Flags: superreview+
Fix checked in to the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Did anyone actually test this in Firefox? I tried porting it myself, and even
wrapping our other loadURI in browser.js, it still only sort-of worked, while
breaking other cases. Gavin's patch, which I assumed was a wip since he didn't
ask for any review, didn't do a single thing for me.
Comment on attachment 194390 [details] [diff] [review]
firefox patch

oops.. ok.  i didn't try it :(
Attachment #194390 - Flags: superreview+ → superreview-
Verified busted in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050831 Firefox/1.6a1 ID:2005083115 - for instance, follow bz's STR in a
new window with the tabbar hidden, you get no change in URL while clicking
anchors; then, ctrl-click a link to open a new background tab, and the urlbar is
marked dirty (default pageproxy icon, can't drag it).

Could someone please back out the Firefox patch?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The Suite patch caused bug 306810 as regression.

Bruno
Blocks: 306810
I think the suite patch should be backed out too. With the patch favicons of
pages opened with "open in new tab" aren't visible and drag&drop of the URI to
the toolbar is broken (you can't get a grip at the icon before the URI).
Blocks: 306832
(In reply to comment #21)
> I think the suite patch should be backed out too. With the patch favicons of
> pages opened with "open in new tab" aren't visible and drag&drop of the URI to
> the toolbar is broken (you can't get a grip at the icon before the URI).

That's Bug 306832 Bookmarks proxy icon (Favicon) not loaded when loading in a
new tab
The suite patch works with Neil's patch from Bug 306810. Bug 306832 gets fixed too.

Bruno
*** Bug 313318 has been marked as a duplicate of this bug. ***
Comment on attachment 194185 [details] [diff] [review]
Suite patch

a=me for SM1.0b on SM only part of code inconjunction with patch for bug 306810, 2nd needed one
SeaMonkey-only portion of patch checked in to the 1.8 branch.
Whiteboard: fixed-seamonkey1.0
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
So isn't this going to be fixed in 2.0?
Flags: blocking-firefox2?
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2
Gavin, can you clean up the patch here and get this fixed?
Assignee: nobody → gavin.sharp
Status: REOPENED → NEW
Attached patch working Firefox patch (obsolete) — — Splinter Review 
The tabbrowser implementations have diverged so much that the previous patch didn't have the intended effect. This one fixes this bug, by porting over more changes from the XPFE tabbrowser, but I haven't spent the time yet to verify that it won't have any unintended consequences, and if we really intend to make a change like this on the branch at this stage I'd like to be much more sure of it than I currently am. It does not regress bug 231393 in my testing.
Attachment #194390 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: -- → P1
Version: Trunk → 2.0 Branch
Given comment 30 and the time left in the release it doesn't look like we can finish this up for now so 181 drivers are taking this off the blocking list...
Flags: blocking-firefox2+ → blocking-firefox2-
Priority: P1 → P3
Target Milestone: Firefox 2 → Firefox 3 alpha1
Version: 2.0 Branch → Trunk
suite is now using toolkit's browser.xml with a port of the old xpfe tabbrowser to use that commone browser.xml - is this still fixed there?

And, don't we need to get the Firefox version in anyways some time?
It's broken again in Suiterunner (and probably still in Firefox).
Flags: blocking-firefox3?
Should probably be in toolkit so we can set the right blocking flags, fwiw...
(In reply to comment #34)
>suite is now using toolkit's browser.xml
Right, this is just one of the many changes that toolkit doesn't have, sigh...
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 alpha1 → Firefox 3 beta1
Priority: P3 → P2
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Target Milestone: Firefox 3 M8 → Firefox 3 M9
This is still broken and a pain while trying to do some fancy AJAX back button hacking... Please someone take another crack at it.  Thanks!
Comment on attachment 235957 [details] [diff] [review]
working Firefox patch

I can confirm that the browser.xml changes refix the bug for SeaMonkey.
Attached patch updated Firefox patch (obsolete) — — Splinter Review 
I still don't fully understand these changes, but they seem to fix the bug.
Attachment #235957 - Attachment is obsolete: true
(In reply to comment #40)
>I still don't fully understand these changes
browser.xml needs to tweak userTypedClear in case the URL is an anchor scroll; in this case there is no network activity associated with the location change so the normal userTypedClear does not occur.
tabbrowser.xml needs to tweak userTypedClear by 2 in case a document is loading; in this case the network stop/start resets userTypedClear to 2 so that browser.xml will only decrement it to 1 and the normal userTypedClear will occur.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Priority: P2 → P3
What's the status of this bug? With Neil's comment, is Gavin's question answered. Ready for review?
Doesn't work here with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121105 Minefield/3.0b2pre

The issue is very annoying for everyone developing or using client-side deep linking.
-> florian
Assignee: gavin.sharp → florian
Status: ASSIGNED → NEW
Gavin, do you know what was left to do here?

Apparently your last patch still applies. I'm going to test it to check that it still works. Maybe this comment should be added https://bugzilla.mozilla.org/show_bug.cgi?id=306810#c12
From what I've been told my latest patch fixes the problem. The reason I haven't asked for review is because it was just a naive copy of the xpfe code, and I haven't had the time to verify that it's correct or even fully understand what it was changing. That comment from Neil in the other bug certainly looks useful, and he's really the expert on this code. I'm just concerned about any Firefox-specific effects due to differences in our tabbrowser.
Please check any patches against this testcase.
Comment on attachment 280617 [details] [diff] [review]
updated Firefox patch

>Index: browser/base/content/browser.js

>           // Setting the urlBar value in some cases causes userTypedValue to
>-          // become set because of oninput, so reset it to its old value.
>-          browser.userTypedValue = userTypedValue;
>+          // become set because of oninput, so reset it to null.
>+          browser.userTypedValue = null;

Please see bug 397815 where I'm removing this.
I don't think the hack in its current form is achieving anything. Changing it to delete userTypedValue seems like it could break stuff.

>Index: browser/base/content/tabbrowser.xml

>             onLocationChange : function(aWebProgress, aRequest, aLocation)
>             {
>               // The document loaded correctly, clear the value if we should
>-              if (this.mBrowser.userTypedClear > 0 && aRequest)
>+              if (this.mBrowser.userTypedClear > 0)
>                 this.mBrowser.userTypedValue = null;

What is this change about? Without aRequest, there's no loading, so the comment isn't accurate anymore.
Attached patch updated Firefox patch (v5) (obsolete) — — Splinter Review 
Updated patch:
 * Added comments from the xpfe version of tabbrowser.xml

 * Replaced a few more getWebNavigation by getBrowser in browser.js

 * Removed the code touching the userTypedClear value from browser.js.  This seems to be duplicated code from tabbrowser.xml.  When I tested, the value of userTypedClear was always twice the value I expected because of this duplication.  I couldn't find any specific case that would execute this code from browser.js and not the same from tabbrowser.xml, please correct me if I'm wrong here :-).

Neil, do you know if there are cases where this code set the userTypedClear field to a non-zero value?
-                  if (this.mBrowser.userTypedClear > 0)
+                  if (this.mBrowser.userTypedClear > 1)
+                    this.mBrowser.userTypedClear -= 2;
+                  else if (this.mBrowser.userTypedClear > 0)
                     this.mBrowser.userTypedClear--;
If there are not, I would prefer to just set the value to 0, it would be more readable.
Attachment #280617 - Attachment is obsolete: true
Attachment #294676 - Flags: superreview?(neil)
Attachment #294676 - Flags: review?(gavin.sharp)
(In reply to comment #48)
> (From update of attachment 280617 [details] [diff] [review])
> >Index: browser/base/content/browser.js
> 
> >           // Setting the urlBar value in some cases causes userTypedValue to
> >-          // become set because of oninput, so reset it to its old value.
> >-          browser.userTypedValue = userTypedValue;
> >+          // become set because of oninput, so reset it to null.
> >+          browser.userTypedValue = null;
> 
> Please see bug 397815 where I'm removing this.

Ok, I removed this part from the patch I have just attached, so that our patches don't bitrot each other.


> >Index: browser/base/content/tabbrowser.xml

> >-              if (this.mBrowser.userTypedClear > 0 && aRequest)
> >+              if (this.mBrowser.userTypedClear > 0)
> >                 this.mBrowser.userTypedValue = null;
> 
> What is this change about? Without aRequest, there's no loading, so the comment
> isn't accurate anymore.
> 

There's a loading without request for anchor scrolls, and we want the location bar to be updated in this case.
(In reply to comment #49)
>Neil, do you know if there are cases where this code set the userTypedClear
>field to a non-zero value?
>-                  if (this.mBrowser.userTypedClear > 0)
>+                  if (this.mBrowser.userTypedClear > 1)
>+                    this.mBrowser.userTypedClear -= 2;
>+                  else if (this.mBrowser.userTypedClear > 0)
>                     this.mBrowser.userTypedClear--;
I know one case is when you initiate a load while one is already in progress; the new load increments userTypedClear to 3, and it needs to remain positive so that the subsequent location change will clear userTypedValue.
(In reply to comment #51)
>I know one case is when you initiate a load while one is already in progress;
>the new load increments userTypedClear to 3, and it needs to remain positive so
>that the subsequent location change will clear userTypedValue.
Actually I don't remember whether it's the first or second load (or both) that is typed and needs to be cleared.
Comment on attachment 294676 [details] [diff] [review]
updated Firefox patch (v5)

>-      // The document loaded correctly, clear the value if we should
>-      if (browser.userTypedClear > 0 && aRequest)
>-        browser.userTypedValue = null;

>-    // It's okay to clear what the user typed when we start
>-    // loading a document. If the user types, this counter gets
>-    // set to zero, if the document load ends without an
>-    // onLocationChange, this counter gets decremented
>-    // (so we keep it while switching tabs after failed loads)
>-    getBrowser().userTypedClear++;
>-

>-    // The document is done loading, we no longer want the
>-    // value cleared.
>-    if (getBrowser().userTypedClear > 0)
>-      getBrowser().userTypedClear--;
Interesting... these changes would probably work in SeaMonkey because its tabbrowser is always in tabbed mode and therefore does all of the heavy lifting, but you might find that if you turn off all tabbed browsing (if that's even possible in Firefox; I haven't read the code carefully) then usedTypedClear would stop working altogether.
(In reply to comment #53)
> you might find that if you turn off all tabbed browsing (if that's
> even possible in Firefox; I haven't read the code carefully) then
> usedTypedClear would stop working altogether.
> 

When I have a single tab (and then, no visible tab bar), the userTypedClear value is still changed from tabbrowser.xml.  I don't see how I could 'turn off' tabbed browsing (I guess you mean using only browser.xml and not tabbrowser.xml) in Firefox.
(In reply to comment #54)
>I don't see how I could 'turn off' tabbed browsing
In Mozilla 1.7 with default preferences, tabbed browsing wasn't enabled, and none of the tabbrowser goodies e.g. favicons were hooked up. In particular no tab progress listeners had been created that could change userTypedClear, which is why it had to be done in nsBrowserStatusHandler.js as well.
(In reply to comment #50)
> > >             onLocationChange : function(aWebProgress, aRequest, aLocation)
> > >             {
> > >               // The document loaded correctly, clear the value if we should
> > >-              if (this.mBrowser.userTypedClear > 0 && aRequest)
> > >+              if (this.mBrowser.userTypedClear > 0)
> > >                 this.mBrowser.userTypedValue = null;
> > 
> > What is this change about? Without aRequest, there's no loading, so the comment
> > isn't accurate anymore.
> > 
> 
> There's a loading without request for anchor scrolls, and we want the location
> bar to be updated in this case.

That's not really "loading". Also, onLocationChange is called without aRequest when you switch tabs. |&& aRequest| was added for that reason in bug 231393 / bug 316059.
(In reply to comment #56)

> onLocationChange is called without aRequest when you switch tabs.
> |&& aRequest| was added for that reason in bug 231393 / bug 316059.
>

I guess that's why in attachment 235957 [details] [diff] [review] Gavin added back the hack in tabbrowser/updateCurrentBrowser.
(In reply to comment #56)
>Also, onLocationChange is called without aRequest when you switch tabs.
Surely that only applies to the consumers of tabbrowser - its progress listener's onLocationChange always has a request.
Comment on attachment 294676 [details] [diff] [review]
updated Firefox patch (v5)

>+            // Remember the current clear state, then set it to false
>+            // so we don't clear the userTypedValue when just switching
>+            // tabs. Set it back to its old state after we're done.
>+            var userTypedClear = this.mCurrentBrowser.userTypedClear;
>+            this.mCurrentBrowser.userTypedClear = 0;
So, given that you're removing the userTypedClear code from browser.js it would seem that you don't need this code.
Attachment #294676 - Flags: superreview?(neil) → superreview+
Comment on attachment 294676 [details] [diff] [review]
updated Firefox patch (v5)

Would be awesome to get some tests in for this... I guess that might not be very easy.
Attachment #294676 - Flags: review?(gavin.sharp) → review+
Comment 59 addressed. Ready for checkin.
Attachment #294676 - Attachment is obsolete: true
mozilla/browser/base/content/browser.js 1.923
mozilla/browser/base/content/tabbrowser.xml 1.257
mozilla/toolkit/content/widgets/browser.xml 1.116
Status: NEW → RESOLVED
Closed: 19 years ago → 17 years ago
Resolution: --- → FIXED
Mano, Florian has commit privileges, fyi... :)
Works great here (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre). Thanks!
Depends on: 412841
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre ID:2008020304
Status: RESOLVED → VERIFIED
It was brought to my attention that a very similar problem still exists in Firefox 3 RC1. Every step for the reproduction is the same except the part that the user has to hit Enter.

STEPS TO REPRODUCE:

1)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=213946
2)  Click in the URL bar, add "#c29" to the end of the URI and do not hit Enter, just leave the bar
3)  Click on the link that says #c30

EXPECTED RESULTS:  URL bar says
https://bugzilla.mozilla.org/show_bug.cgi?id=213946#c30

ACTUAL RESULTS:  URL bar says https://bugzilla.mozilla.org/show_bug.cgi?id=213946#c29

If the user hasn't hit enter, the page navigation should NOT change the url bar.  Otherwise the page could keep a user from typing in a URL.
Boris,
where is that pattern described? Why IE, Safari and Opera don't work like this?
Can you give a sample of a site that keeps the user from typing in a URL.
> where is that pattern described? 

Other than in the code?  I have no idea.

> Why IE, Safari and Opera don't work like this?

Because they don't want to?  It's up to their UI designers to decide how their UI will behave.

> Can you give a sample of a site that keeps the user from typing in a URL

Any site that keeps setting location.hash every 50ms, say, if every set will reset the URL bar as you propose.
The above couple of comments look like miscommunication.

To rephrase slightly, should it work like this:

If the location bar has focus, then scripts should not be able to update the location bar. If it doesn't scripts should be allowed to update the url.

Is it not that simple?
Just because it doesn't have focus doesn't mean I'm done typing the URL.  I could be copying some text to paste into it.  I could have just focused another window temporarily.
In any case, if what you want is a change in designed behavior, you probably want to file a separate bug.  This bug was about a situation in which behavior departed from the plan, whereas you want a change of plan.
(In reply to comment #68)
> Why IE, Safari and Opera don't work like this?
Worse still, IE does this even when I'm typing in its open location dialog, which makes no sense at all!
Take for example Gmail which uses JavaScript to update the location.hash property. When the address bar value is edited like this the application cannot update it anymore and copying the address at a later stage won't be correct.

Why this behavior is limited to location.hash but not to location.href? This actually confuses Ajax/Flash linking but there are no such limitations for plain non-anchor links.

If you're convinced that the bug case is valid I'll create a new entry.
(In reply to comment #71)
> Just because it doesn't have focus doesn't mean I'm done typing the URL.  I
> could be copying some text to paste into it.  I could have just focused another
> window temporarily.
> 

Yeah, this is true, but if you start navigating again, in the same Firefox window, shouldn't the address bar lose focus automatically?

It's clear that you are no longer interested in whatever editing of the address bar you were doing at that point, no?

The original report of the bug was due to a RIA that allowed editing of the URI. One of our use cases was edit something, do something else, then edit again, then hit return. Our view was that the URI field should behave as a textbox that is the single element in a form whose action is location.href=location.href. So, forms generally don't submit on the leave event.
(In reply to comment #76)
> One of our use cases was edit something, do something else, then edit
> again, then hit return. 
> 

And that's a great goal, because IE totally screws the user on that front.

However, when the "do something else" in your use case, is to take steps such as clicking navigation links in the browser, I think that's an ideal time to start updating the address bar again.  Isn't that also a valid use case?


(In reply to comment #77)
> However, when the "do something else" in your use case, is to take steps such
> as clicking navigation links in the browser, I think that's an ideal time to
> start updating the address bar again.  Isn't that also a valid use case?
> 

That was another use-case. Our expectation was if the JS wanted to write to the URI, it should take precedence over any unused modifications.

Of course, the (now defunct) RIA that generated this bug report would have also accepted that the URI bar acted as a textbox that linked onBlur to onSubmit.

If a script attempts to change the location bar while the user has focus, then it should not start getting updated again.

The user's focus in the location bar is always the most important. If the user clicks another UI element, or otherwise changes that focus (submits a form or something manually - which would require them to move focus from the location bar to something else, I'd imagine), then the url bar should be updated again - unless the location text has been changed, and the enter button hasn't been pressed because of the point made in Comment #71.

If that's not robust enough (only time will tell) then that's for another bug.
Ok, I think we can all agree, that if the user has started changing stuff in the address bar, that scripts shouldn't be allowed to overwrite that, until the user presses either ENTER (submit) or ESCAPE (cancel).

However, IMHO the statements made in Comment #71 do not apply, as in my suggested use case the user is clearly no longer interested in whatever partial updates they did to the address bar.

If the user starts to change the address bar, hits neither ENTER or ESCAPE, but starts navigating again in the same Firefox window/tab, that (in my mind) is the same as if they pressed ESCAPE.  At the point they start navigating again, the address bar should lose focus.  As an end user this seems pretty clear cut?


(In reply to comment #80)
> Ok, I think we can all agree, that if the user has started changing stuff in
> the address bar, that scripts shouldn't be allowed to overwrite that, until the
> user presses either ENTER (submit) or ESCAPE (cancel).
> 
> However, IMHO the statements made in Comment #71 do not apply, as in my
> suggested use case the user is clearly no longer interested in whatever partial
> updates they did to the address bar.
> 
> If the user starts to change the address bar, hits neither ENTER or ESCAPE, but
> starts navigating again in the same Firefox window/tab, that (in my mind) is
> the same as if they pressed ESCAPE.  At the point they start navigating again,
> the address bar should lose focus.  As an end user this seems pretty clear cut?
> 

In other words, onBlur should be treated as ESCAPE. That sounds reasonable to me.


> In other words, onBlur should be treated as ESCAPE. That sounds reasonable to
me.

Just to clarify, onBlur would be treated as ESCAPE only if the text has not been edited (if the user pastes one thing in, and goes to copy another part, the onBlur would be triggered).
Can we please take the discussion to the appropriate forum?  In this case, this would be the mozilla.dev.apps.firefox newsgroup.  This bug, as I filed it, is fixed, and the mail from it is interfering with work actually getting done, to be honest...
Oh, and quoting the entirety of a comment in bugzilla is pretty poor form...
I don't know what exactly we can discuss anymore. Here is the new bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=435932
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: