Closed Bug 400671 Opened 17 years ago Closed 17 years ago

Location bar ignores entered url and uses autocomplete selected url instead

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: mossop, Assigned: moco)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

1. Start typing in the url bar so autocomplete pops up some suggestions.
2. Use the down arrow keys to select one.
3. Use the mouse and click into the url bar, click once to remove hte autocomplete and leave the cursor at the end then click again to move the cursor to somewhere other than the end of the url.
4. Type some changes then hit enter.

At this point the url reverts to that selected in step 2 ignoring the changes made.
Flags: blocking-firefox3?
I've been hit by this for a while, and it's one of the most annoying things I have to deal with currently.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102204 Minefield/3.0a9pre
OS: Mac OS X → All
Hardware: PC → All
After Step 2 you lost me, but maybe you mean bug 376868 ?
(In reply to comment #2)
> After Step 2 you lost me, but maybe you mean bug 376868 ?

That appears to be about the form fill autocomplete, not the url bar. And also 'm reasonably sure that this is a recentish regression but haven't had chance to double check that yet.
I'm able to reproduce this on windows xp, as well.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M11
Priority: -- → P3
(In reply to comment #3)
> (In reply to comment #2)
> > After Step 2 you lost me, but maybe you mean bug 376868 ?
> 
> That appears to be about the form fill autocomplete, not the url bar.

There's also bug 79069. But it's all the same widget, right?
(In reply to comment #5)
> There's also bug 79069. But it's all the same widget, right?

No that is core which is the old style autocomplete. I believe the toolkit version works differently.
bug 357220, filed on branch, looks pretty similar too.
the steps to reproduce from mossop sound related to bug #406487.

I'll work on a fix for that bug, and see if it addresses this one.
Assignee: nobody → sspitzer
Depends on: 406487
> I'll work on a fix for that bug, and see if it addresses this one.

The fix for bug #406487 does not address this bug, but this still seems related.
Status: NEW → ASSIGNED
This bug is a regression from bug 279703 so a few months earlier than the new 2 line autocomplete. I lost the bonsai link for the regression window.
ok, I think I have an idea of what is going on.

in nsAutoCompleteController::EnterMatch(), the selected index of our pop is >= 0, even though it should be -1 (or maybe we should check if the pop is closed or not?)

while that fix (checking if the popup is open) works, I think the problem is that when I click to close the popup, we don't call ClosePopup() which should call SetSelectedIndex(-1), which would fix this bug.
here's where we hide the window on windows:

 	gkwidget.dll!nsWindow::Show(int bState=0)  Line 1769	C++
 	gklayout.dll!nsView::SetVisibility(nsViewVisibility aVisibility=nsViewVisibility_kHide)  Line 463	C++
 	gklayout.dll!nsViewManager::SetViewVisibility(nsIView * aView=0x069b6880, nsViewVisibility aVisible=nsViewVisibility_kHide)  Line 1651	C++
 	gklayout.dll!nsMenuPopupFrame::HidePopup(int aDeselectMenu=1, nsPopupState aNewState=ePopupClosed)  Line 611	C++
 	gklayout.dll!nsXULPopupManager::HidePopupCallback(nsIContent * aPopup=0x06197890, nsMenuPopupFrame * aPopupFrame=0x069a6edc, nsIContent * aNextPopup=0x00000000, nsIContent * aLastPopup=0x00000000, nsPopupType aPopupType=ePopupTypePanel, int aDeselectMenu=1)  Line 680	C++
>	gklayout.dll!nsXULPopupManager::FirePopupHidingEvent(nsIContent * aPopup=0x06197890, nsIContent * aNextPopup=0x00000000, nsIContent * aLastPopup=0x00000000, nsPresContext * aPresContext=0x06ee4598, nsPopupType aPopupType=ePopupTypePanel, int aDeselectMenu=1)  Line 1006	C++
 	gklayout.dll!nsXULPopupManager::HidePopup(nsIContent * aPopup=0x06197890, int aHideChain=1, int aDeselectMenu=1, int aAsynchronous=0)  Line 634	C++
 	gklayout.dll!nsXULPopupManager::Rollup(nsIContent * * aLastRolledUp=0x025360d4)  Line 185	C++
 	gkwidget.dll!nsWindow::DealWithPopups(HWND__ * inWnd=0x0002112a, unsigned int inMsg=513, unsigned int inWParam=1, long inLParam=2818439, long * outResult=0x0012f7a4)  Line 7782	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x0002112a, unsigned int msg=513, unsigned int wParam=1, long lParam=2818439)  Line 1240 + 0x19 bytes	C++
 	user32.dll!7e418734() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
 	user32.dll!7e418816() 	
 	user32.dll!7e4189cd() 	
 	user32.dll!7e419402() 	
 	user32.dll!7e418a10() 	
 	gkwidget.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=1)  Line 149	C++
 	gkwidget.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=1)  Line 137 + 0x11 bytes	C++
 	gkwidget.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x00c01ed8, int mayWait=1, unsigned int recursionDepth=0)  Line 247 + 0xf bytes	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f990)  Line 500	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00c01ed8, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 181 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=3, char * * argv=0x00bdd880, const nsXREAppData * aAppData=0x00bddc70)  Line 3149 + 0x25 bytes	C++
 	firefox.exe!main(int argc=3, char * * argv=0x00bdd880)  Line 153 + 0x12 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	

in nsXULPopupManager::FirePopupHidingEvent(), we fire the onpopuphiding event.

maybe I need an onpopuphiding handler, and on that event, if we have the right event.target, call SetSelectedIndex(-1)?
Attached patch patch (obsolete) — Splinter Review
we already have a popuphiding handler on autocomplete-base-popup!  all we need to do is set the selectedIndex to -1 and whamo.
Attachment #292884 - Attachment is obsolete: true
Attachment #292896 - Flags: review?
Attached patch patch (obsolete) — Splinter Review
Attachment #292896 - Attachment is obsolete: true
Attachment #292896 - Flags: review?
Comment on attachment 292897 [details] [diff] [review]
patch

note, this will work for rich and non-rich autocomplete, as both have selectedIndex setters.
Attachment #292897 - Flags: review?(gavin.sharp)
Keywords: regression
Priority: P3 → P2
Attachment #292897 - Attachment is obsolete: true
Attachment #293157 - Flags: review?(gavin.sharp)
Attachment #292897 - Flags: review?(gavin.sharp)
over irc, gavin asked:

	gavin|	was wondering if you had any idea why it was caused by neil's popup rework
	

	sspitzerMsgMe	I don't know for sure, but if before neil's fix
	sspitzerMsgMe	we called
	sspitzerMsgMe	nsAutoCompleteController::ClosePopup()
	sspitzerMsgMe	directly
	sspitzerMsgMe	that would explain it
	sspitzerMsgMe	as that would call popup->SetSelectedIndex(-1);
	sspitzerMsgMe	popup->SetSelectedIndex(-1);

I haven't tested this theory out yet.
Comment on attachment 293157 [details] [diff] [review]
same fix, but add a comment

(In reply to comment #20)
>         gavin|  was wondering if you had any idea why it was caused by neil's
> popup rework

I think I figured this out. Prior to Neil's patch for bug 279703[1], the autocomplete controller was a nsIRollupListener for the popup, and it's nsIRollupListener::Rollup implementation [2], which would have been called when the popup was hidden by a click, calls ClosePopup(), which calls SetSelectedIndex(-1).

[1] http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAutoCompleteController.cpp&branch=&root=/cvsroot&subdir=mozilla/toolkit/components/autocomplete/src&command=DIFF_FRAMESET&rev1=1.57&rev2=1.58
[2] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp&rev=1.73#667

(This also brings up the fact that the controller no longer needs to implement nsIRollupListener, given the change in [1]. I'll file a new bug on that.)
Attachment #293157 - Flags: review?(gavin.sharp) → review+
fixed.

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.103; previous revision: 1.102
done

gavin, thanks for figuring out the answer to your question and for the review.

did you file a new bug about no longer needing to implement nsIRollupListener?

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
(In reply to comment #22)
> did you file a new bug about no longer needing to implement nsIRollupListener?

I have now: bug 408892.
Mossop: thanks for the easily-transplantable-into-Litmus testcase; in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5054
Flags: in-litmus? → in-litmus+
V. FIXED
Status: RESOLVED → VERIFIED
This has not been fixed. I still see this Feb 02 build. Reopen this bug!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: