1269333 - Back out bug 804968 now that bug 907001 landed

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

I landed 804968 because of long data URIs in switch-to-tab causing autocomplete to take a long time. This was because we were using getBoundingClientRect to compute the height but it also computes the width.

Now that bug 907001 landed we'll never get text results longer than 255 characters, so we can back out bug 804968. That will in turn fix bug 901946 and allow Activity Stream to use rows of different heights.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1269333 - Don't assume the height of the first row for all other rows.

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff38c7af9e0c

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8748304 [details]
Bug 1269333 - Don't assume the height of the first row for all other rows.

https://reviewboard.mozilla.org/r/50207/#review47235

::: toolkit/content/widgets/autocomplete.xml:1177
(Diff revision 1)
> -              let transition = style.transitionProperty;
> +            let transition = style.transitionProperty;
> -              this._rlbAnimated = transition && transition != "none";
> +            this._rlbAnimated = transition && transition != "none";
>  
> -              let paddingTop = parseInt(style.paddingTop) || 0;
> +            let paddingTop = parseInt(style.paddingTop) || 0;
> -              let paddingBottom = parseInt(style.paddingBottom) || 0;
> +            let paddingBottom = parseInt(style.paddingBottom) || 0;
> -              this._rlbPadding = paddingTop + paddingBottom;
> +            this._rlbPadding = paddingTop + paddingBottom;

Both _rlbAnimated and _rlbPadding are something that we'd like to calculate just once, rather than at every adjustHeight, cause they are unlikely to change. That's why they were behind the _rowHeight check.
Maybe we could check this._rlbPadding === undefined to do that?

And I'd move firstRowRect and lastRowRect closer to their first use.

::: toolkit/content/widgets/autocomplete.xml:1184
(Diff revision 1)
> -              // Set a fixed max-height to avoid flicker when growing the panel.
> +            // Set a fixed max-height to avoid flicker when growing the panel.
> +            let rowHeights = Array.prototype.map.call(rows,
> +              (node) => node.getBoundingClientRect().height);
> +            let maxRowHeight = Math.max.apply(null, rowHeights);
> -              this.richlistbox.style.maxHeight =
> +            this.richlistbox.style.maxHeight =
> -                ((this._rowHeight * this.maxRows) + this._rlbPadding) + "px";
> +              ((maxRowHeight * this.maxRows) + this._rlbPadding) + "px";

I *think* this is wrong.
What this tries to prevent is the panel growing over the limit it is supposed to show and then reducing.
This is probably only visible if maxRichResults is set to a number bigger than 10 and a scrollbar is visible.
So previously it was calculating the maximum size we'd have shown (rowHeight * maxRows), only once.
Here we are resetting it at every adjustHeight, so I guess we could try to use the real value.

Basically we could probably set it only if numRows > maxRows and set it to height of the first maxRows rows.

Does this make sense?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8748304 [details]
Bug 1269333 - Don't assume the height of the first row for all other rows.

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

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8748304 [details]
Bug 1269333 - Don't assume the height of the first row for all other rows.

https://reviewboard.mozilla.org/r/50207/#review48795

::: toolkit/content/widgets/autocomplete.xml:1168
(Diff revision 2)
>            // Default the height to 0 if we have no rows to show
>            let height = 0;
>            if (numRows) {
> -            if (!this._rowHeight) {
> -              let firstRowRect = rows[0].getBoundingClientRect();
> +            let firstRowRect = rows[0].getBoundingClientRect();
> -              this._rowHeight = firstRowRect.height;
> +            if (!this._rlbPadding) {

what if the padding is set to 0, isnt that a valid value? shouldnt we explicitly check for undefined instead?

::: toolkit/content/widgets/autocomplete.xml:1178
(Diff revision 2)
>  
>                let paddingTop = parseInt(style.paddingTop) || 0;
>                let paddingBottom = parseInt(style.paddingBottom) || 0;
>                this._rlbPadding = paddingTop + paddingBottom;
>  
> +              if (numRows > this.maxRows) {

AFAICT this should happen outside the this._rlbPadding check, since we need to recalculate maxHeight at every adjust, not just once.

::: toolkit/content/widgets/autocomplete.xml:1180
(Diff revision 2)
>                let paddingBottom = parseInt(style.paddingBottom) || 0;
>                this._rlbPadding = paddingTop + paddingBottom;
>  
> +              if (numRows > this.maxRows) {
> -              // Set a fixed max-height to avoid flicker when growing the panel.
> +                // Set a fixed max-height to avoid flicker when growing the panel.
> +                let lastVisibleRowRect = rows[this.maxRows].getBoundingClientRect();

maxRows - 1? for a maxrows of 10, the last row is at index 9.

Comment 6

[Marco Bonardo [:mak]]

Jared, do you plan to complete this? I'd like to have it for Activity Stream work.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8748304 [details]
Bug 1269333 - Don't assume the height of the first row for all other rows.

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

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8748304 [details]
Bug 1269333 - Don't assume the height of the first row for all other rows.

https://reviewboard.mozilla.org/r/50207/#review56332

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6f305ccfd776
Don't assume the height of the first row for all other rows. r=mak

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f305ccfd776