Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#42343 closed defect (bug) (fixed)

Customize: Themes search debounce is not debounced properly

Reported by: afercia's profile afercia Owned by: westonruter's profile 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 6 years ago.
Remove extraneous call to checkTerm() and trim search term in term change check.
42343.2.diff (1.3 KB) - added by celloexpressions 6 years 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
6 years ago

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

#2 @adamsilverstein
6 years ago

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

#3 @westonruter
6 years 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.


6 years ago

#5 in reply to: ↑ 1 @subrataemfluence
6 years 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
6 years ago

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

#7 @adamsilverstein
6 years ago

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

#8 follow-up: @westonruter
6 years ago

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

#9 in reply to: ↑ 8 @adamsilverstein
6 years 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
6 years ago

@celloexpressions Can you provide support here?

@celloexpressions
6 years ago

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

#11 @celloexpressions
6 years 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
6 years ago

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

#13 @westonruter
6 years 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
6 years 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
6 years ago

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

#15 @celloexpressions
6 years 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
6 years 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.