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

Ensure withFocusReturn's context props are not passed to wrappedElement #17354

Merged
merged 2 commits into from Sep 6, 2019

Conversation

@talldan
Copy link
Contributor

commented Sep 6, 2019

Related #17342.

Description

The withFocusReturn HOC was passing through an additional prop focusHistory to its wrapped component. It receives this prop from its context provider.

In most cases this wasn't an issue, the additional prop is ignored. However, some components, like Button pass all props straight through to a DOM element, and when those components are wrapped with withFocusReturn the result is a react warning about invalid prop types:

["Warning: React does not recognize the `focusHistory` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `focushistory` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
        in div (created by PostPublishPanel)
        in PostPublishPanel (created by WithConstrainedTabbing(PostPublishPanel))
        in div (created by WithConstrainedTabbing(PostPublishPanel))
        in WithConstrainedTabbing(PostPublishPanel) (created by FocusReturn)

This PR splits the props passed through to FocusReturn so that props received from context and props intended for the child are kept separate. Only childProps are passed to the wrapped component.

How has this been tested?

Added a unit test.

Also searched the codebase to ensure focusHistory isn't used by wrapped components.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
@talldan talldan self-assigned this Sep 6, 2019
@talldan talldan requested a review from pento Sep 6, 2019
@talldan talldan changed the title Ensure context props are not passed to wrappedElement Ensure withFocusReturn's context props are not passed to wrappedElement Sep 6, 2019
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { stubTrue, without, omit } from 'lodash';
import { stubTrue, without } from 'lodash';

This comment has been minimized.

Copy link
@talldan

talldan Sep 6, 2019

Author Contributor

Strange that I was able to commit this with a linting error 🤔

@pento
pento approved these changes Sep 6, 2019
Copy link
Member

left a comment

🕺🏻 This fixes the StrictMode error I was seeing.

@talldan talldan merged commit 6164851 into master Sep 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@talldan talldan deleted the fix/focus-return-passing-extra-props branch Sep 6, 2019
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
…nt (WordPress#17354)

* Ensure context props are not passed to wrappedElement

* Fix linting error
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.