Skip to content
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

Block Editor: LinkControl: Incorporate settings in edit state #20052

Merged
merged 16 commits into from Feb 10, 2020

Conversation

@aduth
Copy link
Member

aduth commented Feb 5, 2020

Closes #20007

This pull request seeks to resolve the issue described in #20007 where additional steps for editing links in paragraphs were introduced when refactoring the link component to use LinkControl. With these changes, the "Open in New Tab" toggle is always shown when viewing or editing a link. This allows for the behavior of Cmd+K to once again launch the user immediately into editing the URL of a link, where prior to these changes it required an additional step to switch from viewing to editing the link (clicking the "Edit" button).

  Previewing Editing
Before Previewing before Editing before
After Previewing after Editing after

Also included:

  • Replace "Reset" with "Submit"
  • Variable-based styling in place of hard-coded
  • Fixes selection placement when pressing Escape
  • Prevents focus shifting back to paragraph after toggling "Open in New Tab"

Testing Instructions:

Confirm the following workflow goals for working with links in paragraphs:

  1. Pressing Cmd+K will always allow you to immediately insert or edit the URL of a link
  2. Pressing Enter after editing the URL will immediately restore your selection to the paragraph
  3. You can toggle "Opens in New Tab" using only the keyboard
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 6, 2020

The changes in 5c39bd2 reflect an end-to-end test workflow for links editing that I would expect to be a target keyboard interaction.

I commented at #17557 (comment) to try to solicit advice on how to reincorporate this setting into the link editing.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 6, 2020

