-
Notifications
You must be signed in to change notification settings - Fork 336
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
Use innerText
instead of textContent
#2980
Conversation
IE8 does not support `Node.textContent` [1] but does support `HTMLElement.innerText` [2]. This means at the minute in IE8: - we're seeing the error "'textContent' is null or not an object" on any page that uses the Character Count or Accordion (which also causes script execution to stop, affecting other components on the page) - the Accordion is missing text for the 'Show/Hide all sections' and 'Show/Hide section' buttons - the Character Count is displaying the fallback message According to MDN [3], the main differences between `Node.textContent` and `HTMLElement.innerText` are: - `textContent` gets the content of all elements, including `<script>` and `<style>` elements. In contrast, `innerText` only shows "human-readable" elements. - `textContent` returns every element in the node. In contrast, `innerText` is aware of styling and won't return the text of "hidden" elements. Moreover, since `innerText` takes CSS styles into account, reading the value of `innerText` triggers a reflow to ensure up-to-date computed styles. (Reflows can be computationally expensive, and thus should be avoided when possible.) - Both `textContent` and `innerText` remove child nodes when altered, but altering `innerText` in Internet Explorer (version 11 and below) also permanently destroys all descendant text nodes. It is impossible to insert the nodes again into any other element or into the same element after doing so. Other than triggering a reflow, I don't think any of these are relevant where we're currently using `textContent`, so we should use `innerText` in order to support these older browsers. (We might want to consider swapping back to `textContent` once we have dropped support for IE8 to avoid the unneccesary reflows) [1]: https://caniuse.com/textcontent [2]: https://caniuse.com/innertext [3]: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent#differences_from_innertext
Requesting a review from @romaricpascal as discussed on Slack as I want to double check there's not a reason I've missed why we need to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good in terms of changes. One thing that wasn’t flagged on MDN in that paragraph is that innerText will replace line breaks by <br>
s:
As a setter this will replace the element’s children with the given value, converting any line breaks into
elements.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText
I wouldn’t put it as a blocker, though, given the strings we translate, and the fact that the next release will be breaking so we can swap to textContent
then.
Now we throw if the textarea description is missing, that element needs to be in the DOM as well. Our use of `document.getElementById` also meant that the elements need to be added to the `document`. This meant that we also need some cleanup between tests as Jest JSDOM doesn't handle that between tests of a same file. Finally, we also needed to use `textContent` rather than `innerText` as[JSDOM does not support the later](jsdom/jsdom#1245), and we were only [using it for IE8](#2980), which we no longer need to support.
Now we throw if the textarea description is missing, that element needs to be in the DOM as well. Our use of `document.getElementById` also meant that the elements need to be added to the `document`. This meant that we also need some cleanup between tests as Jest JSDOM doesn't handle that between tests of a same file. Finally, we also needed to use `textContent` rather than `innerText` as[JSDOM does not support the later](jsdom/jsdom#1245), and we were only [using it for IE8](#2980), which we no longer need to support.
IE8 does not support
Node.textContent
but does supportHTMLElement.innerText
.This means at the minute in IE8:
According to MDN, the main differences between
Node.textContent
andHTMLElement.innerText
are:Other than triggering a reflow, I don't think any of these are relevant where we're currently using
textContent
, so we should useinnerText
in order to support these older browsers.(We might want to consider swapping back to
textContent
once we have dropped support for IE8 to avoid the unneccesary reflows)