Autocompletion popup is very jumpy while typing text

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Unassigned)

Tracking

unspecified
Firefox 23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 732658 [details]
screencast

Most likely because there are some very ugly hacks in AP__updateSize. Maybe we can do better.
(Reporter)

Comment 1

5 years ago
Created attachment 732659 [details] [diff] [review]
v1

This seems to do the trick.

There were a few typos in there as well. Most notably:
> this._list.height = this._panel.clientHeight;
It should be _list.clientHeight if we want to preserve the actual height and not incrementally enlarge it with every key press.
Attachment #732659 - Flags: review?(mihai.sucan)
(Reporter)

Comment 2

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fc01c3092e0e
Status: NEW → ASSIGNED
Comment on attachment 732659 [details] [diff] [review]
v1

Review of attachment 732659 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch. I can't notice the problem you report on my system (Linux). What I see is a bit of popup flickering every time I type something. That's not fixed with this patch.

::: browser/devtools/shared/AutocompletePopup.jsm
@@ +84,4 @@
>      this._panel.hidePopup();
>    }
>  
> +  this._list.setAttribute("flex", "1");

Is this change needed? I had toolkit/browser reviewers asking me to change foo.setAttribute("flex", "1") to foo.flex.

@@ +261,3 @@
>        // Height change is required, otherwise the panel is drawn at an offset
>        // the first time.
> +      this._list.height = this._list.clientHeight;

I'm unsure about this change, but it works for me (Linux here), and if it works for you as well, then this is fine.

@@ +265,5 @@
>        this._list.top = 0;
>        // Changing panel height might make the selected item out of view, so
>        // bring it back to view.
>        this._list.ensureIndexIsVisible(this._list.selectedIndex);
> +    }}, 0);

Afaik 5ms was there for a purpose, back when I wrote the code. I tried 0 and it didn't work *back then*. I can't notice any regression now. We leave this 0 now.
Attachment #732659 - Flags: review?(mihai.sucan) → review+
(Reporter)

Comment 4

5 years ago
(In reply to Mihai Sucan [:msucan] from comment #3)
> 
> Thanks for your patch. I can't notice the problem you report on my system
> (Linux). What I see is a bit of popup flickering every time I type
> something. That's not fixed with this patch.
> 

That's because we're setting the list width to take scrollbars into account. I can't think of any non-incredibly hacky way of doing this.

> ::: browser/devtools/shared/AutocompletePopup.jsm
> @@ +84,4 @@
> >      this._panel.hidePopup();
> >    }
> >  
> > +  this._list.setAttribute("flex", "1");
> 
> Is this change needed? I had toolkit/browser reviewers asking me to change
> foo.setAttribute("flex", "1") to foo.flex.
> 

Probably not.

> @@ +261,3 @@
> >        // Height change is required, otherwise the panel is drawn at an offset
> >        // the first time.
> > +      this._list.height = this._list.clientHeight;
> 
> I'm unsure about this change, but it works for me (Linux here), and if it
> works for you as well, then this is fine.
> 

It was stupid to begin with.

<rant>
The list is contained in a panel. Therefore the panel height is by definition >= than the list height. In practice, it was a few pixels larger. Therefore, without this change, the panel height was increasing a few pixels in height every time a key was pressed and the timeout hit.
</rant>

> @@ +265,5 @@
> >        this._list.top = 0;
> >        // Changing panel height might make the selected item out of view, so
> >        // bring it back to view.
> >        this._list.ensureIndexIsVisible(this._list.selectedIndex);
> > +    }}, 0);
> 
> Afaik 5ms was there for a purpose, back when I wrote the code. I tried 0 and
> it didn't work *back then*. I can't notice any regression now. We leave this
> 0 now.

This is not a timeout anymore, it's a thread dispatch.

The 0 there is a constant === Ci.nsIThread.DISPATCH_NORMAL. The other value is Ci.nsIThread.DISPATCH_SYNC which is useless here.
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
Even I don't see the jumping on Windows. The only flickering that I see for a millisecond is the width being adjusted to remove the scrollbar need. The same thing that Mihai mentioned.
https://hg.mozilla.org/integration/fx-team/rev/f6d26cee5c6c
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
(Reporter)

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f6d26cee5c6c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.