Shiny Updates: "Add New Plugin" shiny search issues

[afercia]

Basically all the issues reported on #37230 with a note and a couple of additional issue:

1. the search field in this screen does have a submit button, even if it's hidden so the same considerations about the search button in #37230 apply also to this screen

2. after the search results get displayed, just move the cursor inside the search field using the arrow keys and a new search will be triggered (even if the search term has not changed, so no actual query) and so you get an empty screen:

3. after the search results get displayed, refresh the page and the "Search Results" tab will appear, it should probably always be displayed (or never when JS is on and the live search is enabled)

[afercia]

For completeness, just wanted to mention that on 4.5 it is also possible to search plugins by author and by tag. Though this functionality has always been a bit hidden, it is now completely missing. The small select with the search type options appears just after forcing a page refresh.

[rahulsprajapati]

Search keup event issue.

[rahulsprajapati]

plugin install hide button when js is working

[rahulsprajapati]

search.patch

• Search keyup event issue.

plugin-install.patch

• Plugin install page, hide "Search Installed Plugins" button ( Hide button via js so, if js is not working button will show )

[swissspidy]

For completeness, just wanted to mention that on 4.5 it is also possible to search plugins by author and by tag. Though this functionality has always been a bit hidden, it is now completely missing. The small select with the search type options appears just after forcing a page refresh.

So on 4.5 that drop down is only shown after the first search, like on the plugin directory. On the new plugin directory, this drop down is currently completely missing.

Only displaying it after the first search seems a bit weird (and confusing to users) when performing an Ajax search. I mean, suddenly a drop down pops up. How would you announce this to screen readers? Would you put focus on it?

I think we should either always show this drop down or completely remove it. It's coupled to the install_search_form() function though. From the source code:

// assume no $type_selector means it's a simplified search form

<?php if ( $type_selector ) : ?>
<select name="type" id="typeselector">
…

[afercia]

Replying to swissspidy:

I think we should either always show this drop down or completely remove it.

I'd agree. I've always thought it's a bit weird to have that drop down appear only after the first search and I wonder how many users are aware they can search also by username or tag. We're in 2016 and ideally a search should search for anything, without asking users to specify keyword/author/key search types. Silently removing the functionality would be something to carefully consider though. Also, see this meta ticket comment ​https://meta.trac.wordpress.org/ticket/1692#comment:17

[afercia]

Just noticed one more issue related to the keyup event, and similar to the arrow keys one. To reproduce:

• do a search, for example "RTL tester"
• some search results will be displayed
• be sure the cursor is still inside the search field
• open a new tab in your browser using the keyboard (Cmd+t on a Mac)
• switch back to the plugins search tab using your keyboard, for example Ctrl+Tab to cycle through tabs or Cmd+number
• as soon as you release the pressed modifier (Ctrl or Cmd) a keyup event occurs and a new search will be triggered
• final result: a screen with no search results

Any non-character key press will trigger a new search (for example pressing and releasing Shift, Alt, etc.)

[swissspidy]

Thanks for the additional details @afercia! That's caused by the same thing es the one with the arrow keys (point 2 in the ticket's description).

In 37233.diff​:

• Add aria-describedby, wp.a11y.speak and esc key improvements like in #37230.
• Hides the search button if JS is enabled, like in #37230 as well
• Do not perform search twice for the same search string (props @rahulsprajapati)
• Adds the "Search Results" tab after performing a search for the first time and marks it as current

[mapk]

The design looks good here aside from my comments on #37230.

I also noticed the issues that @afercia mentioned about the search results disappearing for odd reasons.

[swissspidy]

Thanks for your feedback!

Unfortunately I can't seem to reproduce this disappearing results bug with 37233.diff​ applied.

[afercia]

Quickly tested 37233.diff​ and the disappearing results thing seems fixed :) Noticed some outstanding and new issues though:

1
on $pluginInstallSearch the search non-standard event should be changed in input as already done for $pluginSearch.

2
the results count message for wp.a11y.speak() is not correct when there are more than 30 results. I guess this depends on the fact the current results count is based on the plugin install list table "per page" limit while it should be based on the total items count:

3
seems the AJAX search doesn't take into account the search "type" set with the select options (works correctly with JS off). In the example in the screenshot below, the search for "author johnbillion" should return 21 results:

[swissspidy]

In 37233.2.diff​:

• Change search event to input
• Return the number of total items found, not the number of items on the page (i.e. 170 vs 30)
• Do not hardcode the "type" to "term", use the value of the dropdown instead

[swissspidy]

Note: See also #37373 which fixes the "More Details" link on multisite.

[adamsilverstein]

Nice work here @afercia identifying these bugs and clearly describing how to reproduce them! I tested the latest patch and verified it resolved the issues raised, thanks @swissspidy.

37233.3.diff​ builds on .2 and adds the following:

• switch the event from search to the standard input as suggested by @afercia and as used for the installed plugin search (and elsewhere in core JS). I am not certain we need keyup at all, I think it is required for certain browsers that don't trigger 'input' - @azaozz would probably know for sure :)
• two events triggered the search event - hitting enter and changing the type dropdown. I changed both of these to trigger input instead
• when changing the dropdown, we should trigger the search again even if the search field string hasn't changed, I updated the logic to ensure that works

I noticed that we are triggering a search when the field is blank, for example changing the dropdown or hitting the escape button. This shows all plugins and seems unexpected. Do you think we should avoid searching when the search field is blank?

[azaozz]

Replying to adamsilverstein:

• switch the event from search to the standard input as suggested by @afercia and as used for the installed plugin search (and elsewhere in core JS). I am not certain we need keyup at all, I think it is required for certain browsers that don't trigger 'input' - @azaozz would probably know for sure :)

Good idea. For text fields the input event is not supported only in IE8: ​http://caniuse.com/#feat=input-event. It is fired on every keystroke that changes the element's value. It's advantage over the key* events is that it is not fired on "non-printing" keys like arrow keys, Esc, page up/down, etc. and is fired on paste and on other input methods like entries from speech recognition. That makes it perfect for triggering anything that requires some user input first.

Looking at 37233.3.diff: to support IE8 (ugh!) and to catch the Esc key the patch will still need to use keyup but separate from input, then do something like if ( ! 'oninput' in document ) .... The searching function will have to be separate so it can be called from both events.

• when changing the dropdown, we should trigger the search again even if the search field string hasn't changed, I updated the logic to ensure that works

I noticed that we are triggering a search when the field is blank, for example changing the dropdown or hitting the escape button. This shows all plugins and seems unexpected. Do you think we should avoid searching when the search field is blank?

Using only the input event will stop triggering the search on "non-printing" keys. However it is best to trigger the AJAX only after the user enters at least 2 characters.

Thinking that ideally it should trigger after 2 ASCII chars or one high UTF-8 char. We can standardize this for all similar cases in core, there are at least 6-7 other places.

[ocean90]

Needs a refresh after [38091], there is also a typo in update.js "plign".

[afercia]

Quickly tested 37233.4.diff​ noticed a couple of things:

• leave the search field empty and change the search type from "keyword" to "author" or "tag" in the select: a new search is triggered even if the search field is empty
• the search type select needs a label (can also be hidden), something like <label for="typeselector" class="screen-reader-text">Search plugins by:</label> could work, or maybe with a better wording

[ocean90]

Committed 37233.4.diff​ with fixes for the issues mentioned in comment:24 in [38119].

Using the history API is a nice idea, but not handling the back button is a bad idea. We'd need $( window ).on( 'popstate', {} ); which parses the URL and triggers a new search... Or should we use replaceState instead?

[swissspidy]

#37429 was marked as a duplicate.

[rahulsprajapati]

Handling the back button using "popstate" and trigger the "input" event for search text box.

[swissspidy]

Thanks for your patch @rahulsprajapati!

Unfortunately I don't have much time to work on this so here are some notes:

• Instead of doing stuff like bodyElem.hasClass( "plugin-install-php" ), we should check the global pagenow variable
• Instead of triggering input events, we should put the search handling in a separate function and call that function. That way it's also better testable.
• What about the dropdown? That should be handled in the popstate callback too.
• wp.urlParam looks interesting. Definitely needs unit tests though.

[rahulsprajapati]

Created saparate function for search plugins.And used it for popstate and input event, as suggested in comment:29 by @swissspidy.

[rahulsprajapati]

Revised : Created saparate function for search plugins.And used it for popstate and input event, as suggested in comment:29 by @swissspidy

[rahulsprajapati]

Replying to swissspidy:

Thanks for your patch @rahulsprajapati!

Unfortunately I don't have much time to work on this so here are some notes:

• Instead of doing stuff like bodyElem.hasClass( "plugin-install-php" ), we should check the global pagenow variable
• Instead of triggering input events, we should put the search handling in a separate function and call that function. That way it's also better testable.
• What about the dropdown? That should be handled in the popstate callback too.
• wp.urlParam looks interesting. Definitely needs unit tests though.

Added the refreshed patch. Let me know if anything else needs to update. :)

[afercia]

Quickly tested the last patch, and the history works nicely. However, needs to take into account that the search term can be made of more than just one word.

Add new plugin search:

Installed plugins search:

When navigating back, the search field gets updated with the search string and urlSearchString needs to be decoded and also to replace any plus sign with a space. About the "sub title" thing added to the page heading, I'd be in favour of removing it entirely as noted also in an inline comment :)
// Can we just ditch this whole subtitle business?.
See also #26601.

Lastly, and unrelated to the last patch: the change event on the search type select can be confusing for users and not ideal for accessibility since it fires in different ways across different browsers/platforms. For the same reason, a similar change was reverted in #31634 but to be honest I wouldn't know how to solve this issue in this context, since there's no submit button.

Minor: coding standards and variables declaration/assignment could be improved a bit.

[rahulsprajapati]

Refreshed Patch : decoded url paramter values as suggested in comment:31 by @afercia

[adamsilverstein]

@rahulsprajapati thanks for the patch - will work on testing it :) disregard my earlier question I now see your description of the changes further up in the comment thread.

[ocean90]

@rahulsprajapati Thanks for your patch and work so far.

That's a pretty big change late in this cycle. Because of that I'd like to use replaceState for now. We do the same for themes already.

[ocean90]

In 38154:

Plugins: Use history.pushState() to customize the URL during searches.

history.pushState() requires an event handler for popstate which doesn't exist (yet).

Props rahulsprajapati for initial patch.
Fixes #37233.

[rahulsprajapati]

Cool. :)