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

Changing a RichText value automatically focus the RichText #17505

Closed
jorgefilipecosta opened this issue Sep 20, 2019 · 11 comments · Fixed by #19536
Closed

Changing a RichText value automatically focus the RichText #17505

jorgefilipecosta opened this issue Sep 20, 2019 · 11 comments · Fixed by #19536
Assignees
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] A11y /packages/a11y [Package] Rich text /packages/rich-text [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Sep 20, 2019

Describe the bug
Currently, if we change a RichText value the RichText gets focused and becomes the active element event if another element was focused and rich text value is changed using a programmatic interface e.g: wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( block.clientId, { content: '1<strong>234</strong>56789' } );.

Previously the popover was closed when a click outside them happened; now they are closed when the focus is outside them. This change makes the rich text automatically gaining focus problem worse. For example, if we have popover with a SelectControl that allows switching the content of a paragraph between some predefined values when the select is used, the popover closes.

Another side effect is that on PR #16014 that adds custom color as a format, each time we try to use the color picker the popover closes, because when we use the color picker there is a change in the format leaving to a change to the content of paragraph and making the focus go outside the popover.
I guess we should address this issue before WordPress 5.3 because this problem may affect third-party plugins that add color options or even other option in a popover.

I guess we may also have a11y related problems with moving the focus.

To reproduce
Steps to reproduce the behavior:

  • Insert paragraph block:
const block = wp.blocks.createBlock( 'core/paragraph', { content: '123456789' } );
wp.data.dispatch( 'core/block-editor' ).insertBlock( block );
  • Put the cursor inside the paragraph e.g. in the middle of the paragraph.
    Focus some element on the page (and verify it gets focused):
document.querySelector( 'button.editor-block-navigation' ).focus();
console.log( document.activeElement );
  • Programatically change the paragraph content and verify the paragraph becomes the active element:
	wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( block.clientId, { content: '1<strong>234</strong>56789' } );
	console.log( document.activeElement );

Sep-20-2019 18-50-38

cc: @ellatrix

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Backwards Compatibility Issues or PRs that impact backwards compatability [Package] A11y /packages/a11y [Package] Rich text /packages/rich-text labels Sep 20, 2019
@youknowriad youknowriad added the [Priority] High Used to indicate top priority items that need quick attention label Sep 23, 2019
@youknowriad
Copy link
Contributor

This looks like high priority for me as it may have broken a lot of plugins. Any ideas how we can solve it?

@jorgefilipecosta
Copy link
Member Author

This looks like high priority for me as it may have broken a lot of plugins. Any ideas how we can solve it?

I don't think RichText regressed it probably was always like that when a change happens the RichText gets focus. The problem is that if we want to keep the behavior of closing popovers when the focus is outside them we need to change this RichText behavior.
For now, I don't have an idea about the direction to solve this inside RichTex, unfortunately, I'm not very knowledgeable on its internals.
But if you think it is worth it, I can shift my priorities and look into how to address this inside RichText.
cc: @ellatrix in case you know an easy solution for the issue.

I tested a third-party plugin "Mighty Blocks" and its functionality to choose a background or text color partially in a paragraph is affected as expected and the color picker popover closes every time a "color move" happens making it very hard to use.

@ellatrix ellatrix self-assigned this Sep 24, 2019
@ellatrix
Copy link
Member

I’ll look into it asap.

@ellatrix
Copy link
Member

Screenshot 2019-09-25 at 12 33 52

I'm not sure if I can reproduce this.

@ellatrix
Copy link
Member

I see now in the instructions to focus RichText, then focus outside, but this was the behaviour before as well? Could you share the user experience that is currently broken with the popovers you mention?

@phpbits
Copy link
Contributor

phpbits commented Sep 25, 2019

@ellatrix The custom color selection in this pull request of mine : #16014

You could also try the issue on this plugin of yours : https://wordpress.org/plugins/advanced-rich-text-tools/ . The custom color picker selection is closing automatically.

Screen Capture on 2019-09-25 at 20-39-06

Let me know your thoughts. Thanks!

@jorgefilipecosta
Copy link
Member Author

I see now in the instructions to focus RichText, then focus outside, but this was the behaviour before as well? Could you share the user experience that is currently broken with the popovers you mention?

Hi @ellatrix, yes to reproduce the focus needs to be inside RichText, and then programmatically focus another element, when the rich text changes even if the user did not interact with it (programmatic change) the RichText will get focused.
I think you are right and this was the behavior before what made the problem noticeable was that the popovers now autoclose if the focus is not inside them. So for example on advanced-rich-text-tools when one applies any type of color changes the color popover closes making it unusable.
For a11y reasons we need to keep the behavior of closing unfocused popovers, so the solution to the problem is making sure RichText does not gains focus when a value change happens.

@ellatrix
Copy link
Member

When was the change to popovers introduced? I'll think about it. Some buttons do rely on rich text gaining focus after a change.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Sep 25, 2019

I think it was introduced in this PR #14851. More or less eight weeks ago.

@phpbits
Copy link
Contributor

phpbits commented Oct 6, 2019

@ellatrix @jorgefilipecosta Found another issue which is probably related to RichText focus. I'm trying to display Modal after consecutive empty paragraphs and focusOnMount seems not working on the Gutenberg plugin version. The cursor is always focused on the RichText, thus clicking TAB doesn't navigate to the modal contents.

Here's the screenshot where the cursor is still on the RichText even if Modal is opened.
Screen Capture on 2019-10-07 at 00-40-50

Working on the core editor though :

Screen Capture on 2019-10-07 at 00-30-22

@phpbits
Copy link
Contributor

phpbits commented Oct 7, 2019

Checked the issue with Modal on the PR fixes and I had to do this to prevent it from happening :

https://github.com/phpbits/block-options/blob/292c19b117eb7153e0c65ddbe9a3d03b267a41a9/src/extensions/transform/empty-paragraphs/components/controls.js#L28

@ellatrix Do you by any chance created function that I can use to remove/reset focus from RichText? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] A11y /packages/a11y [Package] Rich text /packages/rich-text [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants