Search Block: add button only with expandable input #50487
Conversation
|
Size Change: +5.48 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7665720. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5082066034
|
jffng
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
jffng
added
[Block] Search
|
Hi @jasmussen — I was reading your most recent feedback on the previous PR:
It seems like having the focus jump to the input field by simply focusing on the button, would feel unexpected. Requiring the explicit user action to expand + focus the input, could feel less jarring. But I'm not sure, so wanted to ask again on this refreshed PR. Let me know what you think 🙏 |
|
First off, this is great. This has been a much needed feature for a long time. Trunk: 
This branch: 
I think longer term we should revisit those dropdowns of options, the iconography isn't entirely solid, and there's arguably a better place in the inspector for some of these. I also think we can be better with the defaults, even keep the defaults smartly contextual so it's always collapsed into a labelles icon only version when inserted inside a container block. But those are followups, and I think there's such value in this one that we should land it quickly. That said, in testing I'm seeing this work only in the editor, not on the frontend: 
I'm not sure the focus should jump anywhere. The way I've been thinking about this is that the button-only search button is a camouflaged input field. It's one and the same, and focusing it simply expands it so you can type. Kind of like one of these:
None of those look particularly great, but hopefully the principle is clear, that it's just a styled input field that looks like a search button when not focused. What do you think? |
Are you using the If that doesn't fix it, do you see any console errors? |
|
npm run dev did it, apologies! Works decently well! |
There was a problem hiding this comment.
Oh good call! I removed it from block.json 73176c1.
Nice, that's worked on my side!
One more thing, it looks like we might need to run npm run fixtures:regenerate to regenerate the test fixtures for the search block, so they include the new block attributes.
| } | ||
| $input_markup = sprintf( |
There was a problem hiding this comment.
It would be nice to do this using the WP_HTML_Tag_Processor. There's an example of this in https://github.com/WordPress/gutenberg/pull/49212/files. Not necessary to get his merged, but a nice improvement.
There was a problem hiding this comment.
Nice, happy to try that in a follow up.
| @@ -0,0 +1,61 @@ | |||
| window.addEventListener( 'DOMContentLoaded', () => { | |||
There was a problem hiding this comment.
I wonder if this could use the new interactivity API cc @gziolo
There was a problem hiding this comment.
Yes, it definitely could. We will run the experiment for Interactivity API until everything is properly tested so it's fine to have two concurrent solutions for 2-3 Gutenberg plugin releases 👍🏻
|
Hi @jasmussen I pushed a change so that the input expands when the button is focused. How is it feeling to you? |
|
Adding to @jeryj 's comment, the current focus behavior feels unusual and a bit disorienting to me. Here's an alternative that came to mind:
This way, both the input and the button would be always accessible to assistive technology, without altering the tab order, and while keeping the button to always be a form submit button. |
|
Hi @ciampo thanks for weighing in. The alternative you suggest is how it worked it in b028cbb. Tabbing to a hidden element also felt unexpected, and there's some uncertainty around whether an element that's visually hidden should be available in the tab order. So the current state of the PR requires a click or keypress to expand the search field. I don't like that this one is a bit convoluted (the button purpose changes depending on the state of the search field, requires some additional client side js to handle aria attributes / tabindex), but I do think it provides a better UX for keyboard navigation. |
| searchField.setAttribute( 'tabindex', '-1' ); | ||
| searchButton.setAttribute( 'aria-expanded', 'false' ); | ||
| searchButton.setAttribute( 'aria-controls', id ); | ||
| searchButton.setAttribute( 'aria-label', ariaLabel ); |
There was a problem hiding this comment.
This causes problems if searchButton never had an aria-label. It will set the aria-label attribute to the string value "null", overriding any visible label the button might have. We should either check that ariaLabel has a value before doing this, or give ariaLabel a fallback value of "" above on line 13.
There was a problem hiding this comment.
searchButton should always have an aria-label, which is defined here.
| searchField.removeAttribute( 'tabindex' ); | ||
| searchButton.removeAttribute( 'aria-expanded' ); | ||
| searchButton.removeAttribute( 'aria-controls' ); | ||
| searchButton.removeAttribute( 'aria-label' ); |
There was a problem hiding this comment.
It's not clear why we're removing aria-label here. If the button has a label, it's probably because it's an icon button. In which case, removing the attribute will leave the button nameless. Am I missing something?
There was a problem hiding this comment.
We remove aria-label here since the behavior of the button changes to its default behavior (type="submit"). Previously the label would read Expand search button. We could set the label to something else, but I thought this would add some unnecessary complexity since the button is performing its expected behavior in this state.
There was a problem hiding this comment.
I updated this so that aria-label is set to "Submit Search", instead of removing it: fe7ff74
|
I have some comments, but first a newbie question – what is the thought process behind including this behaviour? Aside from it being a nice addition (#31128), have we validated that it's a useful and wanted bit of functionality, for both creators and consumers? I'm always a little wary of including things just because we think something will be popular. Happy to be shown previous conversations around this. Anyway, thoughts (in a bit of a brain dump – sorry!)... When you click on a The field remains hidden when it receives focus, potentially leaving the user in a confusing state. For most users this won't be an issue, because they can neither click on the label (see 1), nor tab to the input. But assistive technologies allow users to navigate in different ways, meaning it is possible to focus on the field, and then not be able to see it. The field should have a It looks like the expectation is that if the search form loses focus, the field should collapse. But it doesn't expand when it gains focus. This feels quite unbalanced to me, as an interactive process. Not that I necessarily think it should expand and collapse based purely on focus – that could get quite jarring. I suppose what I'm thinking is that I'm not sure about the multi-use single-button approach, but that's more my gut than any evidence-based appraisal! Finally, is it expected that if the field has a value (for example, on the results page, or if I just don't submit the search) it still collapses? This feels weird to me. |
|
@andrewhayward thank you for the thoughts and feedback!
I've seen this pattern come up a lot from theme designers, and it was prioritized here as part of a larger effort to improve the Search Block.
Nice, will address this 👍 EDIT: addressed in 7665720.
I'm not sure I follow these pieces of feedback together — if the field has a focus listener to make it visible (A), how would it not expand on focus (B)?
Yes, that was the intention. It seemed related to these guidelines, so I thought there should be a way to collapse / dismiss the field, even if the field has a value. |
|
I made a few more enhancements for keyboard and assistive technology users:
Is there anything else that should be addressed in this PR? There's varying opinions on what UX feels good, so it would be good to get something in and iterate, assuming this is still a feature we want to land. |
There was a problem hiding this comment.
Thanks for addressing all the feedback! This is testing well for me:
- Tested using a mouse ✅
- Clicking the label focuses the search field
- Tested using a keyboard ✅
- Escape closes the form when the input OR button has focus
- Tested with VoiceOver on a Mac using the keyboard to navigate ✅
aria-labelandtypeattributes reflect the button's purpose- Element descriptions work well
- Tested using a touch device ✅
- Also works well on several different resolutions
I've noticed that the aria- and related attributes aren't updated in the editor when you interact with this block. I think it would be good to update these to reflect the different states, but I'm not sure it should block this PR. What do you think?
|
Thanks @mikachan !
Yes, though I'm not exactly sure what those attributes / labels need to be since their purpose differs in the editor context, e.g. the button doesn't submit the form in the expanded state. So yes, a follow up could be good there! |
|
Thank you @mikachan! I opened up a follow up issue above to track these suggestions and will merge when the tests pass 💯 |
* Build docs. * Fix lint errors. * Fix php lint. * Remove duplicate call to view script. * List all dependencies in useEffect calls. * Add isSearchFieldHidden and buttonBehavior attributes to fixtures. * Allow input to expand on button focus and clean up CSS. * Add hidden class on initial block rendering. * Remove unneeded CSS. * Allow focus and blur to toggle search field, and simplify logic. * Handle keyboard navigation, event handling, and refactor. * Escape to close input, and do not allow focus to expand invisible element. * Add aria attributes to describe hidden / expanded states. * Fix lint warnings. * Fix php warnings. * Add and remove relevant ARIA attributes when input state changes. * Make strict comparison. * Clean up aria and take input out of taborder by default. * Fix label clicking behavior, remove unneeded event listeners. * Fix comparison bug. * Add and remove type and aria-label attributes to reflect form state. * Fix conditional on useEffect to show input on width change.
What?
Adds the Button (Icon) Only option to the search block. With this option selected, the behavior of the button changes so that when it's clicked, the search input field expands to the defined width.
Why?
Solves #31128
How?
If the revelant block attributes are present, a small client side script is enqueued to handle showing / hiding the search field. h/t @apeatling @aristath in #31719 for the starting point.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Editor
button-only-editor.mov
Front-end
button-only-view.mov