autocomplete dropdown covers textbox when textbox is near bottom of screen

RESOLVED FIXED

Status

()

Firefox
Location Bar
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: Martijn Wargers (dead))

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

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

Comment 3

14 years ago
I'm getting the same as Jesse but on Linux.
OS: Windows XP → All

Comment 4

14 years ago
OK, I see this now.

Updated

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

Comment 6

13 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

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

Updated

13 years ago
Assignee: hewitt → bugs

Comment 8

13 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

13 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

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

Comment 10

13 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

13 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

13 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

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

Comment 13

12 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

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

Comment 15

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

Comment 16

12 years ago
Although I don't really know anything about toolkit autocomplete...
(Assignee)

Comment 17

12 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.

Updated

12 years ago
Assignee: bugs → nobody
QA Contact: davidpjames → location.bar
(Assignee)

Comment 18

12 years ago
*** Bug 309274 has been marked as a duplicate of this bug. ***
*** Bug 292831 has been marked as a duplicate of this bug. ***

Comment 20

11 years ago
I need this to work, so accepting.
Assignee: nobody → dougt

Comment 21

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

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

Comment 23

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

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

11 years ago
Attachment #191526 - Attachment is obsolete: false

Comment 25

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

Comment 26

11 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

11 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.

Comment 28

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

11 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?

Comment 30

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

Comment 32

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

Comment 34

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

Updated

11 years ago
Attachment #215205 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+

Comment 35

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

Comment 36

11 years ago
what should we do about the rest of this bug?  Should we add the new interface for 1.8?
(Assignee)

Comment 37

11 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

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

Comment 39

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

Comment 40

10 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 41

10 years ago
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

10 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 43

10 years ago
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

10 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

10 years ago
Attachment #282769 - Attachment is obsolete: true
Attachment #282769 - Flags: review?(neil)

Comment 45

10 years ago
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

10 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

10 years ago
Attachment #282853 - Attachment is patch: true
Attachment #282853 - Attachment mime type: application/octet-stream → text/plain

Comment 47

10 years ago
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

10 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

10 years ago
Attachment #282853 - Flags: approval1.9?

Updated

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

Comment 49

10 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

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

Comment 50

10 years ago
This might have caused bug 398154.

Comment 51

10 years ago
This caused bug 399316.

Updated

10 years ago
Depends on: 399316
You need to log in before you can comment on or make changes to this bug.