Closed
Bug 1384830
Opened 7 years ago
Closed 7 years ago
Preferences search is janky and unresponsive
Categories
(Firefox :: Settings UI, enhancement, P3)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: timdream, Assigned: timdream)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-preference])
Attachments
(1 file)
While working on bug 1382170, I realized the Preferences search is quite janky and not responsive. One thing to change right away is to use "input" event instead of "command" event, which has a built-in timeout to batch multiple keyboard events. This change will be a quick win, but it will make the jank more visible. The jank obviously comes from searchFunction(), because it naively mutates the DOM a lot without care for its own frame budget. This will require a bit of work to improve, essentially, I would need to break it up into little pieces and wrap it inside requestAnimationFrame(). For each rAF() call the pieces run should not take more than 10ms. This can be measured with Performance Timing. As things become async, each input would need to cancel the previous run properly.
Updated•7 years ago
|
Whiteboard: [reserve-photon-performance][photon-preference][triage] → [reserve-photon-performance][photon-preference]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, Tests are failing :'(. Let me know if you have any feedback first.
Attachment #8893293 -
Flags: review?(jaws) → feedback?(jaws)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review169754 (marking as r- because mozreview doesn't handle cleared review requests well. in my experience, once a review request is cleared pushing to mozreview doesn't request review anymore) ::: browser/components/preferences/in-content-new/findInPage.js:240 (Diff revision 2) > // Building the range for highlighted areas > let rootPreferencesChildren = document > .querySelectorAll("#mainPrefPane > *:not([data-hidden-from-search])"); > > - // Showing all the children to bind JS, Access Keys, etc > + if (subQuery) { > + // Since the previous query is a substrying of the curreny query, s/substrying/subset/ ::: browser/components/preferences/in-content-new/findInPage.js:258 (Diff revision 2) > + let ts = performance.now(); > + let FRAME_THRESHOLD = 1000 / 60; > + > // Showing or Hiding specific section depending on if words in query are found > for (let i = 0; i < rootPreferencesChildren.length; i++) { > + if (performance.now() - ts > FRAME_THRESHOLD) { I'm a little confused by this. If the current iteration is longer than 16ms, then we will start to build tooltips? ::: browser/components/preferences/in-content-new/findInPage.js:259 (Diff revision 2) > + let FRAME_THRESHOLD = 1000 / 60; > + > // Showing or Hiding specific section depending on if words in query are found > for (let i = 0; i < rootPreferencesChildren.length; i++) { > + if (performance.now() - ts > FRAME_THRESHOLD) { > + // Creating tooltips for all the instances found Why are we doing this in two places? See line 300. Building the tooltips in the middle of the search will be very expensive layout wise since we will start invalidating the layout and then make further calculations at the next iteration. ::: browser/components/preferences/in-content-new/findInPage.js:260 (Diff revision 2) > + > // Showing or Hiding specific section depending on if words in query are found > for (let i = 0; i < rootPreferencesChildren.length; i++) { > + if (performance.now() - ts > FRAME_THRESHOLD) { > + // Creating tooltips for all the instances found > + this.listSearchTooltips.forEach((anchorNode) => this.createSearchTooltip(anchorNode, this.query)); While you're here, please change .forEach to use for-of. See https://bugzilla.mozilla.org/show_bug.cgi?id=1355874#c8 and https://bugzilla.mozilla.org/show_bug.cgi?id=1383362 ::: browser/components/preferences/in-content-new/findInPage.js:477 (Diff revision 2) > - this.listSearchTooltips.forEach((anchorNode) => anchorNode.tooltipNode.remove()); > + this.listSearchTooltips.forEach((anchorNode) => { > + anchorNode.tooltipNode.remove(); Same comment about .forEach
Attachment #8893293 -
Flags: review-
Updated•7 years ago
|
Attachment #8893293 -
Flags: feedback?(jaws)
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review170162 ::: browser/components/preferences/in-content-new/findInPage.js:259 (Diff revision 2) > + let FRAME_THRESHOLD = 1000 / 60; > + > // Showing or Hiding specific section depending on if words in query are found > for (let i = 0; i < rootPreferencesChildren.length; i++) { > + if (performance.now() - ts > FRAME_THRESHOLD) { > + // Creating tooltips for all the instances found I would like to build tooltips for elements that's already found so they show up at this frame. They will be early returned when we do it for the last time in line 300. I understand it's expensive but it doesn't seem to be something avoidable. Unless we don't want to show tooltips at this frame...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, This is not ready for review -- still have on functional issue caught by test failure to address (see TODO in the patch).
Attachment #8893293 -
Flags: review?(jaws)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Now that's ready for review.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8893293 -
Flags: review?(jaws)
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, (I am very confused on how MozReview works...)
Attachment #8893293 -
Flags: review?(jaws)
Assignee | ||
Comment 12•7 years ago
|
||
Lint error should be fixed after review.
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review170306 Testing this patch, clicking the X in the search box should clear the search and return to the #general category. With this patch, it just clears the input box but the search results stay on screen. ::: browser/components/preferences/in-content-new/findInPage.js:213 (Diff revisions 2 - 5) > * @param String event > * to search for filted query in > */ > async searchFunction(event) { > let query = event.target.value.trim().toLowerCase(); > - let subQuery = false; > + let subQuery = (this.query && query.indexOf(this.query) !== -1); nit, please remove the surrounding parens here. ::: browser/components/preferences/in-content-new/findInPage.js:238 (Diff revisions 2 - 5) > // Building the range for highlighted areas > let rootPreferencesChildren = document > .querySelectorAll("#mainPrefPane > *:not([data-hidden-from-search])"); > > if (subQuery) { > - // Since the previous query is a substrying of the curreny query, > + // Since the previous query is a subset of the curreny query, s/curreny/current/
Attachment #8893293 -
Flags: review?(jaws) → review-
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review170312 ::: browser/components/preferences/in-content-new/findInPage.js:21 (Diff revision 5) > } > this.inited = true; > this.searchInput = document.getElementById("searchInput"); > this.searchInput.hidden = !Services.prefs.getBoolPref("browser.preferences.search"); > if (!this.searchInput.hidden) { > - this.searchInput.addEventListener("command", this); > + this.searchInput.addEventListener("input", this); This change from "command" to "input" is what broke clearing the search field and returning to #general. Clicking on the "X" button doesn't generate an "input" event, but it does generate a "command" event. You may still need to add an event listener for the "command" event. I'm not sure if you will need to filter the "command" events so you don't run searches twice since you're now listening for "input".
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Whiteboard: [reserve-photon-performance][photon-preference] → [photon-preference]
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review171972 ::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_1.js:319 (Diff revisions 5 - 6) > + > + is_element_hidden(generalPane, "Should not be in general"); > + > + // Performs search > + let searchInput = gBrowser.contentDocument.getElementById("searchInput"); > + is(searchInput, gBrowser.contentDocument.activeElement.closest("#searchInput"), I don't think the call to .closest() here is necessary, since searchInput is the element with that ID and .closest is looking for the ancestor with that ID. I would understand if this was dealing with an anonymous content that was a descendent of #searchInput.
Attachment #8893293 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893293 [details] Bug 1384830 - Show search result as the user types in Preferences search, https://reviewboard.mozilla.org/r/164350/#review171972 > I don't think the call to .closest() here is necessary, since searchInput is the element with that ID and .closest is looking for the ancestor with that ID. > > I would understand if this was dealing with an anonymous content that was a descendent of #searchInput. The `closest()` here is needed because `document.activeElement` indeed points to `<html:input type="search">` within.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
This is ready to land after I split the tests. :'(
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f226ac09cb49d9cfe34ae963dc2e92e0123f68cc
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #17) > Comment on attachment 8893293 [details] > Bug 1384830 - Show search result as the user types in Preferences search, > > https://reviewboard.mozilla.org/r/164350/#review171972 > > > I don't think the call to .closest() here is necessary, since searchInput is the element with that ID and .closest is looking for the ancestor with that ID. > > > > I would understand if this was dealing with an anonymous content that was a descendent of #searchInput. > > The `closest()` here is needed because `document.activeElement` indeed > points to `<html:input type="search">` within. Okay makes sense, thanks for correcting me.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 24•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 8ba7a0774964 -d 75509cda693e: rebasing 413053:8ba7a0774964 "Bug 1384830 - Show search result as the user types in Preferences search, r=jaws" (tip) merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_4.js merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js and browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js to browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js merging browser/themes/shared/incontentprefs/preferences.inc.css warning: conflicts while merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 26•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 71cc065b9165 -d 33901272db44: rebasing 413210:71cc065b9165 "Bug 1384830 - Show search result as the user types in Preferences search, r=jaws" (tip) merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_4.js merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js and browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js to browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js warning: conflicts while merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_2.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_6.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1906ad77a237 Show search result as the user types in Preferences search, r=jaws
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1906ad77a237
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 31•7 years ago
|
||
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-14), so I'm marking this bug as VERIFIED. Thanks.
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•