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

Don't wrap text in <span> elements #5753

Merged
merged 1 commit into from Feb 18, 2016

Conversation

Projects
None yet
9 participants
@mwiencek
Contributor

mwiencek commented Dec 30, 2015

Removes the <span> wrappers from text nodes, and handles cases where the browser may have split or merged adjacent text nodes.

Sorry if this is a totally insufficient way of going about this. It seems to work fine for my needs. If you can point me in a different direction I'm happy to keep working on it.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 30, 2015

Contributor

Worth mentioning; https://bugzilla.mozilla.org/show_bug.cgi?id=194231, Node.normalize() and possibly other scenarios. While relatively unlikely it's apparently fragile to rely on individually created adjacent text nodes, the only truly safe option seems to be to treat all adjacent text nodes as one.

Contributor

syranide commented Dec 30, 2015

Worth mentioning; https://bugzilla.mozilla.org/show_bug.cgi?id=194231, Node.normalize() and possibly other scenarios. While relatively unlikely it's apparently fragile to rely on individually created adjacent text nodes, the only truly safe option seems to be to treat all adjacent text nodes as one.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Dec 30, 2015

Contributor

Node.normalize() does seem to break things. I'll add a test and see if the other commit works with it.

Contributor

mwiencek commented Dec 30, 2015

Node.normalize() does seem to break things. I'll add a test and see if the other commit works with it.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 30, 2015

@mwiencek updated the pull request.

facebook-github-bot commented Dec 30, 2015

@mwiencek updated the pull request.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 30, 2015

Contributor

This breaks if you're rendering onto pre-rendered markup right? Adjacent text nodes would then be merged and empty ones non-existent.

Contributor

syranide commented Dec 30, 2015

This breaks if you're rendering onto pre-rendered markup right? Adjacent text nodes would then be merged and empty ones non-existent.

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao
Member

zpao commented Dec 30, 2015

cc @spicyj

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Dec 30, 2015

Contributor

@syranide It seems to complain about invalid checksums if you try to render text nodes over something generated with ReactDOMServer.renderToString, if that's what you mean.

Contributor

mwiencek commented Dec 30, 2015

@syranide It seems to complain about invalid checksums if you try to render text nodes over something generated with ReactDOMServer.renderToString, if that's what you mean.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Dec 30, 2015

Contributor

Er, no, I accidentally changed the markup in the test, so I think it was right to complain. e.g. 9238567 works. If you have a small example of what you mean I can try to add another test.

Contributor

mwiencek commented Dec 30, 2015

Er, no, I accidentally changed the markup in the test, so I think it was right to complain. e.g. 9238567 works. If you have a small example of what you mean I can try to add another test.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 30, 2015

@mwiencek updated the pull request.

facebook-github-bot commented Dec 30, 2015

@mwiencek updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 9, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Jan 9, 2016

@mwiencek updated the pull request.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 9, 2016

Contributor

So I think my first approach of trying to merge the text elements before they're instantiated (and then expecting the browser to mount them sanely) was incorrect. Thanks @syranide for pointing me in a different direction.

The new approach is to try and detect when text nodes have been merged/split, and handle things after the fact.

Contributor

mwiencek commented Jan 9, 2016

So I think my first approach of trying to merge the text elements before they're instantiated (and then expecting the browser to mount them sanely) was incorrect. Thanks @syranide for pointing me in a different direction.

The new approach is to try and detect when text nodes have been merged/split, and handle things after the fact.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 9, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Jan 9, 2016

@mwiencek updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 10, 2016

Member

Sorry for not commenting earlier:

I was thinking that the best approach here might be to add delimiting comment tags around each text node so

<div>{'a'}{'b'}</div>

would become

<div>
  <!-- react-text: 2 -->a<!-- /react-text -->
  <!-- react-text: 3 -->b<!-- /react-text -->
</div>

