Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030816 Mozilla Firebird/0.6.1+ (also in 0809) Steps to repro: 1. Drag a http://www.google.com/ window such that the textbox is within several textbox-heights of the bottom of the screen. 2. Start typing into the textbox so autocomplete appears. Result: the autocomplete dropdown covers the textbox, so you can't see what you're typing. (This is especially annoying if the box is for something like a URL, because you can type several characters and still have a lot of autocomplete results.) Expected: the bottom of the autocomplete dropdown should coincide with the top of the textbox, not the bottom of the textbox.

Jesse, I can't reproduce this with the 20030830 build on W2K. Can you please test this with a more current nightly build and a fresh profile. Thanks.

Still happens with 09/02 on WinXP, even with a fresh profile.

I'm getting the same as Jesse but on Linux.

OK, I see this now.

-ing will take patch

Well, this works, but a part of this code should be done here (the adjustment of the popup postion part): http://lxr.mozilla.org/seamonkey/source/toolkit/components/satchel/src/nsFormFillController.cpp#231 But I am not able to do that. With this patch, I can only assume what the height of the input box is (I take 23px for that). This patch also fixes the positioning of regular xul autocomplete textboxes (and that fix is reliable).

*** Bug 274568 has been marked as a duplicate of this bug. ***

This toolkit.zip archive repairs this bug. Download this ZIP archive, uncompress it and copy this to the "Program Files/Mozilla Firefox/chrome" directory. Then restart FireFox.

This patch is reliable, I think. But I had to change the nsIAutoCompletePopup.idl file for this. I don't know if that is allowed. I find the name 'popupRect' as variable in nsFormFillController::SetPopupOpen kind of misleading, by the way. As far as I know, I get here the coordinates of the corresponding input element, so 'inputRect' or something like that seems more appropriate to me.

Dean, could you review? By the way, I believe that this part of the patch: + this.popup.openPopup(this, -1, -1, this.boxObject.width, this.boxObject.height); could be safely changed to: + this.popup.openPopup(this, -1, -1);

