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

Rich Text: Add missing deprecated set focused element prop. #17421

Merged

Conversation

@epiqueras
Copy link
Contributor

commented Sep 12, 2019

Refs #17405

Description

This PR fixes backwards compatibility issues that came with the unintentional removal of the setFocusedElement prop in RichText.

It does this by adding the prop back in with a deprecation message.

How has this been tested?

The issues in #17405 were verified to be resolved.

Types of Changes

Bug Fix: Fix backwards compatibility issues arising from accidentally removing the deprecated setFocusedElement prop from RichText.

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.
@afercia

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

@epiqueras thanks for working on this! Seems it fixes the specific blocks issue mentioned on #17405 as an example of backwards compatibility breakage.

However, please consider #17405 is a broader issue and there are other things to address before it can be called "fixed". It's not related to a specific plugin: other plugins may break as well and I'd tend to think there's the need to explore potential additional problems a bit more in depth. I'd like to ask you to not close #17405 when this PR will be merged.

As mentioned in #17405, at the very least:

  • The keepPlaceholderOnFocus prop was removed: this should be documented and a backwards compatibility mechanism should be implemented. Maybe with CSS as @ellatrix suggested.
  • Several props have been renamed and marked as unstable: as @gziolo mentioned on Slack, there's the need to investigate RichText changes, make sure there are no breaking changes, and translate old props to their alternatives as close as possible if necessary

Aside: on this PR, I'm seeing keyboard navigation with the Tab key is completely broken on the Yoast SEO blocks. It's not possible to Tab away from a block and something triggers a very bad behavior: focus gets moved automatically through blocks in a sort of infinite loop. See attached animated GIF where I'm using a red outline to highlight where focus is.

loop

Note the GIF frame rate alters a bit what really happens visually (seen live, the loop is much faster).

I think this terrible behavior is related to incompatibility between the Gutenberg new edit/navigation modes and the Yoast blocks internal focus management mechanism. Not 100% sure though, will investigate further.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Thanks both for your work, testing and identifying the regressions.

I've done a quick audit of the component and I've noticed that setFocusedElement and keepPlaceholderOnFocus were removed. One of them is restored on this PR, the second one should have a dev note at least (I added the tag to the corresponding PR)

I tried tracking the unstable props but AFAIK, these are unstable props for the new wp.richText.RichText component which is a completely new component that didn't exist in WP 5.2. The wrapper component (the wp.blockEditor.RichText) has the same props (aside the exceptions above).

It is very hard to confirm though, because it's not easy to navigate the history of that component, so I'd appreciate a confirmation from @ellatrix

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

Should we find a way to add keepPlaceholderOnFocus back in?

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

I tried tracking the unstable props but AFAIK, these are unstable props for the new wp.richText.RichText component which is a completely new component that didn't exist in WP 5.2. The wrapper component (the wp.blockEditor.RichText) has the same props (aside the exceptions above).

Not quite following. What do you want to track?

Should we find a way to add keepPlaceholderOnFocus back in?

I can add it or we can add a note... Either using CSS to force the placeholder to display on focus.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Not quite following. What do you want to track?

If there are any props from the RichText component in 5.2 that have been removed/renamed aside the two above.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Don’t think so. I’ll restore the placeholder one with a PR tonight

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Thanks everyone, let's merge this one and restore the placeholder one. I think that will cover all the potential regressions.

@youknowriad youknowriad merged commit 41920bb into master Sep 14, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the fix/rich-text-missing-deprecated-set-focused-element-prop branch Sep 14, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
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.