Preferences search is janky and unresponsive

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Preferences
P3
normal
VERIFIED FIXED
4 months ago
a month ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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

4 months ago
Whiteboard: [reserve-photon-performance][photon-preference][triage] → [reserve-photon-performance][photon-preference]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

4 months 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-
Attachment #8893293 - Flags: feedback?(jaws)
(Assignee)

Comment 5

4 months 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)
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)
Now that's ready for review.
Comment hidden (mozreview-request)
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 13

4 months 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

4 months 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

3 months ago
Whiteboard: [reserve-photon-performance][photon-preference] → [photon-preference]

Comment 16

3 months 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

3 months 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)
This is ready to land after I split the tests. :'(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f226ac09cb49d9cfe34ae963dc2e92e0123f68cc
Comment hidden (mozreview-request)
(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.
Keywords: checkin-needed

Comment 24

3 months 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)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed

Comment 26

3 months 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)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

3 months 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
https://hg.mozilla.org/mozilla-central/rev/1906ad77a237
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months 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

Updated

a month ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.