Closed Bug 216434 Opened 17 years ago Closed 13 years ago

autocomplete dropdown covers textbox when textbox is near bottom of screen

Categories

(Firefox :: Address Bar, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: martijn.martijn)

References

Details

Attachments

(3 files, 7 obsolete files)

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.
Severity: major → normal
Still happens with 09/02 on WinXP, even with a fresh profile.
I'm getting the same as Jesse but on Linux.
OS: Windows XP → All
OK, I see this now.
Flags: blocking1.0?
-ing will take patch
Flags: blocking1.0? → blocking1.0-
Attached patch Ugly patch (obsolete) — Splinter Review
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. ***
Assignee: hewitt → bugs
Attached patch JAR archive repairs this bug. (obsolete) — Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #155424 - Attachment is obsolete: true
Attachment #169206 - Flags: review?(dean_tessman)
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.
Attachment #169206 - Flags: review?(dean_tessman) → review-
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.
Attachment #169206 - Attachment is obsolete: true
Attachment #191526 - Flags: review?(dean_tessman)
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.

Assignee: bugs → nobody
QA Contact: davidpjames → location.bar
*** 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.
Assignee: nobody → dougt
Attached patch Fixes problem without API change (obsolete) — Splinter Review
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.
Attachment #168775 - Attachment is obsolete: true
Attachment #191526 - Attachment is obsolete: true
Attachment #214692 - Flags: review?(mconners)
Attachment #214692 - Flags: approval-branch-1.8.1?(mconners)
Attachment #191526 - Flags: review?(dean_tessman)
+                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;
Attachment #214692 - Attachment is obsolete: true
Attachment #214692 - Flags: review?(mconners)
Attachment #214692 - Flags: review-
Attachment #214692 - Flags: approval-branch-1.8.1?(mconners)
Attachment #214692 - Flags: approval-branch-1.8.1-
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)?
Attachment #191526 - Attachment is obsolete: false
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).
Attachment #215205 - Flags: review?(mconners)
Attachment #215205 - Flags: approval-branch-1.8.1?(mconners)
Attachment #215205 - Flags: review?(mconnor)
Attachment #215205 - Flags: review?(mconners)
Attachment #215205 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #215205 - Flags: approval-branch-1.8.1?(mconners)
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
Attachment #215205 - Flags: review?(mconnor) → review+
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
Attachment #215205 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
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. ***
Attached patch patch (obsolete) — Splinter Review
Ok, the suggestion in comment 15 seems to be working now in trunk.
Attachment #282691 - Flags: review?(neil)
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)
Attached patch patchv2 (obsolete) — Splinter Review
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.
Attachment #282691 - Attachment is obsolete: true
Attachment #282769 - Flags: review?(neil)
Attachment #282691 - Flags: review?(neil)
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.
Attached patch patchv2, updated (obsolete) — Splinter Review
It's good you didn't try the previous patch yet, because it contained another serious error.
Attachment #282787 - Flags: review?(neil)
Attachment #282769 - Attachment is obsolete: true
Attachment #282769 - Flags: review?(neil)
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.
Attachment #282787 - Flags: review?(neil) → review+
Attached patch patchv2, updatedSplinter Review
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.
Attachment #282787 - Attachment is obsolete: true
Attachment #282853 - Flags: review?(neil)
Attachment #282853 - Attachment is patch: true
Attachment #282853 - Attachment mime type: application/octet-stream → text/plain
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)
Attachment #282853 - Flags: review?(neil) → review+
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.
Attachment #282853 - Flags: approval1.9?
Attachment #282853 - Flags: approval1.9? → approval1.9+
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.
Assignee: dougt → martijn.martijn
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This might have caused bug 398154.
This caused bug 399316.
Depends on: 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
You need to log in before you can comment on or make changes to this bug.