when rendered. When updating we could update everything in between the matching comment tags. This would immediately solve the merging/splitting problem and would also leave us more resilient to browser extensions that change the page, like the popular "cloud to butt" extension or you might imagine an extension that bolds or highlights certain words on a page by adding tags. This seems more robust in my mind. (Technically we wouldn't even need the "closing" comments but I think it's simpler to have them.)

We also shouldn't add any DOM-specific logic to ReactChildReconciler – that's used on React Native too.

What do you think?

Member

sophiebits commented Jan 10, 2016

Sorry for not commenting earlier:

I was thinking that the best approach here might be to add delimiting comment tags around each text node so

<div>{'a'}{'b'}</div>

would become

<div>
  <!-- react-text: 2 -->a<!-- /react-text -->
  <!-- react-text: 3 -->b<!-- /react-text -->
</div>

when rendered. When updating we could update everything in between the matching comment tags. This would immediately solve the merging/splitting problem and would also leave us more resilient to browser extensions that change the page, like the popular "cloud to butt" extension or you might imagine an extension that bolds or highlights certain words on a page by adding tags. This seems more robust in my mind. (Technically we wouldn't even need the "closing" comments but I think it's simpler to have them.)

We also shouldn't add any DOM-specific logic to ReactChildReconciler – that's used on React Native too.

What do you think?

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jan 10, 2016

Contributor

@spicyj It might not be a good idea for complexity/performance reasons, but I would imagine the best solution would be to simply merge adjacent strings (and remove empty strings) from the intermediate representation that gets rendered to the DOM. I.e. the DOM renderer only ever sees individual strings sandwiched between elements, never multiple or empty strings. Minimal markup and is completely safe, but intuitively it might demand some rather non-trivial logic.

As for <!-- react-text: 2 -->a<!-- /react-text --> I would say <div><!-- react-text: 2 -->a<!-- react-text: 3 -->b</div> should be sufficient no? The closing comment would not actually provide any benefit. The only downside to comments is that all elements do come with a "significant cost", so avoiding it if possible would be a good thing, but other than that it seems great to me.

Contributor

syranide commented Jan 10, 2016

@spicyj It might not be a good idea for complexity/performance reasons, but I would imagine the best solution would be to simply merge adjacent strings (and remove empty strings) from the intermediate representation that gets rendered to the DOM. I.e. the DOM renderer only ever sees individual strings sandwiched between elements, never multiple or empty strings. Minimal markup and is completely safe, but intuitively it might demand some rather non-trivial logic.

As for <!-- react-text: 2 -->a<!-- /react-text --> I would say <div><!-- react-text: 2 -->a<!-- react-text: 3 -->b</div> should be sufficient no? The closing comment would not actually provide any benefit. The only downside to comments is that all elements do come with a "significant cost", so avoiding it if possible would be a good thing, but other than that it seems great to me.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Jan 10, 2016

Contributor

Yeah, I know things got better when we removed the react-data ids. Using comments may be no worse than using spans, but it does seem like it would be better to merge the strings if it doesn't make the code substantially more complex.

Contributor

jimfb commented Jan 10, 2016

Yeah, I know things got better when we removed the react-data ids. Using comments may be no worse than using spans, but it does seem like it would be better to merge the strings if it doesn't make the code substantially more complex.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 11, 2016

Contributor

My main motivation here was to remove the chance of surprises from unexpected markup differences while we port a lot of files from TemplateToolkit into React. A lesser motivation was to remove clutter from the browser's elements inspector, so it's easier to see only the things that matter.

I think the comment delimiters solve the first motivation, and partially the second (they at least remove a level of nesting, and are easier to ignore). So definitely sounds fine to me if you guys deem that the safest way forward.

Trying to merge the strings sounds closer to what I attempted at first, but I think I was doing it in the wrong place: at the ReactElement level, before the components were instantiated, where it may not have made sense outside of a browser context.

Contributor

mwiencek commented Jan 11, 2016

