Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37233 closed defect (bug) (fixed)

Shiny Updates: "Add New Plugin" shiny search issues

Reported by: afercia's profile afercia Owned by: swissspidy's profile swissspidy
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Plugins Keywords: has-screenshots shiny-updates has-patch needs-refresh
Focuses: ui, accessibility, javascript Cc:

Description

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
  1. 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:

https://cldup.com/rwX9MT7qiG.png

  1. 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)

https://cldup.com/EJ2DnGEtZU.png

Attachments (10)

search.patch (678 bytes) - added by rahulsprajapati 8 years ago.
Search keup event issue.
plugin-install.patch (388 bytes) - added by rahulsprajapati 8 years ago.
plugin install hide button when js is working
37233.diff (9.2 KB) - added by swissspidy 8 years ago.
37233.2.diff (9.2 KB) - added by swissspidy 8 years ago.
37233.3.diff (9.8 KB) - added by adamsilverstein 8 years ago.
37233.4.diff (9.8 KB) - added by ocean90 8 years ago.
37233.comment:25.1.patch (3.6 KB) - added by rahulsprajapati 8 years ago.
Handling the back button using "popstate" and trigger the "input" event for search text box.
37233.comment:29.patch (9.9 KB) - added by rahulsprajapati 8 years ago.
Created saparate function for search plugins.And used it for popstate and input event, as suggested in comment:29 by @swissspidy.
37233.comment:29.1.patch (9.7 KB) - added by rahulsprajapati 8 years ago.
Revised : Created saparate function for search plugins.And used it for popstate and input event, as suggested in comment:29 by @swissspidy
37233.comment:31.patch (9.7 KB) - added by rahulsprajapati 8 years ago.
Refreshed Patch : decoded url paramter values as suggested in comment:31 by @afercia

Download all attachments as: .zip

Change History (46)

#1 @afercia
8 years ago

  • Keywords shiny-updates added

#2 @afercia
8 years ago

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.

https://cldup.com/xfm-zTPzcF.jpg

#3 @ocean90
8 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major

@rahulsprajapati
8 years ago

Search keup event issue.

@rahulsprajapati
8 years ago

plugin install hide button when js is working

#4 @rahulsprajapati
8 years ago

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 )

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


8 years ago

#6 @swissspidy
8 years ago

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

This ticket was mentioned in Slack in #design by swissspidy. View the logs.


8 years ago

#8 follow-up: @swissspidy
8 years ago

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">
…

#9 in reply to: ↑ 8 @afercia
8 years ago

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

#10 @afercia
8 years ago

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.)

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


8 years ago

@swissspidy
8 years ago

#12 @swissspidy
8 years ago

  • Keywords has-patch added; needs-patch removed

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

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


8 years ago

#14 @mapk
8 years ago

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.

#15 @swissspidy
8 years ago

Thanks for your feedback!

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

#16 @afercia
8 years ago

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:
https://cldup.com/lkNFmLxPyB.png

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:

https://cldup.com/jHudZTcMvV.png

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


8 years ago

@swissspidy
8 years ago

#18 @swissspidy
8 years ago

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

#19 @swissspidy
8 years ago

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

#20 follow-up: @adamsilverstein
8 years ago

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?

#21 in reply to: ↑ 20 @azaozz
8 years ago

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.

#22 @ocean90
8 years ago

  • Keywords needs-refresh added

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

@ocean90
8 years ago

#23 @ocean90
8 years ago

  • Keywords needs-refresh removed

#24 @afercia
8 years ago

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

#25 @ocean90
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Priority changed from high to normal
  • Severity changed from major to normal

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?

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


8 years ago

#27 @swissspidy
8 years ago

#37429 was marked as a duplicate.

@rahulsprajapati
8 years ago

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

#28 @rahulsprajapati
8 years ago

  • Keywords has-patch added; needs-patch removed

#29 follow-up: @swissspidy
8 years ago

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
8 years ago

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

@rahulsprajapati
8 years ago

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

#30 in reply to: ↑ 29 @rahulsprajapati
8 years ago

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. :)

#31 @afercia
8 years ago

  • Keywords needs-refresh added

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:

https://cldup.com/Bfh7BRV9sy.png

Installed plugins search:

https://cldup.com/1gafpa0_Jh.png

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
8 years ago

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

#32 @adamsilverstein
8 years ago

@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.

Last edited 8 years ago by adamsilverstein (previous) (diff)

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


8 years ago

#34 @ocean90
8 years ago

@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.

#35 @ocean90
8 years ago

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

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.

Note: See TracTickets for help on using tickets.