richlistbox's ensureElementIsVisible causes a layout flush that contributes to the jank while opening the awesomebar panel

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Autocomplete
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: florian, Assigned: mak)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance] [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
See this profile: https://perf-html.io/public/d795e2cda214b8156da0d504d52750fd0f03c241/calltree/?callTreeFilters=prefixjs-FBqQpFBsFC0FC1FYoFDdFDeFDi&jsOnly&range=10.1061_10.6607~10.1284_10.1776&thread=0 taken on a slow laptop with an i3 CPU.

I think it should be possible to special case selecting the first item, to avoid doing the getBoundingClientRect calls when the scrollbar is already at the top.
(Reporter)

Updated

3 months ago
See Also: → bug 1353563
(Assignee)

Comment 1

3 months ago
I think we can fix this for the autocomplete case itself. I have an idea for that.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Component: XUL Widgets → Autocomplete
(Assignee)

Comment 2

3 months ago
I think it's risky to touch richlistbox cause we may break some edge-cases. And that's why I suspect it would be better to fix its consumer, the autocomplete widget.
Actually in some cases the autocomplete RLB cannot have scrollbars, and in those cases any ensureElementIsVisible can be avoided since they are always visible.
(Assignee)

Updated

3 months ago
Priority: -- → P1
Whiteboard: [photon] → [photon][fxsearch]
(Reporter)

Updated

3 months ago
Blocks: 1353725
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

3 months ago
Marco, could you please explain to me why "if (this.maxRows < this.maxResults) {" ensures there's no scrollbar?

Also, what values do you expect there? Searching quickly gave me 10 for maxrows (http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/browser/base/content/browser.xul#691) and 10 for maxResults (http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/browser/app/profile/firefox.js#274)
Flags: needinfo?(mak77)
(Assignee)

Comment 6

3 months ago
Sorry, I should have added that detail in the commit message.

this.maxRows is the maximum number of rows to show AT ONCE in the autocomplete popup, for example for the awesomebar it's 10. If we have more than maxRows results then there's a scrollbar.

this.maxResults is the maximum number of results that we will fetch from the search to populate the popup.

It's clear that if we can fetch less or equal than the maximum number of rows we show at once, there won't be a scrollbar, since we can show all the results we could fetch.

For the awesomebar both of those values are 10.
Flags: needinfo?(mak77)

Comment 7

3 months ago
(In reply to Marco Bonardo [::mak] from comment #6)
> Sorry, I should have added that detail in the commit message.
> 
> this.maxRows is the maximum number of rows to show AT ONCE in the
> autocomplete popup, for example for the awesomebar it's 10. If we have more
> than maxRows results then there's a scrollbar.
> 
> this.maxResults is the maximum number of results that we will fetch from the
> search to populate the popup.
> 
> It's clear that if we can fetch less or equal than the maximum number of
> rows we show at once, there won't be a scrollbar, since we can show all the
> results we could fetch.
> 
> For the awesomebar both of those values are 10.

A trimmed down version of that might be useful as code comment too...
(Reporter)

Comment 8

3 months ago
(In reply to Dão Gottwald [::dao] from comment #7)

> A trimmed down version of that might be useful as code comment too...

+1

Also, should it be <= rather than < ?
(Assignee)

Comment 9

3 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Also, should it be <= rather than < ?

No, compared to what I said in comment 6, the code is inverted (it figures out when there CAN be a scrollbar, while in comment 6 I say when there can't be a scrollbar).
I'll add a comment.
Comment hidden (mozreview-request)
(Reporter)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8854958 [details]
Bug 1353708 - Avoid autocomplete call to richlistbox's ensureElementIsVisible when there can't be a scrollbar.

https://reviewboard.mozilla.org/r/126876/#review130340

Thanks!
Attachment #8854958 - Flags: review?(florian) → review+

Comment 12

3 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/609c12db5796
Avoid autocomplete call to richlistbox's ensureElementIsVisible when there can't be a scrollbar. r=florian
https://hg.mozilla.org/mozilla-central/rev/609c12db5796
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

2 months ago
See Also: → bug 1356763

Updated

2 months ago
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]

Updated

2 months ago
Flags: qe-verify? → qe-verify-

Updated

2 months ago
See Also: → bug 1354194
(Reporter)

Updated

2 months ago
No longer blocks: 1348289
(Reporter)

Updated

2 months ago
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.