Closed Bug 395174 Opened 13 years ago Closed 12 years ago

Identity pop-up should disappear as soon as I start typing in the location bar


(Firefox :: General, defect)

Not set



Firefox 3 beta1


(Reporter: ria.klaassen, Assigned: johnath)




(1 file, 6 obsolete files)

Steps to reproduce:

- Click on the favicon in the URLbar in order to select the field, the identity popup will appear
- Start typing, several history autocomplete pop-ups will appear and overlap the indentity popup
- When the autocompletes stop appearing you'll still see the identity pop-up of the site you planned to leave and that's illogical

Maybe one would say: don't click on the favicon if you want to leave, but it is the only way that ALWAYS selects the location bar field.
The pop-up also stays after hitting enter and while the new site is loading (although the information in the pop-up changes) and it only disappears when you click in the site.
Flags: blocking-firefox3?
This is similar to, but not identical to 395314; IMO, as soon as the URL field has changed, Larry should no longer report website identity, since it no longer relates to the address that the user is viewing.
Flags: blocking-firefox3? → blocking-firefox3+
agreed.  would like to fix for M9
Assignee: nobody → johnath
Target Milestone: --- → Firefox 3 M9
see also bug 400935
So this patch does both things mentioned in the bug:
 a) hide the popup, if it's showing, when the user types in the location bar (this was the bug as originally reported)
 b) hide the identity box as well, when the user types.

Doing a) makes perfect sense, but having it here and interacting with it, b) feels weird.  It causes the location bar to jump around while you're typing - and even though the caret does the right thing, the shifting text is still a little jarring.

I think part of what makes it weird is that we don't have any precedent - we don't, for instance, take away the yellow background on input.  Maybe that should happen too, but the current behaviour feels "off" somehow.  I'm open to suggestions here, since beltzner and mconnor both seemed to support this behaviour - maybe I'm alone in thinking this way?

As you can see from the patch, it would be trivial to modify it to do only a), if we felt that was appropriate/preferable.
(In reply to comment #6)
> we don't, for instance, take away the yellow background on input.

Bug 398020 does that.
But IMHO doing that isn't really related to the problem that occurs if you remove the box as the user starts to type.
Is it possible to blank the contents of the identity box when edit begins, then then wait until the user hits enter or clicks the go button or selects an auto-complete entry or whatever other actions are associated with end-of-edit.  At that point if the url is effectively unchanged then restore the contents else hide the box and let the new page load.
Drat! Forgot to mention that by "blank" I mean hide the context of the identity box while leaving its dimensions the same. (text colour set to background colour?)
Attached patch Just hide the popup (obsolete) — Splinter Review
This patch just hides the popup, but leaves the identity box untouched, per the original problem report.  Bug 401560 filed to track the broader discussion about whether removing SSL indicators on locbar-input is desireable.
Attachment #286340 - Attachment is obsolete: true
Attachment #286557 - Flags: review?(
Comment on attachment 286557 [details] [diff] [review]
Just hide the popup

Could consolidate input event handlers by adding the code from the URL bar's oninput to this new handler.
Attachment #286557 - Flags: review?( → review+
Per Gavin's suggestion, this patch removes the oninput from browser.xul and brings the one-liner into the URLBarOnInput handler.
Attachment #286557 - Attachment is obsolete: true
Keywords: checkin-needed
Doesn't the listener need to be added in BrowserToolboxCustomizeDone, too?
As Dao mentioned, registering the input handler in DelayedStartup means also having to register it in other places, like BrowserToolboxCustomizeDone.  This is actually a bug for the drag and drop handlers too, filed bug 401576.
Attachment #286561 - Attachment is obsolete: true
Comment on attachment 286568 [details] [diff] [review]
Register handler using xul attribute instead of addEventListener call

>+                   oninput="return URLBarOnInput(event);"

IMHO the |return| should be removed. URLBarOnInput doesn't return anything, and if needed there's the preventDefault event method.
Dao's right, I think - don't make it look like oninput is going to return when the handler doesn't.
Attachment #286568 - Attachment is obsolete: true
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.878; previous revision: 1.877
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.385; previous revision: 1.384
Closed: 13 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Verified. Great!
Flags: in-litmus? → in-litmus+
Stephen, what's the litmus test for this?

Is the patch that has been checked in still making a difference? That is, can you actually type when the popup is open? I can't.
(In reply to comment #19)
> Stephen, what's the litmus test for this?

Sorry, I should've posted it:  I originally tried to add this case to, but they're distinct enough that I created the new one.

> Is the patch that has been checked in still making a difference? That is, can
> you actually type when the popup is open? I can't.

It doesn't work for me, either, using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre.

I think this has changed from what Ria saw in comment 0 because we no longer select the URL bar on favicon/Larry click, right?  Should we reopen, or file a new bug? 

(In reply to comment #20)
> I think this has changed from what Ria saw in comment 0 because we no longer
> select the URL bar on favicon/Larry click, right?

Yes, and that's intentional: bug 400935. So no new bug is needed.
We should probably back out this bug's patch, since it's just unneeded overhead, and resolve this bug as invalid -- it was valid when it was filed, but the steps to reproduce don't work anymore.
Resolution: FIXED → ---
Attached patch revert patch (obsolete) — Splinter Review
Attachment #318238 - Flags: review?(
Flags: blocking-firefox3+ → blocking-firefox3-
Comment on attachment 318238 [details] [diff] [review]
revert patch

Can we get confirmation that this isn't needed on Linux? IIRC popup behavior there is special and might require this kind of hack.
Tested on Linux: opening the identity popup doesn't focus the address field, so you can't type there.
Comment on attachment 318238 [details] [diff] [review]
revert patch

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

>+                 oninput="gBrowser.userTypedValue = this.value"

Nit: add a trailing ";".

r=me, but I don't think we should land this until we branch/switch to Hg/whatever.
Attachment #318238 - Flags: review?( → review+
Attachment #286575 - Attachment is obsolete: true
Attachment #318238 - Attachment is obsolete: true
Keywords: checkin-needed
revert patch landed:
Closed: 13 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.