1470103 - Autocomplete popup width is too small

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Julian Descottes [:jdescottes]]

STRs:
- open console
- run `var w = window`
- type `w` then `.` (don't simply paste `w.`)

The autocomplete popup is too narrow to show the suggestions full width. 
It seems linked to the fact that when typing `w` the autocomplete is already displayed for "w" and "window" and doesn't need to have a big width. 

Also occurs on release, not a recent regression.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: autocomplete-too-narrow.png

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Let's try to fix this as part of the jsterm improvements

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

This looks like the combination of 2 things:

1. When calling setItems in the autocomplete popup, we don't call updateSize, which computes the maximum number of chars of a label, and set it as the width of the inner `_list` element. updateSize is only called when calling openPopup, which is not called in the STR from Comment 0.
2. Even if we call updateSize in setItems, it gets overridden by the explicit width the HTMLTooltip is setting (https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/devtools/client/shared/widgets/tooltip/HTMLTooltip.js#545). I guess we should re-trigger updateContainerBounds when setting items as well.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1470103 - Fix autocomplete popup sizing issue in the console; .

If the popup is already opened and the consumer
set its items, the popup wasn't resized, which
could lead to clipped popup items.
We now make sure to properly resize the popup
when setting items.
A test is added to make sure that the popup
reacts to its content change.

Review commit: https://reviewboard.mozilla.org/r/258346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258346/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

The test isn't ideal, but it does fail without the fix in the code.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8993669 [details]
Bug 1470103 - Fix autocomplete popup sizing issue in the console; .

removing flag since there are failure in TRY run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a17f1bd91b96d7230c9aba7264ddf0fcde440e93&selectedJob=189197506)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8993669 [details]
Bug 1470103 - Fix autocomplete popup sizing issue in the console; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258346/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8993669 [details]
Bug 1470103 - Fix autocomplete popup sizing issue in the console; .

https://reviewboard.mozilla.org/r/258346/#review265672

Nice, fixes the issue and glad we have a test.

::: devtools/client/webconsole/components/JSTerm.js:1292
(Diff revision 2)
>        }
>  
>        if (popupAlignElement) {
>          popup.openPopup(popupAlignElement, xOffset, yOffset);
>        }
>      } else if (items.length === 0 && popup.isOpen) {

In theory this could now be `else if (popup.isOpen)` since we know that items is an array, and length is not > 0 ... but I actually think it's better to keep it. Helps parsing the code quickly :)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8993669 [details]
Bug 1470103 - Fix autocomplete popup sizing issue in the console; .

https://reviewboard.mozilla.org/r/258346/#review265672

> In theory this could now be `else if (popup.isOpen)` since we know that items is an array, and length is not > 0 ... but I actually think it's better to keep it. Helps parsing the code quickly :)

yes, that was my thought since the if block is a bit "big"

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59adeffe6843
Fix autocomplete popup sizing issue in the console; r=jdescottes.

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/59adeffe6843