The recent commits change the implementation to one where the "Open in New Tab" toggle is shown both when previewing and when editing a link, essentially treating it at separate from the "form" that is the link entry. As it reverts to using a ToggleControl, changes to this setting will take effect on the link immediately, regardless of whether the dialog is in the preview or view state. Special care is required here to ensure that focus only shifts back to the link when the URL is changed, and not when the "Open in New Tab" setting is toggled (using the last of the approaches mentioned in #20007 (comment)).

Previewing Editing
Previewing Editing

The design inspiration is drawn from some of the earlier mockups in #17557, notably #17557 (comment) . One caveat to note based on the conversation in the other issue: because these toggle settings will always be visible, it does not scale especially well when many settings are rendered. Because this component is still experimental and the primary use-case at the moment is limited to "Open in new tab", I consider it an acceptable short-term compromise to optimize for resolving the current keyboard usability issues.

There are a couple other changes:

  • Cherry-picks changes from #19971 to replace "Reset" with "Submit"
  • Fixes behavior of pressing Escape while editing a link (previously, selection would reset to the beginning of the paragraph)
@phpbits

This comment has been minimized.

Copy link
Contributor

phpbits commented Feb 10, 2020

@aduth Would it be really okay to add "Experimental" component on the version that will be available on WordPress 5.4? Thanks!

Copy link
Contributor

draganescu left a comment

This works very well on the desktop but not so well on mobile.

Codewise it looks good.

Screenshot 2020-02-10 at 14 02 10

Screenshot 2020-02-10 at 14 00 11

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 10, 2020

Would it be really okay to add "Experimental" component on the version that will be available on WordPress 5.4?

Yes, Core already uses a dozen of exeperimental APIs. This means these are ready for internal usage but not ready for third-party usage because of potential breaking change that can happen.

@phpbits

This comment has been minimized.

Copy link
Contributor

phpbits commented Feb 10, 2020

Yes, Core already uses a dozen of exeperimental APIs. This means these are ready for internal usage but not ready for third-party usage because of potential breaking change that can happen.

Thanks a lot for the clarification. I'm just concern on third-party conflicts but since you've mentioned that there are lots of them, I'm sure it'll be okay. Thanks again!

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 10, 2020

@draganescu Can you confirm whether those issues are present in master as well? @jasmussen had previously noted a few mobile-related issues as well, but I suspect they are not directly related to the changes being proposed here.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 10, 2020

Keyboard navigation needs more work but it's better than master when I tested a week ago. There are a few small glitches then we need to clarify and eventually address during the Beta phase:

link-settings

  • there is no way to enter the edit mode for the link other than with cmd + k keyboard shortcut
  • you can navigate between suggestions with arrow keys, but when I press esc it closes the whole popover, my intent was to go back to the input field
  • when pressing space or enter to change the state of the checkbox (for opening in New Tab), sometimes the popover moves to the right
  • when opening the link popover from the block toolbar, the popover is placed in the position relative to the toolbar rather than the selected text, it would be also great to keep the indicator for the selected text

I'm using Safari and macOS.

I'll test with on iPad now.

Copy link
Member

ellatrix left a comment

Looks good. Mobile issues seem unrelated and possible to address separately.

Copy link
Contributor

youknowriad left a comment

This is looking good to me.

@@ -82,6 +78,11 @@ $block-selected-vertical-margin-child: $block-edge-to-content;
// Buttons & UI Widgets
$radius-round-rectangle: 4px;
$radius-round: 50%;
$icon-button-size: 36px;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 10, 2020

Contributor

I keep seeing these variables moving back and forth. (g2 branch too)

<LinkControl
value={ linkValue }
onChange={ onChangeLink }
forceIsEditingLink={ addingLink }

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 10, 2020

Contributor

I'd have preferred to avoid knowing that a "mode" exists outside the component but I don't have an alternative.

This comment has been minimized.

Copy link
@aduth

aduth Feb 10, 2020

Author Member

I'd have preferred to avoid knowing that a "mode" exists outside the component but I don't have an alternative.

I am on the fence about it. I think based on discussion in #18061, the statefulness of toggling between previewing and editing is a key feature of the component. I agree that this is (and should be) normally handled internal to the component itself, but I don't think it needs to be hidden so far as being a publicly-acknowledged feature.

Allowing these to be controlled is an awkward position, and I weighed whether this should justify some rethink about whether it actually be managed as internal state vs. provided by the rendering parent. I think the conveniences and primary use-cases of the component should still defend that it continue to exist as state of the component.

The use of force-prefixed state controlling props does have some prior art as well, notably in the PostPreviewButton component (forcePreviewLink, forceIsAutosaveable).

@aduth aduth dismissed draganescu’s stale review Feb 10, 2020

I've tested and confirmed that these issues exist, both in this branch and on master. Since they were not introduced by these changes, I'd not consider it a blocker. I will follow-up with an issue to ensure that the mobile issues are tracked for fix separately.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 10, 2020

there is no way to enter the edit mode for the link other than with cmd + k keyboard shortcut

Depending on what you mean by this, it might be the expected/intended behavior. The "Preview" mode is not one which is intended to include functionality not otherwise available to be edited, and thus will not receive focus. This was the original problem of trying to make these controls available, which caused a regression in the standard workflow described in #20007. To clarify, there should still be an option to insert or remove a link from the block toolbar, which would behave the same as it has since quite a while.

you can navigate between suggestions with arrow keys, but when I press esc it closes the whole popover, my intent was to go back to the input field

It might need to be worth some testing, since technically the focus should never have left the input field. Your selection and caret is still in the input even while navigating selections, and you can continue to make edits without pressing any other keys. If it's not clear, it would be worth exploring improvements to help clarify.

when pressing space or enter to change the state of the checkbox (for opening in New Tab), sometimes the popover moves to the right

Hm, that is strange. I've not seen it. I expect it could also be an issue prior to this pull request, since the toggle was already available, albeit only in the preview.

when opening the link popover from the block toolbar, the popover is placed in the position relative to the toolbar rather than the selected text, it would be also great to keep the indicator for the selected text

I agree, that sounds buggy. I can't actually reproduce this myself on either master or this branch, neither for a collapsed or uncollapsed selection. But the logic for positioning the popover was not affected by any of these changes, so I sense it is not directly related. If you are able to create a reliable set of steps to reproduce, would you mind to create an issue to follow up on?

@aduth aduth merged commit 6c591f4 into master Feb 10, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 10, 2020
@aduth aduth deleted the update/link-control-open-new-tab branch Feb 10, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 10, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 10, 2020

Depending on what you mean by this, it might be the expected/intended behavior. The "Preview" mode is not one which is intended to include functionality not otherwise available to be edited, and thus will not receive focus. This was the original problem of trying to make these controls available, which caused a regression in the standard workflow described in #20007. To clarify, there should still be an option to insert or remove a link from the block toolbar, which would behave the same as it has since quite a while.

My issue was that the preview looks exactly the same as in edit mode so it sounds like design challenge – how to make it easier to learn how to use the interface.

you can navigate between suggestions with arrow keys, but when I press esc it closes the whole popover, my intent was to go back to the input field

It might need to be worth some testing, since technically the focus should never have left the input field. Your selection and caret is still in the input even while navigating selections, and you can continue to make edits without pressing any other keys. If it's not clear, it would be worth exploring improvements to help clarify.

You are correct. I still can type in the input field. It's me having an issue with figuring it out :)

when pressing space or enter to change the state of the checkbox (for opening in New Tab), sometimes the popover moves to the right

Hm, that is strange. I've not seen it. I expect it could also be an issue prior to this pull request, since the toggle was already available, albeit only in the preview.

I suspect it's an issue with the popover and the selection re-rendering after the state updates. It feels like the popover needs more work. I don't see the reason why your changes would have impact on it.

when opening the link popover from the block toolbar, the popover is placed in the position relative to the toolbar rather than the selected text, it would be also great to keep the indicator for the selected text

I agree, that sounds buggy. I can't actually reproduce this myself on either master or this branch, neither for a collapsed or uncollapsed selection. But the logic for positioning the popover was not affected by any of these changes, so I sense it is not directly related. If you are able to create a reliable set of steps to reproduce, would you mind to create an issue to follow up on?

Steps to reproduce:

  1. Enter Edit mode in the text block.
  2. Selects text with arrow keys + shift.
  3. Press shift + tab until you reach the Link button the block toolbar.
  4. Press enter or space to launch the link popover
  5. Observe whether the link is positioned correctly.
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 10, 2020

Steps to reproduce:

  1. Enter Edit mode in the text block.
  2. Selects text with arrow keys + shift.
  3. Press shift + tab until you reach the Link button the block toolbar.
  4. Press enter or space to launch the link popover
  5. Observe whether the link is positioned correctly.

These are the steps I had followed originally, and I could not (and still cannot) reproduce 🤷‍♂ Do you think it could be browser-specific?

edit-link

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 10, 2020

I created an issue at #20146 to track the bugginess of the mobile display of this popover.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 11, 2020

Steps to reproduce:

  1. Enter Edit mode in the text block.
  2. Selects text with arrow keys + shift.
  3. Press shift + tab until you reach the Link button the block toolbar.
  4. Press enter or space to launch the link popover
  5. Observe whether the link is positioned correctly.

These are the steps I had followed originally, and I could not (and still cannot) reproduce 🤷‍♂ Do you think it could be browser-specific?

edit-link

As noted, I tested using Safari + macOS. Maybe it behaves differently when there are multiple lines in the Paragraph block and you select text on the very bottom? It's hard to tell :)

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 14, 2020

@gziolo Sorry I missed your earlier note about Safari. I'm having trouble testing this, but I created an issue at #20250 to make sure it's tracked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.