Skip to content
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

Properly handle HTML fragments with self closing tags #674

Closed

Conversation

arambert
Copy link

@arambert arambert commented Nov 8, 2023

Type of PR (feature, enhancement, bug fix, etc.)

Bug Fix

Description

Properly handle HTML fragments including self closing tags like img, br, input that Nokogiri XML parser would normally
try to fix by adding a closing tag. For example, <div><img src='x'><span>test</span></div> would be changed internally to <div><img src='x'><span>test</span></img></div> by Nokogiri and then converted to <div><img src='x'></div> by Nokogiri through the to_html method.

This is due to the use of Nokogiri parser for the selector morphs (as opposed to Nokogiri::HTML5::Document parser), but we cannot use the Nokogiri::HTML5::Document when handling incomplete HTML documents (see 69cb070 for more info)

One could say that we should produce valid html with / at the end of self-closing tags (ie: <br/> instead of <br>) but Nokogiri itself strips the trailling / when producing html string from XML nodes...

This PR detects these instances and handle them gracefully.

Fixes #652 (issue)

Why should this be added

The main issue for the user before this PR is that html tags are sometimes removed from the reflex response because of this behaviour.

This PR allows Stimulus Reflex users to apply selector morphs with HTML containing self-closing tags with siblings.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

Copy link

netlify bot commented Nov 8, 2023

Deploy Preview for stimulusreflex ready!

Name Link
🔨 Latest commit 7c6d90a
🔍 Latest deploy log https://app.netlify.com/sites/stimulusreflex/deploys/654ba8a2aa310d000804a25a
😎 Deploy Preview https://deploy-preview-674--stimulusreflex.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@marcoroth
Copy link
Member

Hey @arambert, thanks for opening this pull request!

We've been experimenting on how to properly handle the issue you outlined here. I have some pending bugs I want to report to Nokogiri which should make most of the workarounds unnecessary.

One of them is here already: sparklemotion/nokogiri#3023

Before merging this, please allow me make sure we can make this work in upstream Nokogiri, so we don't need to work around this in StimulusReflex.

@arambert
Copy link
Author

arambert commented Nov 27, 2023

Hi @marcoroth,

Thank you for your review!

I agree with you on the fact that this issue is originally a Nokogiri issue. Of course ideally it would be fixed on Nokogiri's side but in the meantime it's also a StimulusReflex issue ;)
For my use case, I've monkeypatched StimulusReflex on our app and it works fine, so I'm not in a hurry.

Thanks again!

@marcoroth
Copy link
Member

@arambert just wanted to tag you and mention that #696 should be solving this issue! If you get a chance, I'd love to hear how it works for you! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document fragment now do not handle 2 siblings inputs
2 participants