First stab at in-toolbar link editing #24021
Conversation
|
Size Change: +1.52 kB (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
add/navigation-link-in-toolbar-editing
branch
2 times, most recently
from
7bd1cab to
2728b35
Compare
adamziel
changed the title
adamziel
requested review from
ajitbohra,
ellatrix,
Soean,
talldan,
youknowriad and
chrisvanpatten
as code owners
adamziel
force-pushed
the
add/navigation-link-in-toolbar-editing
branch
from
4ea158a to
45ab3d8
Compare
|
I fixed the keyboard navigation in this PR so this aspect shouldn't be a blocker now. |
packages/block-editor/src/components/block-toolbar-link-control/compute-nice-url.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-toolbar-link-control/context.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm finding this UI much harder to use than the current one, and suggest we rethink this somewhat. Having the linkControl in the toolbar looks tidier, but the functionality should be identical to the current one. Having to press the "Done" button is really counterintuitive for any user, and it's currently unusable on a screen reader because there is no way of knowing that extra action is needed to submit the link. Because we're using the ARIA combobox pattern for the input/dropdown, we need to stick to the pattern rules to ensure full accessibility.
An idea: as this toolbar is specific to the Link block, would it make sense for "Open in new tab" and "nofollow" to be their own toolbar items?
Also, a couple of bugs:
- When editing an existing link, deleting the existing text and starting to type, the first typed character will be the first character of the deleted text, no matter which key is pressed 😅
- When adding a new Link block, picking a link or writing a custom link won't automatically fill in the link text. Saving the nav without writing the link text manually causes that item to not appear in the updated nav. This could easily trip up any user, but especially when using a screen reader there is no indication that the link text has to be manually filled in.
packages/block-editor/src/components/block-toolbar-link-control/compute-nice-url.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-toolbar-link-control/compute-nice-url.js
Outdated
Show resolved
Hide resolved
| <ToolbarGroup className="toolbar-link-control__input-group"> | ||
| <SettingsToolbarItem /> | ||
| </ToolbarGroup> | ||
| <ToolbarGroup> |
There was a problem hiding this comment.
Any reason for wrapping each element in its own group?
There was a problem hiding this comment.
It provides vertical dividers between different elements, it seems like it varies from design to design so maybe we don't need it.
There was a problem hiding this comment.
Semantically, toolbar groups should be used to separate different groups of buttons from each other. As far as I can tell, there's no reason to use multiple toolbar groups here.
There was a problem hiding this comment.
Actually, now that I think about it, maybe the "Done" button should stay separated from the others after all.
packages/block-editor/src/components/block-toolbar-link-control/link-input-toolbar-item.js
Outdated
Show resolved
Hide resolved
I share the feeling, maybe these changes should be applied right away in which case the "Done" button would just switch back to the original toolbar. In this scenario, selecting another block would not revert the changes.
It specific to the Link block only initially. In the longer run it could become the link editing UI for all blocks: button, paragraph, image. See #23375 |
Good spot @tellthemachines! This is fixed now.
I adjusted it so that the text is filled in for links with missing text. Also, it's worth noting that this interaction is going to be affected by #21050 and #23375:
|
My intention with the design is the following flow:
This is not the way it works in this PR. (Maybe it could? ;)) Here's a GIF showing how I expect it to work:
Do you think this flow address the issue you mention, @tellthemachines?
I'm reading W3.org about this, but I'm not entirely sure if I'm understanding it correctly. I think the flow I outlined above follows the pattern. |
It's almost the same, but not quite 😅: in the demos on the page you link to, items in the dropdown can be selected by arrow key navigation followed by pressing enter (you should also be able to click an option with the mouse to select it, but for some reason that's not working currently). So far so good, but the difference is that in the demos, when enter is pressed, the item is immediately selected, and tabbing to the next demo the item stays selected. With this toolbar, even if we transfer focus to the "Done" button on pressing Enter, there is still an extra step (pressing "Done") that needs to be completed in order for the selection to be effective. That whole interaction is unexpected for a combobox pattern: first, transferring focus to another button when the dropdown closes (it would usually remain on the input), and then, requiring the extra button click to save the action.
Got it, then it makes sense for everything to be in a single component. Still, we should be able to toggle "new tab" and "nofollow" without needing to confirm the changes with the "Done" button, like we can in the "more tools" dropdown. |
There was a problem hiding this comment.
These are good improvements!
Just a couple more things:
- Typing in the input and immediately pressing enter now selects the item but doesn't close the dropdown, which feels a bit weird.
- It would be nice to close the dropdown with "Escape" (focus should go to the input).
|
Just noting that Google docs handles this similarly. When using the mouse, clicking to add a linked item from their dropdown keeps the LinkControl UI open and requires a manual click on the "Apply" button. However, I noticed that when using the keyboard, once I select an item from the dropdown, the LinkControl UI closed entirely. Although I've seen this interaction also leave the UI open and require one to click "Apply" as well. I'm not sure if that depends on which type of item in the dropdown was selected.
|
|
@tellthemachines I updated the PR to reflect these two suggestions. Also note that the only thing the "Done" button does now, is it brings back the original toolba. Any changes (link, rel, target) are applied immediately without the need to confirm them. |
|
@tellthemachines @shaunandrews @mapk Would you say this is good to go as a first iteration, or should we discuss the interaction more? |
|
A note to myself - @draganescu mentioned the enter does not behave as intended in Firefox, I should check it before merging. |
|
@afercia I'd recommend testing this branch instead as it includes all the changes. You may have to reset the branch as I've rebased it onto the changes from #25177. Something like this should work: The feature is currently only active on the experimental navigation page. |
| toggleProps={ { | ||
| ...toolbarItemProps, | ||
| name: 'link-options', | ||
| title: __( 'Link options' ), |
There was a problem hiding this comment.
This renders a title attribute on the button. Title attributes are an accessibility and usability anti-pattern and should not be used. In the last years, a lot of work has been done to remove as much title attributes as possible from the admin. For no reason new title attributes should be introduced.
Also, the button doesn't have a tooltip so it doesn't expose its name on focus. Looking at how the DropdownMenu toggle button works, I think a label prop needs to be used directly on the DropdownMenu to correctly label the button and make the tooltip work.
Lastly, not sure what the name prop does other than rendering a name attribute name="link-options" on the button. Is this prop necessary for some reason?
There was a problem hiding this comment.
I think this actually ends up setting the title prop on a Button, which in turn sets the aria-label of the underlying HTML <button>?
There was a problem hiding this comment.
That said, I haven't actually tested out this PR, so I could be wrong.
There was a problem hiding this comment.
I can confirm it is adding a title attribute to the button element. If we want to render a tooltip, we should instead use showTooltip together with label="Link options", which sets an aria-label and shows the tooltip as used in editor icon buttons.
I'm also not sure what that name prop is about; it's not a documented option in either the DropdownMenu or the Button components.
| </ToolbarGroup> | ||
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| name="done" |
There was a problem hiding this comment.
Not sure what this name prop does other than rendering a name attribute on the button. Is this necessary from some reason?
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| name="done" | ||
| title={ __( 'Done' ) } |
There was a problem hiding this comment.
This renders an aria-label attribute aria-label="Done" but the button already has a visible text "Done" so it's redundant and should be removed.
| title={ __( 'Done' ) } | ||
| onClick={ close } | ||
| > | ||
| Done |
There was a problem hiding this comment.
This text is not translatable.
| <LinkControlSearchInput | ||
| ref={ searchInputRef } | ||
| currentLink={ currentLink } | ||
| placeholder="Start typing" |
There was a problem hiding this comment.
This text is not translatable.
Thanks @talldan ! Still getting errors also on this branch. Seems something related to the typescript version check and happens only when running |
|
Some considerations after a first round of testing:
These are big plus as the ARIA toolbar pattern semantics and expected interaction are preserved when the link interface is not rendered. 🎉 Things that still need to be improved:
I think this is because the link interface still uses
|
|
I am testing out the PR and currently see this:
Clicking an existing link or a new link shows the same icon. I do not know which of the nav links are linked or not. I select the About page in the navigation. For instance in Google doc as @mapk shared here: #24021 (comment) We are able to see the word This as part of the sentence and we also see the drop down showing Text and Link. How would we go about showing the Text, the Link and where it belongs at the same time? Some brain storming.... Here is one idea where one can change the Text and Link at the same time. We are still not able to see where the link belongs as the drop down covers the menu location of the about page.
Creating a border around the selected menu item when Link control is active shows the connection between menu item, Text and Link. I will think about how we could show a drop down without it covering the location in the menu of the About page. EDIT: 21 Sept I just want to say again that it is important for the user that the Navigation screen and the Gutenberg area both contain a link control where one can easily see which word (menu item) contains a link. Google docs does this really well. |
I don't think we should bubble escape key while the input is focused @talldan. |
|
@afercia Thanks for testing and providing feedback. One of the things I wasn't sure about is whether we should still treat the items inside the 'popover' as toolbar items (still using arrow key navigation), but it sounds like you're saying it should behave just as the previous link control did, with tab used to move focus. That does simplify things a bit for this use case, which is good. I'll work on the feedback and let you know when it's ready to re-test. |
|
Thanks @talldan! Yep, I meant to keep it simple and just make it a "popup" with modal behavior. |
talldan
force-pushed
the
try/block-toolbar-inline-edit-slot
branch
from
3da4ed1 to
ed254f8
Compare
Only use the experimental toolbar link editing on the navigation screen Re-add missing block-toolbar-link-control Prioritize inputProps over toolbarItemProps Pass ref directly to InputControl instead of going through LinkControlSearchInput Show spinner when creating new page Create snackbar notice when error occured in useCreatePage Revert LinkControlSearchInput change Restore is-vertically-retracted to search-input.js Restore renderControl to searchInput include rel in link Decide suggestions popover visibility based on the settings dropdown state Improve keyboard navigation Formatted Restore changes from master Make the "create new page" button work Rename computeNiceURL to computeDisplayURL Move <LinkInputToolbarItem /> and <SettingsToolbarItem /> to the same toolbar group Fix overriding url in updateCurrentLink Save link block attributes on each change, not just once Done is clicked Remove setCurrentLink from the context Hide the suggestions dropdown on blur Hide suggestions on escape
talldan
force-pushed
the
add/navigation-link-in-toolbar-editing
branch
from
5abeeab to
95ad8b2
Compare
talldan
force-pushed
the
add/navigation-link-in-toolbar-editing
branch
from
95ad8b2 to
78f0862
Compare
Hey @talldan 👋 Please let us know when this PR is ready for another test. The a11y team thinks this idea has potential and if it works as expected we'd like to see a similar pattern followed on other toolbars. |
|
@enriquesanchez @afercia Given the navigation editor won't be in 5.6, I think it makes sense to put this on pause and focus on the image block's toolbar problems instead. Sorry I haven't been able to make much progress, I'll keep this open and return to it when it's a good time. |
|
Regardless of this PR, bringing over |
|
I don't think this should still be in the Navigation editor project, so I'll take it out. |
draganescu
added
the
[Status] Stale
|
I'm going to close this PR as it was marked as stale over a year ago and there wan no action since then. Feel free to reopen once you decide to revisit this feature. |
Description
This PR adds
ToolbarLinkControlas a default way of editing links on the experimental navigation screen (see #23375).The control wires together expanded BlockControls and reusable parts of LinkControl to provide in-toolbar link editing UI as on the screenshot below:
Caveats:
Keyboard navigation is far from perfect and needs to be iterated on, but maybe it's not a blocker for merging the first iteration?At the moment link-related changes are only accepted if you explicitly confirm them by pressing the "Done" button inside the toolbar. If you select a different block, the changes are disregarded. To me, it feels natural with regard to the text field, but unnatural with regard to the dropdown menu - discussion appreciated.How has this been tested?
relandopensInNewPageattributes does not actually work, but it's unrelated to this PR.Types of changes
New feature (non-breaking change which adds functionality).
Checklist: