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

prevent firefox marking required textareas invalid #16578

Merged
merged 3 commits into from Sep 18, 2019

Conversation

@halvves
Copy link
Contributor

@halvves halvves commented Aug 27, 2019

Bug was caused by an IE10/IE11 bugfix dealing with the placeholder attribute and textContent. Solved by avoiding the IE bugfix when textContent was empty.

Closes #16402

Test Plan

Open up DOM test fixtures and compare (in Firefox)...

local: http://screeching-degree.surge.sh/textareas
to latest: http://screeching-degree.surge.sh/textareas?version=16.9.0

...and verify that textareas are no longer marked invalid on load when required.

Also confirm that the original IE bugfix is still functioning as intended.

Bug was caused by an IE10/IE11 bugfix dealing with the placeholder attribute and textContent. Solved by avoiding the IE bugfix when textContent was empty.

Closes #16402
@nhunzaker
Copy link
Collaborator

@nhunzaker nhunzaker commented Aug 27, 2019

@aweary no worries if you can't get to it, just flagging you about the original IE10/11 fix back in 4dd625a.

@sizebot
Copy link

@sizebot sizebot commented Aug 27, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

ReactDOM.render(<textarea value="" readOnly={true} />, container);

expect(counter).toEqual(0);
});

This comment has been minimized.

@nhunzaker

nhunzaker Aug 27, 2019
Collaborator

For others - we pulled this test from ReactDOMInput-test.js, which uses a similar technique to check mutations for an iOS Safari date bug.

I still think this is a weird test, and I wonder if it makes sense to create a common helper for this sort of thing.

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Awesome! I did see a red outline once (see inline comment), is that intended?

<b>
<i>not</i>
</b>{' '}
see a red aura, indicating the textarea is invalid.

This comment has been minimized.

@philipp-spiess

philipp-spiess Aug 27, 2019
Member

Can you explain this a bit better? I do see a red outline specifically when I focus the "Empty value prop string" textarea, hit any character on the keyboard, and then move the focus to one of the other textareas. Is this expected?

This comment has been minimized.

@nhunzaker

nhunzaker Aug 27, 2019
Collaborator

Interesting. Great catch! I don't think there is anything we can do about this though, assuming this is what's happening:

  1. A user enters text
  2. The onChange callback does not update state (no value change)
  3. The text is still entered, so React must assign an empty value to "control" it
  4. Firefox triggers validation on blur, and the input has been modified at this point, so the field is marked as invalid.

So controlling the input is at odds with input validation. I think this is okay for fields that don't allow any text input because they should really have readOnly on them instead of a blank change callback.

I think these are setup this way to avoid needing to scaffold out the controlled input code. Would it be less confusing if they were set up this way?

This comment has been minimized.

@philipp-spiess

philipp-spiess Aug 27, 2019
Member

Ah that seems reasonable. Maybe update the instructions then to be clearer about this case.

I think these are setup this way to avoid needing to scaffold out the controlled input code. Would it be less confusing if they were set up this way?

Not quite sure what you mean by that but I think what we have now is already better than what we had 🙂

This comment has been minimized.

@nhunzaker

nhunzaker Aug 27, 2019
Collaborator

Oh! I just meant adding the callbacks and state for the controlled inputs so that the values are editable. Right now we don't have them, I think because it isn't essential for the fixture.

This comment has been minimized.

@halvves

halvves Sep 10, 2019
Author Contributor

updated expected result to read "...see a red aura on initial page load, indicating the textarea is invalid..."

do you think this is sufficient clarification?

packages/react-dom/src/client/ReactDOMTextarea.js Outdated Show resolved Hide resolved
re: @philipp-spiess code review
@halvves halvves requested review from nhunzaker and philipp-spiess Sep 10, 2019
better describe the behavior we are testing for
re: @philipp-spiess code review
Copy link
Collaborator

@nhunzaker nhunzaker left a comment

@halvves this looks great to me. Later today, I'll conduct some more general browser testing. I need to make sure these changes don't have an impact on the IE placeholder fix.

Copy link
Collaborator

@nhunzaker nhunzaker left a comment

I've conducted browser testing cross IE, Firefox, Safari, and Chrome.

I think this is good to go, 👍. @philipp-spiess any final thoughts?

@nhunzaker
Copy link
Collaborator

@nhunzaker nhunzaker commented Sep 18, 2019

@halvves Thanks for being patient with us. This is good by me. Let's ship it.

@nhunzaker nhunzaker merged commit a5df18a into facebook:master Sep 18, 2019
12 checks passed
12 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.