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
21 days ago
7 days 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

21 days 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

21 days ago
See Also: → bug 1353563
(Assignee)

Comment 1

21 days 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

21 days 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

21 days ago
Priority: -- → P1
Whiteboard: [photon] → [photon][fxsearch]
(Reporter)

Updated

21 days ago
Blocks: 1353725
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

20 days 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

20 days 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)
(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

20 days 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

20 days 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

19 days 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

19 days 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: 18 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

11 days ago
See Also: → bug 1356763
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]
Flags: qe-verify? → qe-verify-
See Also: → bug 1354194
You need to log in before you can comment on or make changes to this bug.