My main motivation here was to remove the chance of surprises from unexpected markup differences while we port a lot of files from TemplateToolkit into React. A lesser motivation was to remove clutter from the browser's elements inspector, so it's easier to see only the things that matter.

I think the comment delimiters solve the first motivation, and partially the second (they at least remove a level of nesting, and are easier to ignore). So definitely sounds fine to me if you guys deem that the safest way forward.

Trying to merge the strings sounds closer to what I attempted at first, but I think I was doing it in the wrong place: at the ReactElement level, before the components were instantiated, where it may not have made sense outside of a browser context.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 11, 2016

Member

We can consider merging adjacent text nodes in a separate diff; I don't think that necessarily needs to be attached to this.

@syranide Yes, we don't need the "closing" tags as I mentioned in my comment but I think the behavior is simpler and easier to reason about if we keep them.

@mwiencek Yes, let's do that if you're up for it. I would like to not have the comment tags but I think it's better to be safe here and do something that we can be confident will work in all scenarios. Let me know if you run into any problems.

Member

sophiebits commented Jan 11, 2016

We can consider merging adjacent text nodes in a separate diff; I don't think that necessarily needs to be attached to this.

@syranide Yes, we don't need the "closing" tags as I mentioned in my comment but I think the behavior is simpler and easier to reason about if we keep them.

@mwiencek Yes, let's do that if you're up for it. I would like to not have the comment tags but I think it's better to be safe here and do something that we can be confident will work in all scenarios. Let me know if you run into any problems.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jan 11, 2016

Contributor

@syranide Yes, we don't need the "closing" tags as I mentioned in my comment but I think the behavior is simpler and easier to reason about if we keep them.

@spicyj It seems to me that it's basically if (node.isComment && node.text === '...') (for closing comment) vs if (node == null || !node.isTextNode) (for no closing comment). Unless I'm missing something they seem pretty much the same to me and IMHO I would favor the second for less markup.

Contributor

syranide commented Jan 11, 2016

@syranide Yes, we don't need the "closing" tags as I mentioned in my comment but I think the behavior is simpler and easier to reason about if we keep them.

@spicyj It seems to me that it's basically if (node.isComment && node.text === '...') (for closing comment) vs if (node == null || !node.isTextNode) (for no closing comment). Unless I'm missing something they seem pretty much the same to me and IMHO I would favor the second for less markup.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 11, 2016

Member

I was planning to support replacing tags in case an extension adds them, like if it wanted to bold a particular word.

Member

sophiebits commented Jan 11, 2016

I was planning to support replacing tags in case an extension adds them, like if it wanted to bold a particular word.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jan 11, 2016

Contributor

@spicyj Hmm, an interesting point... previously we could test if a node was managed by React, is that not possible in master anymore? That would suffice if we could. Otherwise yeah that's not a bad idea, and realistically we could shorten the closing comment to <!-- / -->, it can't occur naturally and it's extremely unlikely that it would be inserted by whatever messed with the code.

However, I'm a little worried about <-- open -->foo<!-- close --><-- open -->bar<!-- close -->, if some external tool tried to bold that, wouldn't it break anyway? (Still a good idea though, this is rather less likely scenario)

Contributor

syranide commented Jan 11, 2016

@spicyj Hmm, an interesting point... previously we could test if a node was managed by React, is that not possible in master anymore? That would suffice if we could. Otherwise yeah that's not a bad idea, and realistically we could shorten the closing comment to <!-- / -->, it can't occur naturally and it's extremely unlikely that it would be inserted by whatever messed with the code.

However, I'm a little worried about <-- open -->foo<!-- close --><-- open -->bar<!-- close -->, if some external tool tried to bold that, wouldn't it break anyway? (Still a good idea though, this is rather less likely scenario)

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 12, 2016

Contributor

So after implementing the comment-tag method, I'm getting a test failure in ReactDefaultPerf that I'm not sure how to fix, but I'll push what I have for now.

Contributor

