[Experiment - Inserter]: Try search input inside each tab #45203
Conversation
ntsekouras
added
[Feature] Inserter
ntsekouras
requested review from
javierarce,
youknowriad,
mtias,
jameskoster and
jasmussen
|
Size Change: +160 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
@ntsekouras This still looks kind of early, but please let me know when this is ready for A11Y review. |
|
Nice PR. This feels like a good approach that will enable the expansion of the Inserter to include media (#44918). It also streamlines search results and makes them much less overwhelming. I found the search term persisting between tabs a bit unexpected. It makes the search feel like an awkward middle-ground between global/local search. Keen to hear what others think of that.
The issue here seems to be: If you search the quick inserter, then click 'Browse all', it opens the inserter with the same search term. So we're faced with a question of which tab should be active in this re-arranged UI. Since the quick inserter searches everything, there would no longer be an equivalent state for the main Inserter, so perhaps the Browse all button should disappear when you search the quick inserter? 🤔 |
I don't think we should hide it because there is the case of searching something and there are just more results available, but no the one we're looking for. This could be most noticeable in patterns. We have a flag of what to prioritize(patterns vs blocks) and we could probably use that.. 🤔 Right now the search results for |
But then you are not browsing 'all'. It seems a bit counter-intuitive to go from viewing results from all categories to viewing only patterns, or blocks. Especially after clicking browse 'all'.
Perhaps Reusable could be a category within the Blocks tab? |
So one option is to hide Are we committed with this design to add the search inside the tabs? If yes, I should look into all the details and the changes especially in tests are expected to be quite a lot. --cc @WordPress/gutenberg-design |
|
I was wondering what this PR was about. Meaning I thought by default it already was this way, but noticed it was not. Adding Search inside each tab seems like a given to me, as it creates a natural consistency that one is able to make a search inside each tab. Making a search for "heading" inside one tab and not finding what I am looking in the specific tab screen it would be natural to have the search term carry over to the next tab without needing to write the search term again. That would be extra useful when there are 4-5 tabs I click through to find what I am looking for. I can easily change the search term or click the x to remove it. |
ntsekouras
force-pushed
the
try/inserter-search-inside-tabs
branch
from
3e195ae
to
d5182ed
Compare
|
@alexstine in this commit I'm basically reverting your previous PR, but I might missing something important in terms of context. When you find some time I'd really appreciate some feedback and/or alternatives on how to make this work best. Thanks! |
|
@ntsekouras You cannot do that because of this apparently. I guess this can be used outside of a sidebar so we need the ability to fire focus from our sidebar, not from the inserter components themselves. CC @youknowriad From my current understanding, wordpress/block-editor could be used in any project. |
It's a different implementation though now. We don't have a single search input outside.. So the idea would be to focus once when the fist tab is selected and if we switch tabs(ex Blocks->Patterns) the search input should not focus? |
|
@ntsekouras I think that is okay but what about the Can you also try to debug why this PR makes it impossible to tab/shift+tab in to the tabs? Thanks. |
I'll check it better tomorrow, but for me shift navigation works. After rendering and setting the first selected tab, the search input on that tab is focused. After that, if I press shift+tab goes to the tabs and then I can go to other tabs with left/right arrow keys. Right now if a new tab is selected, the focus moves to that tab's search input. Isn't that the case for you? I appreciate the feedback Alex! |
|
@ntsekouras Reproduction steps:
I think the issue is the roving tabindex is applied to the tab content, not the sidebar itself. Thanks. |
ntsekouras
force-pushed
the
try/inserter-search-inside-tabs
branch
from
9e6c65c
to
464152b
Compare
I couldn't reproduce in chrome, but I think that's an existing issue in trunk too.. Can you give it a go in trunk? |
| @@ -136,6 +136,7 @@ export function TabPanel( { | |||
| role="tabpanel" | |||
| id={ `${ selectedId }-view` } | |||
| className="components-tab-panel__tab-content" | |||
| tabIndex={ -1 } | |||
There was a problem hiding this comment.
Include tabindex="-1" on enable a tabpanel to receive focus without putting the tabpanel in the page Tab focus sequence for keyboard users.
According to tabpanel role docs, this seems to be okay I think. -cc @ciampo
There was a problem hiding this comment.
Thank you for the ping! I must admit that I'm not super expert about TabPanel (the component pre-dates the start of my work on components, and I haven't really used it often so far), but the MDN docs seem quite clear.
What's the reason why you're adding that attribute specifically for this PR ? Do we want all TabPanel instances to have tabIndex = -1 ?
Best thing to do here would be to split changes to TabPanel to a separate PR, so that it can be discussed more in details
There was a problem hiding this comment.
The reason would be:
I think the issue is the roving tabindex is applied to the tab content, not the sidebar itself.
I'm not really sure if that's what we want either here or for the component and right now I'm reading and exploring. If we need a change like that eventually, I'll open a separate PR.
In general is really hard because different browsers have different behaviors for me right now and is baffling 😄
There was a problem hiding this comment.
Opened an issue to track this separately #45390
There was a problem hiding this comment.
@ntsekouras That is not the reason. The reason is, your PR changes the DOM order. In trunk, I can press tab once focus is placed in the search input and move in to the TabPanel. Because the focus is placed in the search input and the search input is wrapped with the constrained tabbing as part of the inserter, there is no way to get back to the tabs as those seem to be outside of the constrained tabbing area.
This is one of those things that needs to be fixed before your PR gets merged or we will end up making the inserter inaccessible. :(
If you do a fresh build of trunk, try the following.
- Open the inserter.
- Press tab.
- Focus is placed on the Blocks tab item as normal.
In your PR.
- Open the inserter.
- Notice how the search input receives focus but it is below the tabs in the DOM.
- Press shift+tab to move to the Blocks tab.
- You land at the bottom category of the inserter.
There was a problem hiding this comment.
Ah, never mind. I think it will not matter for this PR. You can eventually get to the tabs, but it is a little hard.
Can someone open a follow-up issue to apply the constrained tabbing to the inserter sidebars and not the specific components themselves? This should help out a lot with this. That way the sidebar has the same roving tabindex no matter what tab is selected.
Aside from that, need to wait until the above issue for manual tab activation is implemented. Once that is done, I give A11Y sign off on this.
There was a problem hiding this comment.
It's still not 100% clear to me though what else I should do regarding the constraint tabbing issue you mention. Inserter sidebars use the useConstrainedTabbing hook and for me it seems we have an existing issue in trunk with composite navigation in some browsers like firefox.
Example:
- Open inserter and leave it to blocks tab
- Focus goes to search input
- Try to enter the block types list with
tab - Observe that you can't.
Do you have some specific solution in mind code wise?
There was a problem hiding this comment.
@ntsekouras Not at this point. I just do not understand how it works right now. I am busy working on other issues.
ntsekouras
requested review from
ajitbohra,
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel,
kevin940726 and
ellatrix
as code owners
…st one pattern is shown
ntsekouras
force-pushed
the
try/inserter-search-inside-tabs
branch
from
c688472
to
7ee3513
Compare
| @@ -117,7 +117,7 @@ describe( 'Site Editor Performance', () => { | |||
| await canvas().click( | |||
| '[data-type="core/post-content"] [data-type="core/paragraph"]' | |||
| ); | |||
| await insertBlock( 'Paragraph' ); | |||
| await insertBlock( 'Paragraph', { checkSelectedTab: true } ); | |||
There was a problem hiding this comment.
This needs to be revisited for this PR as it will probably affect the perf results. With this PR the results are contained to the selected tab, so since we have patterns as the first tab in site editor, we have to first click to the blocks tab before inserting the block.. --cc @youknowriad @dmsnell
There was a problem hiding this comment.
are you saying we need to add a step in the performance e2e tests so that the block inserter appears?
There was a problem hiding this comment.
sounds good! I haven't touched anything inside the performance tests, but you are welcome to also tag me as a reviewer when you update them.
|
This seems to be blocked by an a11y issue mentioned by @alexstine . I think this is the same thing that exists on trunk and I created a new issue to figure this out. |
|
@ntsekouras Thanks for putting this one on hold while the bigger A11Y issue is figured out. |
What?
This PR explores how we should handle the search in the inserter. It's still just an early start to discuss design and UX, as it's not clear yet from the existing suggestions.
Some designs about this can be found: #44496 (comment), #44918 (comment).
It's also related to the
Mediatab in the inserter(PR).Notes
browse allbutton in quick inserter.Testing Instructions
Screenshots or screencast
Screen.Recording.2022-10-21.at.1.15.34.PM.mov