-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try the link control in the link format #19462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that you can't resubmit a link you want to edit without making any modification, it seems like the Input is missing a submit button or something. Otherwise choosing a suggestion becomes mandatory so we need to type again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. I'm surprised as to how nice this feels.
One thing I noticed was that once I created a link of portion of text in a paragraph block I couldn't easily dismiss the popover UI.
I do wonder whether an explicit "close" icon could be required even if it adds a little extra clutter to the UI.
@@ -181,6 +183,7 @@ function LinkControl( { | |||
onClose={ closeLinkUI } | |||
position="bottom center" | |||
focusOnMount="firstElement" | |||
{ ...popoverProps } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems logical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think, there's something wrong in the control for now. I feel instead of directly being a "Popover", we need two components:
- LinkControl, which is an inline UI to edit links. (like all controls, TextControl, SelectControl, CustomSelectControl... )
- LinkControlPopover, potentially this is useless and can be easily replaced with just Popover wrapping LinkControl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, LinkControl
should be independent of Popover
. You should be able to render the control wherever you want.
This PR or another one @youknowriad ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it for another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird that we grant that we want to remove the Popover rendering, in the same pull request where we're adding a new popoverProps
prop. I guess we don't think it's an issue in that the component is already "experimental" anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that was my thinking. Trying to limit the changes to implement this and separate the Popover from the Control in its own PR.
Noted.
|
ca5310f
to
b1595a7
Compare
This is looking cool, there's still some e2e tests to fix (probably due to different interactions, new UI) but we should get some design eyes. @jasmussen @karmatosed |
Wow, this is great work @youknowriad! It really fits in better than I thought it would without much adjusting. I don't see this yet on images so I guess this hasn't been added yet there? I can test each block once we do have it added though, for now, the feeling I have is that this is a great way forward to unify the link interface and stop having so many. |
b1595a7
to
e85f48a
Compare
e85f48a
to
f22a044
Compare
if ( addingLink ) { | ||
setMountingKey( uniqueId() ); | ||
} | ||
}, [ addingLink ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed in e2e tests that there's an expectation that the focus move back to the Popover if we hit "ctrl+k" on an existing link. This is a hack that does that (a similar more implicit hack was used before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We can iterate later on a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed in e2e tests that there's an expectation that the focus move back to the Popover if we hit "ctrl+k" on an existing link. This is a hack that does that (a similar more implicit hack was used before)
There might be another option here, based on the fact that the issue occurs because the popover is shown both when the caret is within a link (isActive
) and when adding/editing a link (addingLink
). We want to shift focus into the popover when editing the link, but not when we simply select it (click on it, or use arrow keys to change selection into it).
So ultimately it seems a question of: When the Popover's focusOnMount
prop changes from false
to 'firstElement'
, should we trigger the focus if the popover is already mounted? That would be enough to resolve the issue, and could be a valid way to control the focus behavior while the popover is already mounted (that's what's happening here). The main concern is... well, the name. It's called focusOnMount
, so it would be odd if we're enhancing it to support functionality specifically serving the use-case that the popover is already mounted 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A compromise solution might be to manually shift focus back into one of the popover elements when starting to edit the link. I could see this being something where:
- Either
Popover
orLinkControl
offer afocus
method. React's documenteduseImperativeHandle
example is exactly what we'd want in this case. - Or, the
InlineLinkUI
renders its ownref
-referenced DOM wrapper toLinkControl
, then tries to find a focusable target within the container to shift focus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this, It doesn't seem like there's a perfect solution. I might think a focus
method is maybe the most explicit and understandable solution. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the simplest "non-decision" option would just be to add a div
wrapper within the popover content, wrapping LinkControl
, then call wp.dom.focus.focusable.find
. Seems like that would be pretty straight forward.
The main downsides might be:
- It may be a common pattern for this sort of use of LinkControl in Popover
- If it becomes a worry, we could consider again some abstraction like URLPopover
- It duplicates a behavior inherent to Popover
Neither are really overly concerning to me. Maybe it could make sense to expose a focus
from Popover at some point, if it's a common need. It could even be reused internally for the component's own focusOnMount
behavior. I don't think we need to commit to that API now though. Similarly, creating a ref
behavior with LinkControl
isn't something I'd want to do without considering how to make this consistent with other *Control
components.
if ( ! isActive && ! addingLink ) { | ||
return null; | ||
if ( addingLink ) { | ||
stopAddingLink(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this here ensures that the focus moves back to the RichText somehow 🤷♂which is expected in e2e tests.
My personal expectation was that the focus should stay in the popover and potentially when I hit "Escape" it moves back to RichText. I decided to not pursue this right now in this PR because:
- There's no explicit API for a Format to say "move back to RichText"
- The RichText autofocus on format hack is still not removed
- I didn't want to change the behavior compared to master in this refactoring PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #19536. We can address it later.
@@ -364,42 +370,6 @@ describe( 'Links', () => { | |||
expect( assertiveContent.trim() ).toBe( 'Warning: the link has been inserted but may have errors. Please test it.' ); | |||
} ); | |||
|
|||
it( 'link popover remains visible after a mouse drag event', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because I believe it's not useful with the new UI.
f22a044
to
3c78359
Compare
@@ -43,16 +43,18 @@ const MODE_EDIT = 'edit'; | |||
|
|||
function LinkControl( { | |||
className, | |||
value, | |||
value = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What properties can this object have? Can they not be normal props instead of nested props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now: url, title, openInNewTab but it supports custom settings adding custom properties there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This should be documented (granted, not really the responsibility of the refactor)
- In this current implementation, there's some added chance of extra work on the React reconciler, since the value is passed through to
LinkControlSettingsDrawer
, and it would be different each time if not provided explicitly (not a consistent default object reference).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cause of some confusion.
The documentation refers to this as an "item", and the example includes some reference to an id
property:
Presumably, this is referring to some post object, like in the underlying URLInput onChange
second argumnet:
However: Even for URLInput
, this value is acknowledged to be null unless the user selects a post. So, what should the value be for when a user chooses to enter a raw URL? An id
won't exist, because there's no post associated with this URL.
@youknowriad , based on your comment, we could choose to make some custom shape which is an amalgamation of a post and various other link-specific values (opensInNewTab
appears to be one).
right now: url, title, openInNewTab but it supports custom settings adding custom properties there.
On this line of thinking, we should:
- Be very specific with what these are (I think the set you mention is agreeable, and could be enhanced later)
- Explicitly omit any other property values. Right now, we're spreading post properties into the changed value. I don't think we should want to set any expectation that these values will be available, as otherwise it becomes very error-prone when the user enters a custom URL which is not associated with a post.
- Fix any inaccuracies (i.e. remove the reference to
item.id
in the current documentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth This makes a lot of sense to me, I wasn't aware that we had these extra properties added there as well.
2d1add5
to
90f7b5d
Compare
I just force-pushed a rebased branch which should bring this up to date with the current master. There have been many changes to the affected components here, notably There's at least one remaining end-to-end test which is a legitimate, reproducible failure (links "should contain a label when it should open in a new tab"). I haven't quite figured out what's going wrong here. It could be related to circumstances where the caret selection is just within the inline boundary of a link. There's also a few rough edges which could be nice to improve, though I don't necessarily see as blocking:
|
// Click on the Edit button | ||
const [ editButton ] = await page.$x( '//button[text()="Edit"]' ); | ||
await editButton.click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make this change in behavior?
Notably, before these changes, upon pressing Cmd+K, the user would be able to edit the link immediately. Now, Cmd+K basically acts as a way to shift focus into the Popover attached to the link in the paragraph, but still in the "view"/"read-only" mode.
Before | After |
---|---|
It might be an okay change to make, if at least a conscious one.
I see both pros and cons:
- Con: More steps to get to the point of editing a link by keyboard alone.
- Pro: Avoids needing to create a way to forcefully control LinkControl's internal state (something like a
forceIsEditing
prop), which may encourage more consistency in how this component is used generally. - Pro: If one wanted to get to the "view" mode details of a link by keyboard alone, this change would make that workflow easier. There are fewer things that change in the UI if one expects Cmd+K purely as a means of navigating into the popover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this previously, but apparently Ctrl+K is pretty well-established as being associated with the intention of editing a link. So we probably want to retain the previous behavior.
control-K is often used to add, edit, or modify a hyperlink to a Web page
https://en.wikipedia.org/wiki/Control-K
The article notes Microsoft Word, and I've also confirmed it works this way in Google Docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this previously, but apparently Ctrl+K is pretty well-established as being associated with the intention of editing a link. So we probably want to retain the previous behavior.
There's a bit of a conflict here that we need to sort out.
Consider the following:
- Insert a paragraph with some text
- Select some or all of that text
- Press Cmd+K to start applying a link
- Input a URL and press Enter
Where should focus be at this point?
In the current plugin, the focus is moved back to the paragraph, so you can proceed to continue editing the text of the paragraph. I've observed this behavior in Google Docs as well.
But there's a problem with this for the new link editing experience, which is: How do I toggle "Open in New Tab" if I'm only using the keyboard? The only way for me to shift focus into the link popover is using Cmd+K, but this immediately puts me into editing the URL, and there's no way for me to access the "Open in New Tab" toggle, because we (no longer) show the toggle when editing the URL.
The options I might consider are:
- Cmd+K should just shift focus into the Popover. This breaks some widespread conventions / expectations around how Cmd+K tends to work, based on my earlier comment.
- Or: Pressing Enter when editing a link should keep focus within the Popover. This could work, but it could slow the workflow for those who might expect to be able to proceed with their writing immediately upon pressing Enter (I would expect pressing Escape at this point would be enough to put the caret back into the paragraph). There's also a technical challenge here, in that the formatting implementation seems to become confused when focus stays within the popover after the new link is applied.
- Or: Cmd+K can continue to immediately start editing the link, and Enter once finished editing a link will still shift focus back to the paragraph, but then we also add an additional way to be able to navigate into the Popover from an active link without triggering it as an edit.
- Edit: Or: Move focus back to the paragraph, but make it possible to navigate to the link popup using toolbar controls (currently, the behavior of the Link toolbar button when actively highlighting a link is to "Unlink").
- Edit: Or: Add a "Cancel" button to the link input which would toggle back to the "View" mode of the popup. See also Improve UX for closing the navigation link editor #19570. This could be confusing if a user might expect "Cancel" to mean "Close the popup" and not "Switch to view mode".
I was curious how Google Docs handles this, since they have a similar "popup", their behavior of Cmd+K is to immediately edit the URL, and their behavior of Enter once finished editing a URL is to shift focus back into the paragraph:
I'm actually not sure how one would be expected to access those "Copy Link" and "Remove Link" buttons in the popup using only keyboard:
There's a Keyboard Shortcuts panel accessible pressing Cmd+?, and it includes an entry for "Move focus to popup", but the key combination does not appear to work for these link popups.
Maybe it's assumed since there are other means to Remove or Copy a link in Google Docs, it's not necessary to have these buttons be accessible via keyboard. In our case, we're not able to make the same compromise, because there's no other way to toggle "Opens in New Tab".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like I'm capable of choosing which behavior makes the most sense.
My cartesian logic (and technical one probably) prefers that this popover behaves like any other popover which is:
- Ctrl + k or click to open popover in its initial state (input for new links, view for edited links)
- Enter in the input mode keeps focus in the popover
- Escape moves focus back to the paragraph
I believe this behavior is also good a11y wise but I understand that this could be confusing.
Any thoughts here @jasmussen @karmatosed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent conversation here. I would love input from skilled keyboard users on what is the most intuitive and elegant here. @MarcoZehe if you have time, perhaps? Your input would be valuable.
Google Docs and the popover that appears when you select a link does not seem like the best pattern to emulate as, and Andrew touches on this, focus remains in text, which means that pressing "tab" inserts a tab in the text.
However both the classic editor, and the specific ⌘K dialog in Google Docs, both seem like they could provide a pattern to follow here as well. In my reading, if focus is in an input field, pressing Enter closes the dialog and brings focus back to text. If focus is on a button, it toggles that button. With that heuristic, the following could work:
- Select text, press ⌘K, paste URL, press Enter and you're back in text.
- Select text, press ⌘K, paster URL, press Tab to set focus on the advanced dropdown. Press enter to open it. Press tab to set focus on "Open in new window". Press shift tab to go back to the Submit button, press Enter and you're back in text.
To my understanding, this is also the behavior of the classic editor:
It's entirely possible I'm missing some context from the new link dialog, but if not, does the above heuristic make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyboard focus can only be in one place at a time, and tab should always act predictably. Besides, why that extra step if I don't even want to set extra options? That sounds to me like an anti-pattern. So, I want to select text, press CTRL+K or CMD+K, paste the URL, enter to submit, or tab to the submit button and hit Enter or Space, and be back in the text to continue working. Shoving another dialog in my face to set options I might not even need would annoy me big times as a user, and also confuse me. That previous Link dialog that's also in the Classic editor where I can expand the advanced options if I need to, worked fine. If it must be a separate popover, make it a button in the main popover that opens this secondary one, and then submits the link with these set options once I okay/submit that secondary popover. Otherwise, keep the interaction as simple and straight-forward as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must apologize and admit that I totally missed the replies here before merging (GitHub threads can be hard to follow, especially for such a large pull request). Trying to follow the conversation as it has evolved here, I want to make sure that nothing is lost for consideration and that we don't regress behaviors. I'm also conscious that this link experience is being proposed to support more advanced use cases of the Navigation Link and Buttons blocks, and brought to the paragraph to ensure a consistent experience for editing links.
From what I'm gathering in this discussion, one thing that seems important is to restore the workflow that Cmd+k immediately puts the user into editing a link, and ideally to treat "Opens in New Tab" as part of a set of advanced options. I think it might require something of a rethink for how we present the link preview when the current selection is within a link. With the discussion here, it feels that it might be problematic to include the "Open in New Tab" as part of this preview, when it's more of an advanced editing feature.
I will make sure to follow up with relevant tasks and/or pull requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify how the current behavior will be after merging this pull request:
- Select some text
- Press Cmd+K
- Now focus is inside the popover in its preview state. From here, the user can tab to the "Edit" button or to the "Open in New Tab" button
- After clicking "Edit", the user can change the URL
- Pressing Enter to submit the changes will place focus back in the paragraph
To me, it seems the most problematic of these is the 3rd step, where pressing Cmd+K will shift focus into the preview popover, but there's still another step required before the user is able to edit the link. This was necessary, however, since in the current implementation of this link component, it's the only place where "Open in New Tab" can be toggled, and otherwise the option would not be reachable by keyboard. The alternative where focus remains in the popup after pressing Enter seems that it would be equally problematic.
It appears the problem may be more about where the "Open in New Tab" toggle is placed. If it's something we think should be part of the link editing workflow (optionally as an advanced step), then I don't think it makes sense that it's shown as part of this preview state. Pressing Cmd+K should allow me to immediately edit a link, and I should still be able to toggle the option to "Open in New Tab" without also breaking my expectation that submitting my changes by pressing Enter would put the focus back in the paragraph.
This is my understanding of the problem, but I would appreciate any thoughts or corrections. I would also require some design help to accommodate this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your careful consideration here, @aduth. I agree with @MarcoZehe about keeping the flow as simple as possible while allowing for access to the advanced options if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure this is tracked, I created an issue at #20007 to follow up on work related to this discussion.
The "Submit" button changes were sort of incidental to the primary goal of this pull request if I understand correctly. And since they were contributing to a number of the "Known issues", I extracted them separately to #19971 for more focused review and discussion. In the meantime, I think I will plan to rebase to remove that commit from this branch to try to consolidate the changes here. |
Co-Authored-By: Dave Smith <[email protected]> Co-Authored-By: Riad Benguella <[email protected]>
90f7b5d
to
48f0477
Compare
I've rebased and pushed another set of changes which I think should put this in an acceptable state. As far as I can tell, all tests should pass now. The "Submit" button changes were removed from the branch and proposed for separate discussion at #19971. I still have some uncertainty around the specific behaviors of Cmd+K and where focus should land after pressing Enter to finish editing a link. With the current set of changes, Cmd+K will shift focus into the popover, but the user will still need to click the "Edit" button to start modifying the link. Pressing Enter will shift focus back to the link in the paragraph so the user can continue typing. This aligns with the current behavior of the Navigation Link block, and ensures that the "Open in New Tab" toggle is still accessible by keyboard. Separately, we may still want to explore some of the alternative options discussed at #19462 (comment). The use of |
I'll plan to follow-up by creating discrete tasks out of the improvements I've mentioned in previous comments, but I would like to have this one included as part of this week's plugin release, ahead of next week's WordPress 5.4 feature freeze. Noting that there is another plugin release planned for next week as well, to include improvements. |
Thanks Andrew for the great work here. |
The LinkControl component has been developed specifically for the Navigation block but unless we use it everywhere elsewhere we need links, it's going to become very specific to the Navigation block.
So before adding a lot of features there that might not be generic enough, let's try to use it to replace the current Link control and other link UIs in other blocks.
This PR works but is still very bug. I'd appreciate help from experts :P cc @ellatrix @getdave
closes #19056
closes #19270