Don't wrap text in <span> elements #5753
Conversation
|
Worth mentioning; https://bugzilla.mozilla.org/show_bug.cgi?id=194231, |
|
|
|
@mwiencek updated the pull request. |
|
This breaks if you're rendering onto pre-rendered markup right? Adjacent text nodes would then be merged and empty ones non-existent. |
|
cc @spicyj |
|
@syranide It seems to complain about invalid checksums if you try to render text nodes over something generated with |
|
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. |
|
@mwiencek updated the pull request. |
9238567
to
8fe256a
Compare
|
@mwiencek updated the pull request. |
|
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. |
8fe256a
to
3868c25
Compare
|
@mwiencek updated the pull request. |
|
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? |
|
@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 |
|
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. |
|
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. |
|
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. |
@spicyj It seems to me that it's basically |
|
I was planning to support replacing tags in case an extension adds them, like if it wanted to bold a particular word. |
|
@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 However, I'm a little worried about |
|
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. |
3868c25
to
35b5dbe
Compare
| if (isClosingTextComment(nodeAfterComment)) { | ||
| // There are no text nodes between the opening and closing comments; insert | ||
| // a new one if stringText isn't empty. | ||
| if (stringText) { |
There was a problem hiding this comment.
I made it so that if it's an empty string it doesn't insert an empty text node between the comments. Not really sure which is preferable.
There was a problem hiding this comment.
Having no text node seems reasonable.
|
@mwiencek updated the pull request. |
|
This looks really good overall so far. Let me know if any of my comments are confusing or if I can help with anything. |
|
@spicyj not a problem, thanks for looking again. I fixed the reordering issue and added a suitable test, I hope. |
| expect(childNodes[3]).toBe(alpha1); | ||
| expect(childNodes[4]).toBe(alpha2); | ||
| expect(childNodes[5]).toBe(alpha3); | ||
| }); |
There was a problem hiding this comment.
Can you add a
// Using Maps as children gives a single warning
expect(console.error.calls.length).toBe(1);
to make sure new warnings don't pop up unexpectedly?
Instead, use opening and closing comment nodes to delimit text data.
3502289
to
2038500
Compare
|
@mwiencek updated the pull request. |
|
This looks great. Thank you so much! |
Don't wrap text in <span> elements
|
woot! thank you! |
|
@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! |
|
@mwiencek Amazing work. Thank you so much for contributing! |
These weren't caught by CI in facebook#5753 because we don't automatically test that yet... fixing that next.
These weren't caught by CI in facebook#5753 because we don't automatically test that yet... fixing that next.
|
quick question: cloud flare is stripping away the |
|
@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. |
|
got it. that should work :) |
|
Could this <!-- 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? |
|
Please see the discussion in #2127. |
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.