WordPress.org

Make WordPress Core

#42343 closed defect (bug) (fixed)

Customize: Themes search debounce is not debounced properly

Reported by: afercia Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch
Focuses: ui, javascript Cc:

Description

Debouncing a search, i.e. not triggering a search while users are still typing the search term, has a few advantages and WordPress is already using this pattern in a few places.

The new Customize Theme search, especially for the .org themes, tries to debounce the search but as far as I see the current code fails to debounce properly. See https://core.trac.wordpress.org/browser/trunk/src/wp-admin/js/customize-controls.js?rev=42025&marks=1837-1846#L1837

This code always calls checkTerm(), the first time is a debounced call and the second time is a direct call.

To reproduce:

  • go in the customizer > change theme > WordPress.org themes
  • type something in the search field at a normal speed
  • observe the search gets triggered for any entered character

The second call to checkTerm() runs always, for each entered character. Also, when typing slowly, checkTerm() is always called twice.

To reproduce:

  • dump some console.log() inside checkTerm()
  • type 10 characters in the search field
  • observe your console dumping your console.log() 10 times + 1 time after 500ms

or

  • enter 1 character and wait for at least 500ms
  • enter a second character
  • observe for each typed character, checkTerm() runs twice

Observing the network panel in Chrome, multiple AJAX requests happen while typing. Seems to me loadThemes() helps a bit reducing the actual number of requests. However, requests should not be sent at all while typing.

Ideally, nothing should happen while typing. The search should be triggered only after users have finished typing and 500ms have passed. As I see it, this is a bug in trunk and should be fixed for 4.9.

Additionally, these kind of searches should be standardized across che admin. This was discussed a few times in the past, see for example https://core.trac.wordpress.org/ticket/37233#comment:21

Ideally:

  • a search should be triggered after at least 2 chars have been entered (technically: 2 ASCII chars or one high UTF-8 char)
  • entering a space should not trigger a search

Right now, entering a space in the search field triggers a search and a re-rendering of the whole themes list, which seems not so ideal to me even from an usability and user experience perspective.

Attachments (2)

42343.diff (722 bytes) - added by celloexpressions 18 months ago.
Remove extraneous call to checkTerm() and trim search term in term change check.
42343.2.diff (1.3 KB) - added by celloexpressions 18 months ago.
Debounce calls to updateCount when it may be called multiple times quickly.

Download all attachments as: .zip

Change History (18)

#1 follow-up: @adamsilverstein
18 months ago

Hey @subrataemfluence, would you like to take a look at this?

#2 @adamsilverstein
18 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#3 @westonruter
18 months ago

I switched from debounce to throttle in [41797] because hitting back from Customizer to themes screen needed to throttle instead of debounce to ensure that the search results are updated immediately instead to prevent a flash of unsearched themes. My intention was that it should do a search with the first keystroke and then will do a subsequent search when the user stops typing. Per Underscore.js docs:

By default, throttle will execute the function as soon as you call it for the first time, and, if you call it again any number of times during the wait period, as soon as that period is over. If you'd like to disable the leading-edge call, pass {leading: false}, and if you'd like to disable the execution on the trailing-edge, pass {trailing: false}

But it isn't working as I expected.

See conversation at https://wordpress.slack.com/archives/C0381N237/p1509028368000714

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

#5 in reply to: ↑ 1 @subrataemfluence
18 months ago

@adamsilverstein Thank you for giving me the opportunity! I would surely take a look tomorrow.

Replying to adamsilverstein:

Hey @subrataemfluence, would you like to take a look at this?

#6 @westonruter
18 months ago

Sorry, my comment above should have been left on #42348 which is a similar issue.

#7 @adamsilverstein
18 months ago

I think it should still be _.debounce, with { trailing: false }?

#8 follow-up: @westonruter
18 months ago

@adamsilverstein wouldn't that mean the last keystrokes then wouldn't be sent in a request?

#9 in reply to: ↑ 8 @adamsilverstein
18 months ago

Replying to westonruter:

@adamsilverstein wouldn't that mean the last keystrokes then wouldn't be sent in a request?

Mmmm, I guess you are right. I'll have to test again to see the exact behavior.

#10 @westonruter
18 months ago

@celloexpressions Can you provide support here?

@celloexpressions
18 months ago

Remove extraneous call to checkTerm() and trim search term in term change check.

#11 @celloexpressions
18 months ago

  • Keywords has-patch commit added; needs-patch removed

It seems that the second call to checkTerm() was extraneous; I'm not sure why that was ever added. Additionally, we can trim spaces from the input when checking if the term changed to avoid unnecessary reloading. 42343.diff should fix this with relatively minimal changes.

As an aside, checkTerm() should probably be renamed before release. maybeInitializeRemoteQuery() or something along those lines would be more appropriate given that it is now only used for searching on remote section instances (it formerly also handled tags and other checks).

#12 @westonruter
18 months ago

  • Owner changed from adamsilverstein to westonruter
  • Status changed from assigned to accepted

#13 @westonruter
18 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Owner changed from westonruter to celloexpressions
  • Status changed from accepted to assigned

@celloexpressions Searching in the WordPress.org themes section is working properly with debouncing in your patch. However, the installed themes section is not getting debounced searches yet. I think that needs to be done as well, as the installed themes admin screen also debounces searching. The issue is made more apparent when using the a11y-speech-synthesis plugin as the browser is continually trying to communicate the number of themes found, but it is doing so too frequently with each keystroke.

#14 @afercia
18 months ago

Worth considering one more reason to make the local and remote searches behave the same: consistency in the user experience.

While I understand the local search is more a "filtering" of a local collection, producing results at each keystroke would introduce an interaction model that's different from the one needed on the remote search.

#31818 tries to address this issue holistically across the admin, taking into considerations all the (many) different search behaviors in the admin. No great progress there so far, unfortunately. It would be great to revive a bit that ticket and consider further improvements there.

@celloexpressions
18 months ago

Debounce calls to updateCount when it may be called multiple times quickly.

#15 @celloexpressions
18 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from celloexpressions to westonruter
  • Status changed from assigned to reviewing

I don't think that we should change the interaction intent at this point. Instead, the installed filter-search needs to debounce calls to announce the updated count. The bug is with the way the information is presented to screen readers, but it works well for other users without artificially slowing down the experience here.We can also improve the visual experience by not animating the count change until the search is completed. See 42343.2.diff.

There are fundamental technical differences in these types of sections, and also between this experience and the wp-admin version. The user experience is going to be significantly different between something that needs to reload from Ajax every time a parameter changes and something that filters information that's already displayed - trying to unify those probably wouldn't result in a better experience overall.

#16 @westonruter
18 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 42040:

Customize: Debounce requests for theme searches and the updating of the resulting filter count.

Props celloexpressions.
See #37661.
Fixes #42343.

Note: See TracTickets for help on using tickets.