Drop .textContent IE8 polyfill and rewrite escaping tests against public API #11331

jeremenichelli · 2017-10-22T23:21:16Z

I want to put some explanation about this test rewrite since I think the outcome might not be what was intended in #11299

escapeTextContentForBrowser is used in client/setTextContent.js only when textContent is not supported for DOM elements, which only happens in IE8 (caniuse.com reference http://caniuse.com/#feat=textcontent) a browser version that is no longer supported (https://reactjs.org/blog/2016/01/12/discontinuing-ie8-support.html).

To actually move this test to run over ReactDOM.render and React.createElement it would be necessary to mock up the entire global.document object, assign null to document.documentElement.textContent, require React packages and then test this util through React element rendering. If authors and contributors think this should be the way to go I can start working again on this approach.

Considering that this part of the code might not be running anywhere today in React web applications, my suggestion would be to not add this heavy lifting on the tests, reconsider modifying these tests by merging the changes I've made and actually start thinking about removing escapeTextContentForBrowser from the codebase.

Since the support was dropped almost two years ago and we should start considering removing IE8 code.

Looking forward to read others take on this :)

syranide · 2017-10-24T06:21:45Z

var testElement = document.createElement('div');
testElement.innerHTML = escapeTextContentForBrowser('&');
expect(testElement.innerHTML).toEqual('&amp;');

Does not actually test escapeTextContentForBrowser for correct behavior. It would pass even if you just did testElement.innerHTML = '&';, which is clearly broken.

If you want to test this in-terms of the public API I'm guessing it would be a better idea to do something along the lines of

React.render(<div>{'&amp;'}</div>, target);
target.firstChild.textContent === '&amp;';

jeremenichelli · 2017-10-24T13:17:00Z

@syranide true that, as I described above the function I'm trying to test only runs in IE8, which could mean it's not used at all today in production.

My best bet is to work again on this PR, mock up the global.document object to mimic the inexistance of the textContent property in elements.

syranide · 2017-10-25T07:23:07Z

@jeremenichelli I'm pretty sure it's used for SSR too, which I'm guessing would be the actual way to test this (my code doesn't actually test it I realize 😄). It should be used for SSR at least.

jeremenichelli · 2017-10-25T13:10:21Z

Ah that's a good catch @syranide, I don't know how SSR works in React to be honest, but if it passes the ExecutionEnvironment.canUseDOM condition and 'textContent' in document.documentElementis false there too, then it will get called for sure.

I'm travelling to Buenos Aires for NodeConfAR now, so updates on this PR will have to wait til next week.

gaearon · 2017-10-26T19:59:37Z

If you're confident something is only used in IE8, let's change this PR to delete that instead.

jeremenichelli · 2017-10-30T14:59:15Z

@gaearon I'm 100% sure this only applies in IE8 cases (which implemented innerText instead of textContent), here's a list of references which support the statement:

The only thing I need to know is if this condition passes while server side rendering:

react/packages/react-dom/src/client/setTextContent.js

Line 43 in 087c48b

