SL-528: Parse start_html using ::HTML to preserve expanded tags and fix App Lab rendering

[ebeastlake]

The following PR fixes several instances in which folks are seeing App Lab levels with jumbled/misplaced UI elements on sample applications (see screenshots below). The issue is that the call to Nokogiri::XML turns expanded tags in our HTML (ex. <button ...></button>) to self-enclosing tags (ex. <button ... />), and for some reason, this "breaks" the element rendering.

It turns out the config option that fixes this issue is already in place at another point in our code. Still, I did not discover that the Nokogiri::XML::Node::SaveOptions::NO_EMPTY_TAGS config value was used elsewhere until I finished my investigation.

I diagnosed by looking at the diff in the HTML at every step in the localized_start_html. The start_html that serves as the argument to this function renders correctly, but as soon as we call start_html_xml = Nokogiri::XML(start_html, &:noblanks), the start_html_xml reproduces the bug in an HTML renderer. Since Nokogiri can perform its operations on the self-enclosing tags, we don't need to expand the tags until before we return them from localized_start_html. This is good because the config option we need to pass to expand all HTML tags is a valid argument to start_html_xml.serialize but cannot be passed to Nokogiri::XML to keep them from getting collapsed in the first place.

NOTE: This config option will expand all tags, including any that were self-closing in our start_html. Theoretically, expanding tags meant to be self-enclosing could also cause issues, but expanding all tags seems to solve most of our problems (see screenshots below).

For posterity/documentation, sparklemotion/nokogiri#2324 describes a similar issue, but in the opposite direction (self-closing tags being converted to expanded tags), and is the issue that eventually led me to my fix.

I do not know why using self-enclosing tags instead of expanded tags would cause the issue, but given the volume of tickets we're receiving, it seems worth getting a fix out while we continue the investigation.

Links

Jira tickets:
https://codedotorg.atlassian.net/browse/SL-528
https://codedotorg.atlassian.net/browse/P20-43

Zendesk tickets:
https://codeorg.zendesk.com/agent/tickets/427471
https://codeorg.zendesk.com/agent/tickets/424893
https://codeorg.zendesk.com/agent/tickets/430408

Testing story

Before
Note: These screenshots were generated by resetting version history while the page is in Danish on studio.code.org. I have reproduced this with other languages, including Spanish and French.

https://studio.code.org/s/csp5-2022/lessons/2/levels/1

https://studio.code.org/s/csp5-2022/lessons/10/levels/1

https://studio.code.org/s/csp7-2022/lessons/4/levels/2

https://studio.code.org/s/csp7-2022/lessons/3/levels/6

After
Note: These screenshots were generated by resetting version history while the page is in Danish on localhost-studio.code.org:3000. I have reproduced this with other languages, including Spanish and French.

http://localhost-studio.code.org:3000/s/csp5-2022/lessons/2/levels/1

http://localhost-studio.code.org:3000/s/csp5-2022/lessons/10/levels/1

http://localhost-studio.code.org:3000/s/csp7-2022/lessons/4/levels/2

http://localhost-studio.code.org:3000/s/csp7-2022/lessons/3/levels/6

The screenshot above is not totally fixed because of another bug with how Nokogiri handles spaces, but it is much better than the original! I will file a follow-up ticket and link here.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[breville]

Can we add a unit test to this PR? It seems the ideal situation for one.

[ebeastlake]

Yes, definitely!

[breville]

Could we reduce this to a more minimal repro case? It might be a bit easier to understand what we are verifying...

[breville]

nit: can we put the long URL on a new line?

[mgc1194]

We should use Nokogiri::HTML.fragment(start_html, &:noblanks) instead of Nokogiri::XML.fragment(start_html, &:noblanks).
The start_html is a well-formed html fragment that gets injected as into the level document.
It doesn't make sense to use XML here, as it is inducing undesired changes in the html tags (not only creating self-enclosing tags, but closing tags).

[ebeastlake]

Yep, going to change based on our conversation. I don't think we need the argument to serialize anymore in this case, but I will confirm.

[mgc1194]

Exactly.
start_html_doc.to_html(encoding: 'UTF-8') should work. We need the UTF-8 encoding as this localized html will contain characters in any language.

[mgc1194]

Awesome! This switch to HTML parsing fixes multiple issues. Thanks!

[ebeastlake]

Note: The screenshots in this PR were tested with the original fix, which was replaced with an alternate approach suggested by @mgc1194 (see aa37b1a). Most of the screenshots are fixed in the same way, but Mario's approach actually leaves the Order Up! app in an even better state (see below).