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

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years 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])

Attachments

(1 attachment)

Reporter

Description

2 years 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

2 years ago
See Also: → 1353563
Assignee

Comment 1

2 years 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

2 years 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

2 years ago
Priority: -- → P1
Whiteboard: [photon] → [photon][fxsearch]
Reporter

Updated

2 years ago
Blocks: 1353725
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 5

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter

Updated

2 years ago
See Also: → 1356763
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]
Flags: qe-verify? → qe-verify-
See Also: → 1354194
Reporter

Updated

2 years ago
No longer blocks: photon-performance-triage
You need to log in before you can comment on or make changes to this bug.