if (!('textContent' in document.documentElement)) {

If other contributors or users make sure SSRR doesn't need this, I will go on that path for sure.

gaearon · 2017-10-31T14:20:20Z

The only thing I need to know is if this condition passes while server side rendering:

SSR doesn't need this.

jeremenichelli · 2017-10-31T18:42:15Z

After some investigation, I noticed that escapeTextContentForBrowser is being used by the ReactPartialRenderer module from the server section of the package.

Also it's used by quoteAttributeValueForBrowser, for server rendering apparently.

Ideally the new roadmap would be to delete the textContent check in the client side since it's only there for IE8 as clarified before. Move the code to the shared folder and rename it escapeText since it's not used in the browser and it isn't applied in the text content situation itself, this will make the use of the code more clear.

We can re-check if the server tests satisfies the approach for quoted attributes and open a new issue.

Let me know if you agree with this new direction.

gaearon · 2017-11-01T11:23:00Z

Sounds reasonable to me.

…actDOMServer API

jeremenichelli · 2017-11-08T04:08:35Z

Just updated this PR. I want to clarify the changes first.

I've renamed escapeTextContentForBrowser to escapeText since it's just a util used by quoteAttributeValueForBrowser which is only used for server rendering.
I've moved tests on ReactServerRendering-test.js related to attribute value escaping.
On quoteAttributeValueforBrowser-test.js I'm testing directly through ReactDOMServer API to check if attribute values are correctly escaped.

A final note is that while doing this I didn't noticed tests for quoteAttributeValueForBrowser file were taken by @andrevargas in the original thread, and I think it would be the right thing to let him know if it's OK to merge this (after review approval) or if he first wants to show his PR to the team; and if the team prefers to merge his that I'll be totally fine with it. People before code 💟

syranide · 2017-11-08T08:30:25Z

I've renamed escapeTextContentForBrowser to escapeText since it's just a util used by quoteAttributeValueForBrowser which is only used for server rendering.

IMHO, that seems like a bad idea. There are many different ways to escape text/strings for the DOM, this is just one of them. At best it's confusing, at worst someone may use it to escape something they shouldn't (CSS, etc). EDIT: Although since it's being used for quoteAttributeValueForBrowser, the name of this function should at least exclude Content from TextContent, because TextContent implies it's only safe to use for textContent (which does not inherently make it safe for quoted attribute values).

Also, I don't understand how it can only be used by quoteAttributeValueForBrowser, how does the server-side escape inline text if not with this function?

@gaearon I'm also not sure why the current implementation is there, we had a far simpler and faster one "a long time ago"?

Unrelated but the description of escapeHtml is hilariously bad, "Escape special characters in the given string of html.".

andrevargas · 2017-11-08T11:20:34Z

Hey, thanks for letting me know about this, @jeremenichelli! I haven't figured out how exactly to test quoteAttributeValueforBrowser-test.js until now, and that's why I haven't sent any PR yet.

I didn't realize I could use ReactDOMServer API. Now I've taken a look at your PR and your tests for quoteAttributeValueForBrowser seem to be very reasonable.

It's awesome that you have done it! It's OK to merge this if approved, I'll be totally fine with it too! 👍
I just want to keep an eye on this PR, and let me know if I can help you with something. Again, thank you for caring about my efforts.

jeremenichelli · 2017-11-08T12:31:02Z

Thanks @andrevargas 💛

@syranide do you agree with changing the name to escapeStringForBrowser and improving description on escapeHtml method?

gaearon · 2017-11-13T15:44:20Z

@syranide Do you plan to continue reviewing this PR?

jeremenichelli · 2017-11-13T15:51:59Z

@syranide @gaearon to proceed I need to know if we agree on escapeStringForBrowser name, improving escapeHtml description, and maybe re-add direct escapeStringForBrowser tests (no through APIs) or through ReactDOMServer.

syranide · 2017-11-13T16:15:38Z

@gaearon Sorry, no time this week (full-day business meetings) and I'm not really on-top of your internal guidelines well enough to suggest something more than what I've already done I feel. My intention was just to add what I know/felt was missing seeing as I've touched this code before.

@syranide do you agree with changing the name to escapeStringForBrowser and improving description on escapeHtml method?

Sure. The implementation of escapeHtml seems outside the scope of this PR though (and the name itself is bad too, also no one will ever read that description 😄).

jeremenichelli · 2017-11-13T17:09:47Z

@syranide well it's a small gardening, and it wouldn't harm the PR talk if we agree on one ahead of new commits. How about escapeHtmlEntities?

gaearon · 2017-11-13T20:57:06Z

I'm not really on-top of your internal guidelines well enough to suggest something more than what I've already done I feel

We're pretty separate from any internal guidelines by now. I fully trust your intuition here. :-)

That said totally understand if you can't find the time. I'll try to dig in next week.

syranide · 2017-11-14T18:54:05Z

That said totally understand if you can't find the time. I'll try to dig in next week.

@gaearon Just unusually busy this week, should have time this weekend :)

