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)
Toolkit
Autocomplete
Tracking
()
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.
Assignee | ||
Comment 1•7 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•7 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•7 years ago
|
Priority: -- → P1
Whiteboard: [photon] → [photon][fxsearch]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•7 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•7 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)
Comment 7•7 years 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•7 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•7 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•7 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•7 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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/609c12db5796
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Reporter | ||
Updated•7 years ago
|
No longer blocks: photon-performance-triage
Reporter | ||
Updated•7 years ago
|
Blocks: photon-performance-triage
You need to log in
before you can comment on or make changes to this bug.
Description
•