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

Keep autoFocus attribute in the DOM #11192

Merged
merged 4 commits into from
Oct 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions fixtures/ssr/src/components/Page.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import React, {Component} from 'react';

import './Page.css';

const autofocusedInputs = [
<input key="0" autoFocus placeholder="Has auto focus" />,
<input key="1" autoFocus placeholder="Has auto focus" />,
];

export default class Page extends Component {
state = {active: false};
handleClick = e => {
Expand All @@ -18,9 +23,16 @@ export default class Page extends Component {
<p suppressHydrationWarning={true}>
A random number: {Math.random()}
</p>
<p>
Autofocus on page load: {autofocusedInputs}
</p>
<p>
{!this.state.active ? link : 'Thanks!'}
</p>
{this.state.active &&
<p>
Autofocus on update: {autofocusedInputs}
</p>}
</div>
);
}
Expand Down
6 changes: 6 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ var registrationNameModules = EventPluginRegistry.registrationNameModules;
var DANGEROUSLY_SET_INNER_HTML = 'dangerouslySetInnerHTML';
var SUPPRESS_CONTENT_EDITABLE_WARNING = 'suppressContentEditableWarning';
var SUPPRESS_HYDRATION_WARNING = 'suppressHydrationWarning';
var AUTOFOCUS = 'autoFocus';
var CHILDREN = 'children';
var STYLE = 'style';
var HTML = '__html';
Expand Down Expand Up @@ -286,6 +287,9 @@ function setInitialDOMProperties(
propKey === SUPPRESS_HYDRATION_WARNING
) {
// Noop
} else if (propKey === AUTOFOCUS) {
// We polyfill it separately on the client during commit.
// We blacklist it here rather than in the property list because we emit it in SSR.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's time to fork that file then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to rework how it's done entirely (see #10805), and it would be more convenient if it was still centralized before I did that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like that we have this extra special case here. :/ Mostly because I hope we can simplify this whole thing and special cases might hang around.

We could instead special case it on the server to minimize code. But I'm not sure that's any better for the maintainability. At least with this it's clear that it is probably the client that needs to change in the future.

} else if (registrationNameModules.hasOwnProperty(propKey)) {
if (nextProp != null) {
if (__DEV__ && typeof nextProp !== 'function') {
Expand Down Expand Up @@ -681,6 +685,8 @@ var ReactDOMFiberComponent = {
propKey === SUPPRESS_HYDRATION_WARNING
) {
// Noop
} else if (propKey === AUTOFOCUS) {
// Noop. It doesn't work on updates anyway.
} else if (registrationNameModules.hasOwnProperty(propKey)) {
// This is a special case. If any listener updates we need to ensure
// that the "current" fiber pointer gets updated so we need a commit
Expand Down
1 change: 0 additions & 1 deletion src/renderers/dom/shared/DOMProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ var invariant = require('fbjs/lib/invariant');
var RESERVED_PROPS = {
children: true,
dangerouslySetInnerHTML: true,
autoFocus: true,
defaultValue: true,
defaultChecked: true,
innerHTML: true,
Expand Down
1 change: 1 addition & 0 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var HTMLDOMPropertyConfig = {
// name warnings.
Properties: {
allowFullScreen: HAS_BOOLEAN_VALUE,
autoFocus: HAS_STRING_BOOLEAN_VALUE,
// specifies target context for links with `preload` type
async: HAS_BOOLEAN_VALUE,
// autoFocus is polyfilled/normalized by AutoFocusUtils
Expand Down
20 changes: 20 additions & 0 deletions src/renderers/dom/shared/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,26 @@ describe('ReactDOMServer', () => {
expect(numClicks).toEqual(2);
});

// We have a polyfill for autoFocus on the client, but we intentionally don't
// want it to call focus() when hydrating because this can mess up existing
// focus before the JS has loaded.
it('should emit autofocus on the server but not focus() when hydrating', () => {
var element = document.createElement('div');
element.innerHTML = ReactDOMServer.renderToString(
<input autoFocus={true} />,
);
expect(element.firstChild.autofocus).toBe(true);

// It should not be called on mount.
element.firstChild.focus = jest.fn();
ReactDOM.hydrate(<input autoFocus={true} />, element);
expect(element.firstChild.focus).not.toHaveBeenCalled();

// Or during an update.
ReactDOM.render(<input autoFocus={true} />, element);
expect(element.firstChild.focus).not.toHaveBeenCalled();
});

it('should throw with silly args', () => {
expect(
ReactDOMServer.renderToString.bind(ReactDOMServer, {x: 123}),
Expand Down