gaearon · 2017-11-20T19:32:14Z

packages/react-dom/src/__tests__/quoteAttributeValueForBrowser-test.js

-  it('should escape boolean to string', () => {
-    expect(quoteAttributeValueForBrowser(true)).toBe('"true"');
-    expect(quoteAttributeValueForBrowser(false)).toBe('"false"');
+    // TODO: can we express this test with only public API?


We can. Just embed it here :-)

Lol, forgot to take it out.

gaearon · 2017-11-20T19:33:03Z

packages/react-dom/src/__tests__/escapeTextContentForBrowser-test.js

-    expect(escapeTextContentForBrowser(false)).toBe('false');
-  });
-
-  it('should escape object to string', () => {


Which of the new tests verifies this?

gaearon · 2017-11-20T19:33:42Z

packages/react-dom/src/__tests__/quoteAttributeValueForBrowser-test.js

  });

-  it('should escape number to string', () => {
-    expect(quoteAttributeValueForBrowser(42)).toBe('"42"');


Same question.

gaearon · 2017-11-20T19:34:43Z

packages/react-dom/src/__tests__/quoteAttributeValueForBrowser-test.js

  });

-  it('should escape string', () => {
-    var escaped = quoteAttributeValueForBrowser(
-      '<script type=\'\' src=""></script>',


Please add a test where this is literally the argument value.

@gaearon I'm thinking of creating tests for escapeText through the ReactDOMServer.renderToString API like these ones, do you think that it would make sense to write them? Also, if you do, shouldn't this test be placed there where the script would actually run?

I'm thinking of creating tests for escapeText thourgh the ReactDOMServer.renderToString API like these ones

Sounds goood.

Also, if you do, shouldn't this test be placed there where the script would actually run?

Not sure what you mean. We just want to verify <script> can't be injected as attribute value.

Great, will add test on both escapeText and quoteAttribute tests.

gaearon

See above comments: let's make sure there is a corresponding test for each case we used to test before.

…dd missing tests

jeremenichelli · 2017-11-20T22:25:57Z

packages/react-dom/src/__tests__/quoteAttributeValueForBrowser-test.js


-    escaped = quoteAttributeValueForBrowser('&');
-    expect(escaped).toBe('"&amp;"');
+  it('number is escaped to string inside attributes', () => {


@gaearon from here, tests for number, object and script tags on attributes have been added back. Above tests for text content in nodes contemplate these cases too, let me know if you ahve any more concerns or something that I might have missed.

gaearon · 2017-11-23T22:20:34Z

packages/react-dom/src/__tests__/quoteAttributeValueForBrowser-test.js


-    escaped = quoteAttributeValueForBrowser('&');
-    expect(escaped).toBe('"&amp;"');


So, the nice thing about these old tests is they didn't assert how we escape, only that we do escape. Which opens the door for changing that in the future.

Maybe it's not too important though.

What I didn't like form this test is that thy are just testing a switch JavaScript statement by doing it directly from the module. These internal modules pass thourgh other checks and algorithms when applied, so someone could modify that code, prevent the script to actually escape the characters and test would still pass, something that won't happen now 😃

Maybe an ideal test would actually create a DOM node, put content into innerHTML and then assert that it only has a text node child with expected content?

Yeah, that could be one way. Though werid since this is used only on the server package, maybe adding cases from renderToStaticMarkup would be valuable.

Though werid since this is used only on the server package

Not sure what you mean. The end goal of rendering to a string is that the HTML shows up in somebody's browser :-) This is exactly what an integration test should verify: that what shows up in the browser would have been consistent with what we expect (as opposed to a dangerous script tag).

I understood it better after writing my response but didn't want to edit it.

Yeah I guess that would mimic the text string from server to node conversion better than just testing the string. Let me know if you are planning on migrating to that. I might think about revisiting these tests in the future if necessary.

If you'd like to send a PR for this, happy to review. I'm not 100% sure it's the right approach but it seems to make sense to me.

…lic API (facebook#11331) * Rename escapeText util. Test quoteAttributeValueForBrowser through ReactDOMServer API * Fix lint errors * Prettier reformatting * Change syntax to prevent prettier escape doble quote * Name and description gardening. Add tests for escapeTextForBrowser. Add missing tests * Improve script tag as text content test * Update escapeTextForBrowser-test.js * Update quoteAttributeValueForBrowser-test.js * Simplify tests * Move utilities to server folder

facebook-github-bot added the CLA Signed label Oct 22, 2017

Rename escapeText util. Test quoteAttributeValueForBrowser through Re…

a8785a5

…actDOMServer API

jeremenichelli force-pushed the escapeTextContentForBrowser-test-improve branch from dd7feec to a8785a5 Compare November 8, 2017 04:02

Fix lint errors

4214c61

jeremenichelli added 2 commits November 10, 2017 15:13

Prettier reformatting

e6da941

Change syntax to prevent prettier escape doble quote

4727856

gaearon reviewed Nov 20, 2017

View reviewed changes

gaearon requested changes Nov 20, 2017

View reviewed changes

gaearon mentioned this pull request Nov 20, 2017

Express more tests via public API #11299

Closed

26 tasks

Name and description gardening. Add tests for escapeTextForBrowser. A…

52b3f90

…dd missing tests

jeremenichelli commented Nov 20, 2017

View reviewed changes

jeremenichelli and others added 5 commits November 20, 2017 19:46

Improve script tag as text content test

095dd70

Merge branch 'master' into escapeTextContentForBrowser-test-improve

ca12b9e

Update escapeTextForBrowser-test.js

895a01c

Update quoteAttributeValueForBrowser-test.js

3a36d1a

Simplify tests

058babe

gaearon reviewed Nov 23, 2017

View reviewed changes

gaearon changed the title ~~Gardening and improvements on escapeTextContentForBrowser tests~~ Drop .textContent IE8 polyfill and rewrite escaping tests against public API Nov 23, 2017

Move utilities to server folder

f6f503d

gaearon approved these changes Nov 23, 2017

View reviewed changes

gaearon merged commit cafe352 into facebook:master Nov 23, 2017

gaearon mentioned this pull request Dec 20, 2017

Clarify reason for setTextContent helper #11813

Merged


		escaped = quoteAttributeValueForBrowser('&');
		expect(escaped).toBe('"&"');

Drop .textContent IE8 polyfill and rewrite escaping tests against public API #11331

Drop .textContent IE8 polyfill and rewrite escaping tests against public API #11331

Conversation

jeremenichelli commented Oct 22, 2017

syranide commented Oct 24, 2017 • edited Loading

jeremenichelli commented Oct 24, 2017

syranide commented Oct 25, 2017 • edited Loading

jeremenichelli commented Oct 25, 2017

gaearon commented Oct 26, 2017

jeremenichelli commented Oct 30, 2017

gaearon commented Oct 31, 2017

jeremenichelli commented Oct 31, 2017 • edited Loading

gaearon commented Nov 1, 2017

jeremenichelli commented Nov 8, 2017

syranide commented Nov 8, 2017 • edited Loading

andrevargas commented Nov 8, 2017

jeremenichelli commented Nov 8, 2017

gaearon commented Nov 13, 2017 • edited Loading

jeremenichelli commented Nov 13, 2017

syranide commented Nov 13, 2017 • edited Loading

jeremenichelli commented Nov 13, 2017

gaearon commented Nov 13, 2017

syranide commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremenichelli Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide commented Oct 24, 2017 •

edited

Loading

syranide commented Oct 25, 2017 •

edited

Loading

jeremenichelli commented Oct 31, 2017 •

edited

Loading

syranide commented Nov 8, 2017 •

edited

Loading

gaearon commented Nov 13, 2017 •

edited

Loading

syranide commented Nov 13, 2017 •

edited

Loading

jeremenichelli Nov 20, 2017 •

edited

Loading

gaearon Nov 25, 2017 •

edited

Loading