autocomplete dropdown covers textbox when textbox is near bottom of screen

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
a year ago

People

(Reporter: jruderman, Assigned: martijn.martijn)

Tracking

unspecified
x86
All
Points:
---
Bug Flags:
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

16 years ago
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
(Reporter)

Comment 2

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

Comment 3

16 years ago
I'm getting the same as Jesse but on Linux.
OS: Windows XP → All
OK, I see this now.

Updated

15 years ago
Flags: blocking1.0?
-ing will take patch
Flags: blocking1.0? → blocking1.0-
(Assignee)

Comment 6

15 years ago
Created attachment 155424 [details] [diff] [review]
Ugly 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).

Comment 7

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

Updated

14 years ago
Assignee: hewitt → bugs

Comment 8

14 years ago
Created attachment 168775 [details] [diff] [review]
JAR archive repairs 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.
(Assignee)

Comment 9

14 years ago
Created attachment 169206 [details] [diff] [review]
Patch

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
(Assignee)

Updated

14 years ago
Attachment #169206 - Flags: review?(dean_tessman)
(Assignee)

Comment 10

14 years ago
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 11

14 years ago
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.
(Assignee)

Comment 12

14 years ago
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.

Updated

14 years ago
Attachment #169206 - Flags: review?(dean_tessman) → review-

Comment 13

14 years ago
Created attachment 191526 [details] [diff] [review]
Unbitrotted patch

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)
(Assignee)

Comment 14

14 years ago
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...
(Assignee)

Comment 17

14 years ago
(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
(Assignee)

Comment 18

14 years ago
*** 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
Created attachment 214692 [details] [diff] [review]
Fixes problem without API change

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)
(Assignee)

Comment 22

13 years ago
+                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)?

Updated

13 years ago
Attachment #191526 - Attachment is obsolete: false
Martijn | Neil
are you happy with the Tim's unbitrotted patch? (other than the comment)
(Assignee)

Comment 26

13 years ago
(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.
(Assignee)

Comment 27

13 years ago
(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.  
(Assignee)

Comment 29

13 years ago
(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?
Created attachment 215205 [details] [diff] [review]
fixes XUL popups (NO API CHANGE)

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?
(Assignee)

Comment 37

13 years ago
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.
(Assignee)

Comment 38

13 years ago
*** Bug 347596 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 39

13 years ago
*** Bug 348953 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 40

12 years ago
Created attachment 282691 [details] [diff] [review]
patch

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)
(Assignee)

Comment 42

12 years ago
Created attachment 282769 [details] [diff] [review]
patchv2

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.
(Assignee)

Comment 44

12 years ago
Created attachment 282787 [details] [diff] [review]
patchv2, updated

It's good you didn't try the previous patch yet, because it contained another serious error.
Attachment #282787 - Flags: review?(neil)
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 46

12 years ago
Created attachment 282853 [details] [diff] [review]
patchv2, updated

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)
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 48

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #282853 - Flags: approval1.9?
Attachment #282853 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 49

12 years ago
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
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 50

12 years ago
This might have caused bug 398154.

Comment 51

12 years ago
This caused bug 399316.

Updated

12 years ago
Depends on: 399316

Comment 52

a year ago
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.