857441 - Autocompletion popup is very jumpy while typing text

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: screencast

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

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: 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.

Comment 2

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=fc01c3092e0e

Comment 3

[Mihai Sucan [:msucan]]

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.

Comment 4

[Victor Porof [:vporof][:vp]]

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

Comment 5

[Girish Sharma [:Optimizer]]

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.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/f6d26cee5c6c

Comment 7

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/f6d26cee5c6c