Use script tags to demarcate text components

[sophiebits]

tl;dr: <script></script>A<script></script>B instead of <span>A</span><span>B</span>.

Old:

New:

This is nicer in that if you add styles on span, your text nodes now won't be affected. Similarly, the target of mouse events will now never be a ReactTextComponent span because the script elements aren't rendered at all.

Test Plan: jest, and some simple browser sanity checking.

[sophiebits]

Thoughts? Haven't benchmarked or tested in IE8 yet.

[syranide]

I would think that we could just insert comments instead (but perhaps that messes with React)? But why not solve it properly? It can't be that hard to just merge adjacent strings just before rendering to the DOM, or can it?

PS. It might be worth using <noscript> instead as I assume it would have less overhead (and also less confusion if something tries to grab all scripts on the page, it's also what we use for null so it makes sense). To me this seems like an interesting short-term solution that I would be OK with (if merging adjacent strings is out of the picture for now).

[syranide]

Btw, the image up there has an inconsistency, b disappeared in your final version. Bug or just a change in the source code? (I'm guessing the latter)

Btw 2, due to issues with non-visible tags in IE8, this will cause the workaround to trigger a lot more often, which will degrade performance somewhat (but it's not horribly slow and unlikely to significantly affect real-life use).

Also, 👍

[syranide]

Hmm, it should be quite easy to use comments instead:

<!-- .0.3.0 -->
If your BAC is
<!-- .0.3.1 -->
then test...

Which is much less hacky and not unreasonable for a framework to do. We don't need any special safeguards as users cannot create comments in React so it's safe "by default".

[sophiebits]

b tag was just a change in the source when testing, not a bug.

[syranide]

I just realized that we sadly can't go this way at all. We still rely on _mountIndex and some browsers do arbitrarily split TextNodes which would obviously offset child indicies.

However, I'm pretty sure we could start using the data-tag instead, it does apply to us according to my interpretation of mozillas interpretation, and would have the end-user same effect as this. Although, it is styleable too (but considerably less likely to be styled by the user).

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/data There they use it to associate text with UPC codes as their definition of "machine readable", i.e. it is context sensitive and there is no implied context, so it should be totally fine to use with reactids.

[sophiebits]

That's why I use .children instead of .childNodes. I believe this works fine.

[syranide]

Aaah, I thought about that but it seemed like it wouldn't work, but you're absolute right it does. Awesome.

[sebmarkbage]

Why is this better?

[syranide]

@sebmarkbage No spans that can be styled. So for all intents and purposes these are "invisible" as opposed to the current spans.

[sophiebits]

Yup – script tags don't get styled and you won't ever get them as the target of a mouse event. I think it's a minor improvement but am willing to drop it in favor of fixing this properly later.

[sophiebits]

I'll close this out but we can bring it back if we later decide we want it in.

[opichals]

@spicyj Could this be taken as a good enough interim solution?

It's been talked about and rejected in favor of 'fixing this properly' several times (e.g. in #1989). I mean there seem to be real styling issues like the flexbox mentioned in #1236 which could hopefully be solved without further due by merging this approach.

Just to add some more info on this, the script tag approach was battle proven in the previous Ember.JS renderer (Up until v1.8).

[sophiebits]

I'll think on it.

[sebmarkbage]

For context, this is a breaking change that can affect a lot of existing code (including all of Facebook). Meaning it is a lot of work for us and others when they upgrade to make sure that styles or rogue query selectors still work.

That's why it's not so easy to just take it as interim solution. It still incurs a lot of work. That's why we figured that we'd only do that work once, with a proper fix.

[opichals]

@sebmarkbage I understand completely.

So perhaps my next question would be how far are we away from the 'proper fix' and if there is a place one could help. Is the solution related to #1570 (comment) and has the merge of #5205 moved us closer in the context of this issue?