mwiencek commented Jan 12, 2016

So after implementing the comment-tag method, I'm getting a test failure in ReactDefaultPerf that I'm not sure how to fix, but I'll push what I have for now.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 12, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Jan 12, 2016

@mwiencek updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 12, 2016

Member

This looks really good overall so far. Let me know if any of my comments are confusing or if I can help with anything.

Member

sophiebits commented Jan 12, 2016

This looks really good overall so far. Let me know if any of my comments are confusing or if I can help with anything.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 12, 2016

Contributor

@spicyj I really appreciate the feedback. The only thing I have trouble understanding is the ReactDefaultPerf failure https://travis-ci.org/facebook/react/jobs/101760702#L224

Contributor

mwiencek commented Jan 12, 2016

@spicyj I really appreciate the feedback. The only thing I have trouble understanding is the ReactDefaultPerf failure https://travis-ci.org/facebook/react/jobs/101760702#L224

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 12, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Jan 12, 2016

@mwiencek updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 18, 2016

Member

I don't have time this moment to look at the code again, but the perf test is probably failing because replaceDelimitedText is not instrumented (see the bottom of DOMChildrenOperations).

Member

sophiebits commented Jan 18, 2016

I don't have time this moment to look at the code again, but the perf test is probably failing because replaceDelimitedText is not instrumented (see the bottom of DOMChildrenOperations).

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 18, 2016

Contributor

Yup, that was it. Less sure of what I'm doing there but tests pass now.

Contributor

mwiencek commented Jan 18, 2016

Yup, that was it. Less sure of what I'm doing there but tests pass now.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 18, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Jan 18, 2016

@mwiencek updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Feb 17, 2016

Member

Sorry for the long delay. This is really awesome and I want to get this in. Can we look at moving the unmounting logic to DOMChildrenOperations? I think we may also have a problem with reordering text nodes when keyed text nodes are involved. Specifically,

<div>{new Map([['a', 'alpha'], ['b', 'beta']])}</div>

renders a div with two text children: alpha (with key a) and beta (with key b). If we swap the order to b then a, the text nodes should get moved around. (Sorry this is a little subtle.) Can you make sure this works and add a quick test in ReactMultiChildText for it?

Other than that, I think this looks great.

Member

sophiebits commented Feb 17, 2016

Sorry for the long delay. This is really awesome and I want to get this in. Can we look at moving the unmounting logic to DOMChildrenOperations? I think we may also have a problem with reordering text nodes when keyed text nodes are involved. Specifically,

<div>{new Map([['a', 'alpha'], ['b', 'beta']])}</div>

renders a div with two text children: alpha (with key a) and beta (with key b). If we swap the order to b then a, the text nodes should get moved around. (Sorry this is a little subtle.) Can you make sure this works and add a quick test in ReactMultiChildText for it?

Other than that, I think this looks great.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 18, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Feb 18, 2016

@mwiencek updated the pull request.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Feb 18, 2016

Contributor

@spicyj not a problem, thanks for looking again. I fixed the reordering issue and added a suitable test, I hope.

Contributor

mwiencek commented Feb 18, 2016

@spicyj not a problem, thanks for looking again. I fixed the reordering issue and added a suitable test, I hope.

Don't wrap text in <span> elements
Instead, use opening and closing comment nodes to delimit text data.
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 18, 2016

@mwiencek updated the pull request.

facebook-github-bot commented Feb 18, 2016

@mwiencek updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Feb 18, 2016

Member

This looks great. Thank you so much!

Member

sophiebits commented Feb 18, 2016

This looks great. Thank you so much!

sophiebits added a commit that referenced this pull request Feb 18, 2016

Merge pull request #5753 from mwiencek/no-text-span-2
Don't wrap text in <span> elements

@sophiebits sophiebits merged commit 684ef3e into facebook:master Feb 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Feb 18, 2016

Contributor

woot! thank you!

Contributor

mwiencek commented Feb 18, 2016

