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

RichText: restore focus after setting selection #17617

Open
wants to merge 10 commits into
base: master
from

Conversation

@ellatrix
Copy link
Member

ellatrix commented Sep 26, 2019

Description

Fixes #17505.

Attempt to NOT return focus to rich text after applying formatting, if focus has been moved elsewhere. This will now be the default for all buttons.

RichText will still allow focus to be set if it is desired, through onChange( value, { focus: true } ). This can be useful if you know that the currently focussed element will be removed from the DOM. This is the case for the link pop up UI.

How has this been tested?

See #17505.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Sep 27, 2019

Whether we return or don't return focus in any given situation, this is generally a difficult problem full of nuance.

For instance, with accessibility in mind, should we make a distinction between clicking on a toolbar control (like Bold) and selecting it with the keyboard? If I select a portion of text and move my mouse to toggle Bold, then I may expect to resume typing, thus focus should be returned to RichText. On the other hand, if I select text, then work my way up to the toolbar controls using Alt+F10 and some tabbing, then perhaps after toggling Bold with Space or Enter I would still like to tab to Italic or Link, especially since the toolbar is still visible. In that case, is my user expectation that I can still tab within the toolbar unreasonable? I have no idea!

@ellatrix ellatrix added this to In progress in WordPress 5.3 Must Have via automation Sep 27, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

@mcsf I couldn't agree more.

  1. I definitely think that focus should remain in the toolbar if you tabbed all the way there. Perhaps to use other tools, or change your mind.
  2. I also think that it would be good for focus to return to the text if you merely click the button... But this seems a more tricky situation to solve. Sometimes (the colour picker is a good example here) you don't want a mere click to return focus. In case of the link UI, there's also a lot more going on than just a click. Ideally, at the end, on submitting the link, I'd expect focus to go to the toolbar where you can view the link.

On the other hand, it kind of also makes sense that focus stays on the button, even if you merely clicked it. After all, you still clicked it, and therefore moved focus? I'm not sure about this one. Most editors do seem to return focus for a simple button click though.

Considering all this, it does seem best if rich text is not focussed after applying formatting, and to add exceptions from there.

It's also worth noting that selection is NOT focus. While we do not move focus, we still maintain the selection.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

@mcsf We could built that special focus return behaviour into RichTextToolbarButton.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Sep 27, 2019

Most editors do seem to return focus for a simple button click though.
[…]
It's also worth noting that selection is NOT focus. While we do not move focus, we still maintain the selection.

Yeah. I wonder if any editors handle this a bit more smartly, perhaps:

  1. Whether with the mouse or keyboard, pressing into the Bold control does move focus to it.
  2. Thus, any additional tabbing will move focus within the toolbar.
  3. However, because selection remains on a portion of text, the editor knows that certain interactions — specifically, moving around with the keyboard arrows — are to be caught so that, then and only then, focus can be returned to the actual rich text.
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Sep 27, 2019

@mcsf We could built that special focus return behaviour into RichTextToolbarButton.

Yeah, that sounds a bit in line with what I simultaneously commented. :)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

Ok, I've modified RichTextToolbarButton to focus the last active element (rich text) if the button is clicked with the mouse. If the button is pressed otherwise, focus will remain on the button. I think this is good behaviour now.

@@ -76,14 +82,14 @@ exports[`RichText should transform backtick to code 2`] = `
<!-- /wp:paragraph -->"
`;

exports[`RichText should update internal selection after fresh focus 1`] = `
exports[`RichText should undo backtick transform with backspace 1`] = `

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 27, 2019

Author Member

These tests were not in alphabetic order... Jest reordered them.

@ellatrix ellatrix force-pushed the try/rich-text-no-focus branch from 71d4b05 to 9de7520 Sep 27, 2019
@ellatrix ellatrix added the [Type] Bug label Sep 27, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

This PR is ready for review now.

@ellatrix ellatrix changed the title RichText: return focus after setting selection RichText: restore focus after setting selection Sep 27, 2019
Copy link
Member

jorgefilipecosta left a comment

Hi @ellatrix, this PR fixed the popover and seems an overall improvement 👍

I tested this PR together with advanced rich text plugin and I noticed the following issue not sure if related to this PR:
If we select some text and then go to the inline color format on the color popup we change the color to red using the input field, the color red is correctly applied, if we go to input field and type green the green is correctly applied, if then I type red again the color is not applied anymore and even if I change the color clicking on the visual color picker the color still does not applies.
Basically the color input works two times but stops working on the third.
Sep-27-2019 12-54-26

Another thing I noticed is that if I click on the bold or italic formats and then I press the arrow keys it moves the cursor on the paragraph but if I use code or subscript format then pressing right-left keys moves the focus on the toolbar buttons. I guess this is expected right given the new way we handle the focus?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

@jorgefilipecosta Thanks for testing! It seems that focussing the input field will also move selection... a document can normally have not more than one selection. I need to think about how we can solve this... :/ Also now I see that for any button in the collapsed menu, focus won't be set back to rich text, but to the toggle button. That's because we look at the last focussed element. The only fix I think it to pass the rich text element to RichTextToolbarButton.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

So, to recap the selection issue: I don't know how we can both allow rich text selection and input field selection. 🤔

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Sep 27, 2019

Testing PR #16014 with this branch produces an even stranger dancing behavior:
Sep-27-2019 13-13-02

It seems like problems only happen when the format contains an input field.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

Ok, found the issue. Working on a fix now.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

@jorgefilipecosta Does the latest commit fix the issue with the colour picker? To me it looks good now.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

I'll now try to fix the clicking for the buttons in the collapsed menu.

} else {
onChange( applyFormat( value, format ) );
onChange( applyFormat( value, format ), { focus: true } );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Sep 27, 2019

Member

This API is interesting is it possible to call onChange without any change? So for example in the color picker when the popovers close and we know that we will not apply additional color changes we would call onChange to move the focus back to the rich text as the inline link does.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Sep 27, 2019

Hi @ellatrix, I found no issues with the color picker from "advanced rich text" and with the color picker being implemented in #16014. Nice work 👍

Also now I see that for any button in the collapsed menu, focus won't be set back to rich text, but to the toggle button. That's because we look at the last focussed element.

Another side effect of this issue is that if we apply a code format and then a bold format, when we click the bold button the focus moves to the more richtext controls button and the tooltip becomes visible.

The only fix I think it to pass the rich text element to RichTextToolbarButton.

This idea seems like a possible path; maybe a reference can be passed using context, so users of RichTextToolbarButton are not aware of that. Any undesirable side effects of following this path?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 27, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Sep 27, 2019

Looking at the previous active element is not good, because in case
of the drop down buttons it would be the toggle button.

What if we never set the toggle button as the previous active element? When the more RichText Controls is the last focused moment we behave as we do if the last focused element was the richtext.

@youknowriad youknowriad removed this from In progress in WordPress 5.3 Must Have Oct 1, 2019
@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Nov 10, 2019

Hey Ella @ellatrix How is this PR coming along?

It would be great to get this merged as the work on #16014 and #8171 will be able to continue.

Thanks!

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