Closed Bug 1353708 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Autocomplete, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Iteration:
55.3 - Apr 17
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: mak)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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.
See Also: → 1353563
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
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.
Priority: -- → P1
Whiteboard: [photon] → [photon][fxsearch]
Blocks: 1353725
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)
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...
(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 < ?
(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 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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
No longer blocks: photon-performance-triage
You need to log in before you can comment on or make changes to this bug.