woot! thank you!

@mwiencek mwiencek deleted the mwiencek:no-text-span-2 branch Feb 18, 2016

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Feb 18, 2016

Contributor

@mwiencek I also wanted to jump in and say thanks! This has been one of our long standing todo items, and no one has tackled it because it was a tough one, but you did a great job! I'm really excited to see this merged! Thanks!

Contributor

jimfb commented Feb 18, 2016

@mwiencek I also wanted to jump in and say thanks! This has been one of our long standing todo items, and no one has tackled it because it was a tough one, but you did a great job! I'm really excited to see this merged! Thanks!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 18, 2016

Member

@mwiencek Amazing work. Thank you so much for contributing!

Member

gaearon commented Feb 18, 2016

@mwiencek Amazing work. Thank you so much for contributing!

sophiebits added a commit to sophiebits/react that referenced this pull request Feb 19, 2016

Fix text component rendering with server markup
These weren't caught by CI in #5753 because we don't automatically test that yet... fixing that next.

sophiebits added a commit to sophiebits/react that referenced this pull request Feb 19, 2016

Fix text component rendering with server markup
These weren't caught by CI in #5753 because we don't automatically test that yet... fixing that next.

alunyov pushed a commit to alunyov/react that referenced this pull request Mar 10, 2016

Fix text component rendering with server markup
These weren't caught by CI in #5753 because we don't automatically test that yet... fixing that next.
@alcedo

This comment has been minimized.

Show comment
Hide comment
@alcedo

alcedo May 19, 2016

quick question: cloud flare is stripping away the <!-- --> tags and rightfully so. what should i do in this case?

alcedo commented May 19, 2016

quick question: cloud flare is stripping away the <!-- --> tags and rightfully so. what should i do in this case?

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb May 19, 2016

Contributor

@alcedo You can try turning off "Auto Minify" via your CloudFlare Settings control panel. Haven't tried it myself, but that sounds like the relevant setting. If that doesn't work, I'd recommend contacting CloudFlare support - I'd be extraordinarily surprised if they didn't provide a way for resources to be served unchanged.

Contributor

jimfb commented May 19, 2016

@alcedo You can try turning off "Auto Minify" via your CloudFlare Settings control panel. Haven't tried it myself, but that sounds like the relevant setting. If that doesn't work, I'd recommend contacting CloudFlare support - I'd be extraordinarily surprised if they didn't provide a way for resources to be served unchanged.

@alcedo

This comment has been minimized.

Show comment
Hide comment
@alcedo

alcedo May 19, 2016

got it. that should work :)

alcedo commented May 19, 2016

got it. that should work :)

@artursgirons

This comment has been minimized.

Show comment
Hide comment
@artursgirons

artursgirons Aug 2, 2016

Could this <!-- foo-->...<!-- /foo --> solution work for implementing "virtual" root element?
One use case for such virtual root element would be for creating wrapper component which could add siblings for wrapped component without interference with main html tag structure.

<!-- foo --> <-- wrapper component ("virtual" component root)
   <style>._c1 {color:red;}</style> <-- sibling to wrapped component added by wrapper
   <div class="_c1">text</div> <-- wrapped component
<!-- /foo -->

Maybe there is already some implementation of such idea?

artursgirons commented Aug 2, 2016

Could this <!-- foo-->...<!-- /foo --> solution work for implementing "virtual" root element?
One use case for such virtual root element would be for creating wrapper component which could add siblings for wrapped component without interference with main html tag structure.

<!-- foo --> <-- wrapper component ("virtual" component root)
   <style>._c1 {color:red;}</style> <-- sibling to wrapped component added by wrapper
   <div class="_c1">text</div> <-- wrapped component
<!-- /foo -->

Maybe there is already some implementation of such idea?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 2, 2016

Member

Please see the discussion in #2127.

Member

gaearon commented Aug 2, 2016

Please see the discussion in #2127.

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment