#42348 closed defect (bug) (fixed)
Themes search: reintroduce the debounced search
Reported by: | afercia | Owned by: | 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)
Change History (14)
This ticket was mentioned in Slack in #core by afercia. View the logs.
7 years ago
#4
@
7 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
@
7 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:
- Go to Installed Themes.
- Search for “fifteen”.
- Click “Live Preview”.
- Close the Customizer.
- 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
@
7 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
@
7 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.
#8
@
7 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.
42348.diff just changes back the 2
_.throttle()
to_.debounce()
and also reintroduces a related inline comment.