Closed Bug 1384830 Opened 7 years ago Closed 7 years ago

Preferences search is janky and unresponsive

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: timdream, Assigned: timdream)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1384830 - Show search result as the user types in Preferences search,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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.
Whiteboard: [reserve-photon-performance][photon-preference][triage] → [reserve-photon-performance][photon-preference]
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 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-
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 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)
Now that's ready for review.
Attachment #8893293 - Flags: review?(jaws)
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)
Lint error should be fixed after 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 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".
Whiteboard: [reserve-photon-performance][photon-preference] → [photon-preference]
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+
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.
This is ready to land after I split the tests. :'(
(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.
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/1906ad77a237
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
QA Contact: Virtual
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: