Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#42348 closed defect (bug) (fixed)

Themes search: reintroduce the debounced search

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, accessibility, javascript Cc:

Description

A recent change in [41797] changed how the theme searches work on the admin screens themes.php and theme-install.php. The searches were debounced while users type a search term, specifically to not trigger a search and wait they enter the complete search term.

This is particularly important for the .org themes, since it avoids to trigger multiple AJAX requests while typing. It is also important for usability and accessibility on both screens.

On theme-install.php seeing the screen that "flashes" displaying partial search results while typing is not the best user experience.

Moreover, on both screens, a wp.a11y.speak() message fires when there are search results. Basically, with this change, it's very likely a message gets announced every time a character is entered in the search field, thus "bombarding" users with continuous, audible messages like "Number of Themes found: nn" and so on and on.

For more details on why the search were debounced, see:

https://core.trac.wordpress.org/changeset/31994
https://core.trac.wordpress.org/ticket/26600

and

https://core.trac.wordpress.org/changeset/27830
https://core.trac.wordpress.org/ticket/27055

The recent [41797] change uses throttling instead of debouncing to address a specific Customizer need, to avoid a "flash of unsearched content" when going back from the Customizer to the themes screen. However, it breaks how the search is supposed to work on the admin screens. Development happened on GitHub, see https://github.com/xwp/wordpress-develop/pull/278 and the specific commit: https://github.com/xwp/wordpress-develop/pull/278/commits/4e3abbb7c37898b9c2a0124e8c10725f3c1d6f82

I've tried to reproduce the original Customizer issue changing back to debounce() and honestly I don't see a great difference in the "flash" of content, unless I'm missing something.

Anyways, in order to fix a minor "flash" of content, this change is breaking a major UI feature, not to mention the multiple AJAX requests, and I'd propose to just revert back to debounce().

Attachments (3)

42348.diff (877 bytes) - added by afercia 6 years ago.
42348.2.diff (2.1 KB) - added by westonruter 6 years ago.
42348.3.diff (2.2 KB) - added by westonruter 6 years ago.

Download all attachments as: .zip

Change History (14)

@afercia
6 years ago

#1 @afercia
6 years ago

  • Keywords has-patch added; needs-patch removed

42348.diff just changes back the 2 _.throttle() to _.debounce() and also reintroduces a related inline comment.

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


6 years ago

#3 @jbpaul17
6 years ago

  • Keywords needs-testing added

#4 @westonruter
6 years ago

  • Keywords needs-patch added; has-patch removed

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

As long as we can get the initial page to load with single non-debounced search, that would fix the issue as well.

#5 @westonruter
6 years ago

  • Keywords needs-testing removed

With 42348.diff the “flash of unsearched content” returns. See demo: https://youtu.be/YA49Ve8Ium0 (at 10 seconds)

To reproduce:

  1. Go to Installed Themes.
  2. Search for “fifteen”.
  3. Click “Live Preview”.
  4. Close the Customizer.
  5. Notice all themes are shown for 500ms and then the list is updated to only show Twenty Seventeen again.

So that needs to be avoided, while also ensuring that the searches are debounced.

#6 @afercia
6 years ago

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.
But it isn't working as I expected.

It works exactly as expected. Unfortunately, its usage in this implementation is wrong. In the theme installer for example, the throttled function doSearch() is called at each keystroke and executes immediately. doSearch() in turns calls each time this.collection.query( request ); so the search is actually triggered at each keystroke. Very simple.

debounce() instead works differently and it will continuously postpone the execution while the function is called. When the function is not called any longer (the user has finished typing), the debounced function will run after the specified delay.

With 42348.diff​ the “flash of unsearched content” returns.

I see it happens just when there's a search term entered in the search field? I'd say that's a minor edge case compared to the breakage this change is causing. Not to mention hammering the servers with multiple AJAX calls.

I'd just like to point out this change is actually breaking a working functionality that was agreed on and broadly discussed. So it's actually a regression in trunk.

#7 @westonruter
6 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

It's a regression either way. If we just switch back to debouncing then there is the flash of unsearched themes. So we need to have a way to force an initial search to bypass the denouncing. I'll work on a patch if someone doesn't get to it first.

Last edited 6 years ago by westonruter (previous) (diff)

@westonruter
6 years ago

@westonruter
6 years ago

#8 @westonruter
6 years ago

  • Keywords has-patch added; needs-patch removed

I found an approach that ensures we get debouncing while also ensuring the initial search from the query param gets immediately called: just defer wrapping doSearch with _.debounce() until after Backbone.history.start() has been called. See 42348.3.diff.

#9 @westonruter
6 years ago

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

In 42029:

Themes: Switch back from throttling to debouncing in theme searches on admin screen.

Start debouncing after initial search performed when search query param is present to prevent initial "flash of unsearched themes".

Props afercia, westonruter.
Amends [41797].
See #40254.
Fixes #42348.

#10 @westonruter
6 years ago

In 42222:

Themes: Prevent JS error on Themes admin screen when only one theme is installed.

Amends [42029].
Props chetan200891, afercia.
See #42348.
Fixes #42673.

#11 @dd32
6 years ago

In 42223:

Themes: Prevent JS error on Themes admin screen when only one theme is installed.

Amends [42029].
Props chetan200891, afercia.
See #42348, westonruter.
Merges [42222] to the 4.9 branch.
Fixes #42673.

Note: See TracTickets for help on using tickets.