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

Correctly render placeholder for textarea in IE11 #8020

Merged
merged 3 commits into from
Nov 5, 2016

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Oct 19, 2016

Resolves #6731

In IE11, setting the node.placeholder will also set node.textContent, so when we ended up setting textContent to "" via DOMLazyTree.queueText in in _createInitialChildren , it caused the placeholder to never show up.

18d418a resolves this by checking if the <textarea/> has a placeholder and text is empty. If it is, it skips queueText.

We were also unconditionally setting node.value = node.textContent, which before 18d418a just meant we'd set it to an empty string, but now we're setting node.value equal to the placeholder in IE11.

07998a5 resolves this by only setting node.value if textContent is our expected initialValue.

Test plan

Tested with this component, which contains uncontrolled/controlled <textarea/>s with and without defaultValue/placeholder

Tested in the following browsers:

  • IE10
  • IE11
  • Firefox 49
  • Chrome 53
  • Safari 9
  • Safari 10
  • iOS 9 (Safari)
  • iOS 8 (Safari)

cc @spicyj @sebmarkbage

// TODO: Validate that text is allowed as a child of this node
if (__DEV__) {
setAndValidateContentChildDev.call(this, contentToUse);
if (shouldSetNodeTextContent(lazyTree, props, contentToUse)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just do contentToUse !== '' here (and move the comment you have above down here to explain). No need to check for textarea/placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I wasn't totally sure if there were situations where setting textContent to an empty string was valid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know of any. :)

// initial value. In IE10/IE11 there is a bug where the placeholder attribute
// will populate textContent as well.
// https://developer.microsoft.com/microsoft-edge/platform/issues/101525/
if (textContent === inst._wrapperState.initialValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, when is this not true?

Copy link
Contributor Author

@aweary aweary Oct 19, 2016

Choose a reason for hiding this comment

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

This is false in IE11 when placeholder has been set on a text area. There's a bug (linked in the comment above) where setting placeholder also sets textContent.

The bug report specifically mentions innerHTML and innerText, but it also effects textContent

@aweary aweary force-pushed the fix-placeholder-textarea-IE11 branch from 331aa88 to 1e3331d Compare October 19, 2016 23:17
@aweary
Copy link
Contributor Author

aweary commented Oct 19, 2016

@spicyj removed shouldUpdateTextNode and added that contentToUse check instead.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

My understanding is:

If you render <textarea defaultValue="A" /> then <textarea defaultValue="B" /> then .value should be A and you should see A in the textarea.

If you start with the empty string instead of A then .value should stay as '' (and you should see the placeholder).

Can you confirm that these still work? I'm concerned that in the second case, .value will change to B when you change defaultValue="" to defaultValue="B".

Accepting contingent on this working properly. If it doesn't, let's do another round.

if (__DEV__) {
setAndValidateContentChildDev.call(this, contentToUse);
// Avoid setting textContent when the text is empty. In IE11 setting
// textContent on a text area will cause the placeholder to not
Copy link
Collaborator

Choose a reason for hiding this comment

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

textarea

@aweary
Copy link
Contributor Author

aweary commented Nov 3, 2016

Can you confirm that these still work? I'm concerned that in the second case, .value will change to B when you change defaultValue="" to defaultValue="B".

@spicyj It looks like it retains the initial empty value and the placeholder remains visible. Here's a JSFiddle with a build that includes this change, demonstrating the behavior: https://jsfiddle.net/e2Lqbc8r/

@sophiebits
Copy link
Collaborator

Cool. I don't have an IE handy but if you tested that case, feel free to ship this.

Brandon Dail added 3 commits November 3, 2016 15:35
shouldSetNodeTextContent returns whether a node.textContent should be
updated. Currently it only covers one case, which is to avoid setting
the textContent if the text is empty and a placeholder exists.
In IE11 textContent is populated when the placeholder attribute is set.
Without this check, we end up setting node.value equal to the
placeholder text, causing the textarea to actually render with the text
inside.

This check makes sure that textContent is equal to our expected
initialValue, which should be the case when using defaultValue.
@aweary aweary force-pushed the fix-placeholder-textarea-IE11 branch from 1e3331d to 561b8f2 Compare November 3, 2016 20:36
@aweary
Copy link
Contributor Author

aweary commented Nov 5, 2016

Tested in IE11 and IE10 👍

@aweary aweary merged commit e644faa into facebook:master Nov 5, 2016
@aweary aweary added this to the 15-next milestone Nov 5, 2016
@gaearon gaearon modified the milestones: 15-hipri, 15-lopri, 15.4.2 Jan 6, 2017
gaearon pushed a commit that referenced this pull request Jan 6, 2017
* Check if textContent should be set for textarea

shouldSetNodeTextContent returns whether a node.textContent should be
updated. Currently it only covers one case, which is to avoid setting
the textContent if the text is empty and a placeholder exists.

* Only set node.value if it's equal to initialValue

In IE11 textContent is populated when the placeholder attribute is set.
Without this check, we end up setting node.value equal to the
placeholder text, causing the textarea to actually render with the text
inside.

This check makes sure that textContent is equal to our expected
initialValue, which should be the case when using defaultValue.

* Remove placeholder/textarea check, use contentToUse instead

(cherry picked from commit e644faa)
@gaearon
Copy link
Collaborator

gaearon commented Jan 6, 2017

This should be out in 15.4.2.
Please verify.

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
* Check if textContent should be set for textarea

shouldSetNodeTextContent returns whether a node.textContent should be
updated. Currently it only covers one case, which is to avoid setting
the textContent if the text is empty and a placeholder exists.

* Only set node.value if it's equal to initialValue

In IE11 textContent is populated when the placeholder attribute is set.
Without this check, we end up setting node.value equal to the
placeholder text, causing the textarea to actually render with the text
inside.

This check makes sure that textContent is equal to our expected
initialValue, which should be the case when using defaultValue.

* Remove placeholder/textarea check, use contentToUse instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textarea placeholder isn't shown in IE 11 being rendered using React
4 participants