Closed Bug 1269333 Opened 5 years ago Closed 5 years ago

Back out bug 804968 now that bug 907001 landed

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

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.
Attachment #8748304 - Flags: review?(mak77)
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?
Blocks: 1257544
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/
Attachment #8748304 - Flags: review?(mak77)
Attachment #8748304 - Flags: review?(mak77)
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.
Jared, do you plan to complete this? I'd like to have it for Activity Stream work.
Flags: needinfo?(jaws)
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/
Attachment #8748304 - Attachment description: MozReview Request: Bug 1269333 - Don't assume the height of the first row for all other rows. r?mak → Bug 1269333 - Don't assume the height of the first row for all other rows.
Attachment #8748304 - Flags: review?(mak77)
Flags: needinfo?(jaws)
Attachment #8748304 - Flags: review?(mak77) → review+
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
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6f305ccfd776
Don't assume the height of the first row for all other rows. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f305ccfd776
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.