Comment on attachment 169206 [details] [diff] [review] Patch nsIAutoCompletePopup.idl is not @FROZEN so it should be OK, but you'd have to rev the uuid in the .idl file. > I find the name 'popupRect' as variable in nsFormFillController::SetPopupOpen kind of misleading I agree. It's a local variable that's only used on the next line, so it's easy enough to change to something like |inputRect|. + if (aY==-1) { Spaces required around |==|. + if (aY+this.boxObject.height > screen.height) aY = aY - this.boxObject.height - aHeight; Spaces required around |+| and a newline is needed after the condition. Before getting into all of these changes, though, is this the right approach? - document.popupNode = null; - this.showPopup(document.documentElement, aX, aY, "popup", null, null); + document.popupNode = null; + if (aY==-1) { + this.showPopup(this.mInput, -1, -1, "popup", 'bottomleft', 'topleft'); + } + else { + if (aY+this.boxObject.height > screen.height) aY = aY - this.boxObject.height - aHeight; + this.showPopup(document.documentElement, aX, aY, "popup", null, null); + } This whole part is confusing to me. Why not just always adjust aY instead of having these two different paths? I've been on holidays for two weeks, so maybe I just need you to explain it to me.

Ok, this is what I think is happening: XUL autocomplete textboxes are opening textboxes with this method: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/autocomplete.xml#302 That is calling this: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/autocomplete.xml#502 But the method (at autocomplete.xml#502) is also called from: http://lxr.mozilla.org/seamonkey/source/toolkit/components/satchel/src/nsFormFillController.cpp#237 (mFocusedPopup->OpenPopup(this, popupRect.x, popupRect.y+popupRect.height, popupRect.width);) When openPopup (at autocomplete.xml#502) is called from openPopup (at autocomplete.xml#302), aInput is of type XULElement. I am able to use this as an argument in this.showPopup, so I can do this: this.showPopup(this.mInput, -1, -1, "popup", 'bottomleft', 'topleft'); This is the ideal situation, as the popup gets 'automagically' positioned in the right place. (the showPopup function seems to handle this fine, I've seen it used in other code, so I guess this is quite normal to do it this way, see: http://lxr.mozilla.org/seamonkey/search?string=showpopup) But this is not the case when openPopup (at autocomplete.xml#502) is called from: http://lxr.mozilla.org/seamonkey/source/toolkit/components/satchel/src/nsFormFillController.cpp#237 Then aInput is of type nsIAutocompleteInput. So that's why for this ' this.showPopup(document.documentElement, aX, aY, "popup", null, null)' is needed. I can't use aInput for this. So, I'm using the '-1' argument as a way to make a distinction between those two.

This bug has started biting me on a site I'm working on. Noticed the patch here, with several comments made about it, so I decided to go ahead and fix it up as per the reviewer's requests. Sorry if I'm stepping on any toes here. I generated a new UUID as requested view Microsoft's GUIDGen program, hopefully that is sufficient. As for the deeper explanation of the patch in general, I hope that the comments above by the original patch submitter suffice for the reviewer. If not, I ask the original patch author to please help with any further explanations if he could, as it would be GREAT to get this cleared up for the coming releases. Thanks in advance.

Thanks for updating the patch, Tim! Neil, maybe you would like to have a look at the patch?

Comment on attachment 191526 [details] [diff] [review] Unbitrotted patch > /* > * Bind the popup to an input object and display it with the given coordinates > * > * @param input - The input object that the popup will be bound to > * @param x - The x coordinate to display the popup at > * @param y - The y coordinate to display the popup at > * @param width - The width that the popup should size itself to > */ You need to update the comment... >- void openPopup(in nsIAutoCompleteInput input, in long x, in long y, in long width); >+ void openPopup(in nsIAutoCompleteInput input, in long x, in long y, in long width, in long height); It seems to me that if you knew the element then everything would be easy. So why not change the definition as follows: void openPopup(in nsIAutoCompleteInput input, in nsIDOMElement element, in long width); so nsFormFillController uses mFocusedInput and autocomplete.xml uses this.

Although I don't really know anything about toolkit autocomplete...

(In reply to comment #15) > It seems to me that if you knew the element then everything would be easy. So > why not change the definition as follows: > void openPopup(in nsIAutoCompleteInput input, in nsIDOMElement element, in long > width); > so nsFormFillController uses mFocusedInput and autocomplete.xml uses this. I think you're right. However, if I remember correctly, I tried that, but didn't succeed in doing. I'm not sure anymore, why.

*** Bug 309274 has been marked as a duplicate of this bug. ***

*** Bug 292831 has been marked as a duplicate of this bug. ***

I need this to work, so accepting.

The last patch was almost correct. I didn't understand why you needed to make any API change. This patch corrects the problem. I need this change on the MOZILLA_1_8_BRANCH.

+ aY = aY - this.boxObject.height - aHeight; Where are you getting the aHeight from?

Comment on attachment 214692 [details] [diff] [review] Fixes problem without API change hmm... you are right. I wonder what gives. I see no error and the right thing happens. Do i even need: if (aY + this.boxObject.height > screen.height) aY = aY - this.boxObject.height - aHeight;

Comment on attachment 214692 [details] [diff] [review] Fixes problem without API change is the URL autocomplete widget different then the one in text fields (like a html text field)?

Martijn | Neil are you happy with the Tim's unbitrotted patch? (other than the comment)

(In reply to comment #24) > is the URL autocomplete widget different then the one in text fields (like a > html text field)? Well, they use the same autocomplete widget, but this.mInput is of nsIAutoCompleteInput (I think) for html text fields, which doesn't work when opening popups. So if you just need it for the url bar, I think you can do: - this.showPopup(document.documentElement, aX, aY, "popup", null, null); + if (aY == -1) { + this.showPopup(this.mInput, -1, -1, "popup", 'bottomleft', 'topleft'); + } + else { + this.showPopup(document.documentElement, aX, aY, "popup", null, null); + } So that way no interface change is needed, but the bug will still be there for html text inputs.

(In reply to comment #25) > Martijn | Neil > are you happy with the Tim's unbitrotted patch? (other than the comment) Fine by me, but Neil should decide.

yes... that is exactly what confused me. In minimo (see screenshot: http://weblogs.mozillazine.org/dougt/archives/009834.html), we use a url bar at the bottom of the screen. the UI is completely broken when autocompleting. I think we should fix this correctly on both the branch and the trunk. I can do the leg work of creating new branch interface, if required.

(In reply to comment #15) > It seems to me that if you knew the element then everything would be easy. So > why not change the definition as follows: > void openPopup(in nsIAutoCompleteInput input, in nsIDOMElement element, in long > width); > so nsFormFillController uses mFocusedInput and autocomplete.xml uses this. I tried now to use this suggestion and then just do this.showPopup(aDOMElement, -1, -1, "popup", 'bottomleft', 'topleft'); in autocomplete.xml, where aDOMElement can be the html input element or the xul element. For the html element, the pop appears at the top left of the document, so something is going very wrong here. No idea where this is going wrong. Maybe the popup code only likes xul elements or something?

lets do this in stages, i suppose. First patch to make XUL popups which are near the bottom of the screen no cover the text field. After this lands, we can consider something like 191526: Unbitrotted patch for the branch (with a new IDL).

Comment on attachment 215205 [details] [diff] [review] fixes XUL popups (NO API CHANGE) I really don't know if this is the right fix, so landing this at the last minute for a1 doesn't seem right, will look at this again tomorrow.

agreed. i want the right fix. please advise.

Comment on attachment 215205 [details] [diff] [review] fixes XUL popups (NO API CHANGE) ok, this should be ok to start, let's get this on trunk and land on branch when the tree reopens

patch 215205 checked into the trunk. Checking in autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.xml new revision: 1.55; previous revision: 1.54 done

Checking in autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.xml new revision: 1.44.2.5; previous revision: 1.44.2.4 done

what should we do about the rest of this bug? Should we add the new interface for 1.8?

Well, it isn't the most elegant thing I did, but it works and I don't think it would cause any trouble. The ideal solution (if someone gets it to work) would be the suggestion in comment 15.

*** Bug 347596 has been marked as a duplicate of this bug. ***

*** Bug 348953 has been marked as a duplicate of this bug. ***

Ok, the suggestion in comment 15 seems to be working now in trunk.

Comment on attachment 282691 [details] [diff] [review] patch You didn't change the other consumer of nsIAutoCompletePopup :-P >- nsRect popupRect = GetScreenOrigin(mFocusedInput); This was the only caller... >+ this.popup.openAutocompletePopup(this); I'm tempted to suggest (this, this); > <parameter name="aInput"/> >- <parameter name="aX"/> >- <parameter name="aY"/> >- <parameter name="aWidth"/> >+ <parameter name="aInput2"/> I think that's a really bad choice of parameter name. >+ var width = input.getBoundingClientRect().right - input.getBoundingClientRect().left; Is getBoundingClientRect() the right API for this? Is it cheap enough to call twice? (I don't know; I would have tried aElement.ownerDocument.getBoxObjectfor(aElement).width)

Ok, I think this fixes most of what you commented upon. I haven't tested the SeaMonkey changes. (In reply to comment #41) > Is getBoundingClientRect() the right API for this? Is it cheap enough to call > twice? (I don't know; I would have tried > aElement.ownerDocument.getBoxObjectfor(aElement).width) In this patch it's now only called one time. I though getBoxObjectFor shouldn't be used for content other than xul. See bug 340571, so it seems to me I should not use that.

Comment on attachment 282769 [details] [diff] [review] patchv2 Just a quick look; I haven't tried it yet. > #include "nsISupports.idl" >+#include "nsIDOMElement.idl" > > interface nsIAutoCompleteInput; Sorry for not spotting this last time but you only need interface nsIDOMElement; > this.width = aWidth; I think this line can go too.

It's good you didn't try the previous patch yet, because it contained another serious error.

Comment on attachment 282787 [details] [diff] [review] patchv2, updated > <body> > if (!this.input) { > this.tree.view = aInput.controller; > this.view = this.tree.view; > this.showCommentColumn = aInput.showCommentColumn; > this.input = aInput; >- this.width = aWidth; > this.invalidate(); >- this.openPopupAtScreen(aX, aY, false); >+ >+ var rect = aElement.getBoundingClientRect(); >+ var width = rect.right - rect.left; >+ this.setAttribute("width", width < 100 ? 100 : width); You'll need to wrap this < in a <![CDATA ]]> block. Alternatively setting the minimum width in XUL or CSS might be a neater approach.

Yeah, good idea to use the minwidth attribute. I added it here to the <content> element, and I added the <![CDATA ]]> block, as you said.

Comment on attachment 282853 [details] [diff] [review] patchv2, updated Um... the whole point of "alternatively" is that you don't require both changes... (since there's no < to wrap in a <![CDATA[ ]] any more)

Yes, well, the rest of the code seems to wrap it in CDATA blocks (even if it's not necessary), so might as well do it for that part.

Checking in toolkit/components/satchel/src/nsFormFillController.cpp; /cvsroot/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp,v <-- nsFormFillController.cpp new revision: 1.91; previous revision: 1.90 done Checking in toolkit/content/widgets/autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.x ml new revision: 1.83; previous revision: 1.82 done Checking in toolkit/components/autocomplete/public/nsIAutoCompletePopup.idl; /cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompletePopup.idl ,v <-- nsIAutoCompletePopup.idl new revision: 1.6; previous revision: 1.5 done Checking in xpfe/components/autocomplete/resources/content/autocomplete.xml; /cvsroot/mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml ,v <-- autocomplete.xml new revision: 1.155; previous revision: 1.154 done Checked into trunk.

This might have caused bug 398154.

This caused bug 399316.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/55c7f52e0e4f autocomplete dropdown covers textbox when textbox is near bottom of screen, r=neil, a=mconnor