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

Textareas being formatted by HTMLFormatter #2838

Closed
APB9785 opened this issue Oct 2, 2023 · 3 comments · Fixed by #2840
Closed

Textareas being formatted by HTMLFormatter #2838

APB9785 opened this issue Oct 2, 2023 · 3 comments · Fixed by #2840

Comments

@APB9785
Copy link
Contributor

APB9785 commented Oct 2, 2023

Environment

  • Elixir version (elixir -v): 1.15.5
  • Phoenix version (mix deps): 1.7.7
  • Phoenix LiveView version (mix deps): 0.19.5
  • Operating system: OS X 12.6.6
  • Browsers you attempted to reproduce this bug on (the more the merrier): Firefox
  • Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

HTMLFormatter is adding a newline between <textarea> tags, which adds unexpected whitespace to the rendered output

iex(1)> Phoenix.LiveView.HTMLFormatter.format("<textarea class=\"test\"></textarea>", heex_line_length: 10)
"<textarea class=\"test\">\n</textarea>\n"

The tag phx-no-format has no effect. The newline is still added even when that attr is included.

Expected behavior

iex(1)> Phoenix.LiveView.HTMLFormatter.format("<textarea class=\"test\"></textarea>", heex_line_length: 10)
"<textarea class=\"test\"></textarea>\n"
@leandrocp
Copy link
Contributor

leandrocp commented Oct 2, 2023

For extra context, this assertion fails (just added it for testing):

      assert_formatter_doesnt_change(
        """
        <textarea></textarea>
        """,
        line_length: 5
      )

But seems like it shouldn't. And the root cause is the check buffer != [] here

if (tag_name in ["pre", "textarea"] or contains_special_attrs?(attrs)) and buffer != [] do
Idk why only nom-empty elements are preserved, but all tests pass removing that check, including the assert above.

@josevalim
Copy link
Member

@leandrocp please send a PR with tests. I think your changes are good.

@leandrocp
Copy link
Contributor

@josevalim thanks. @APB9785 will push a PR soon.

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 a pull request may close this issue.

3 participants