Skip to content
This repository

Textarea newline fix breaks haml (3-2-stable) #5619

Merged
merged 1 commit into from over 2 years ago

7 participants

James Coleman Rafael Mendonça França Jan M. Faber Andrew Hubbs Obie Fernandez Philip Arndt Santiago Pastorino
James Coleman

See issue #393, issue #4000, issue #5190, and issue #5191. Adds a newline after the textarea opening tag based on @codykrieger's original patch so that we don't cause regressions in Haml-using apps. The regression caused textarea tags to add newlines to the field unintentionally (each update/save added an extra newline.)

James Coleman

This adds a hash lookup to each tag generated (which some have objected to) but I've run some benchmarks:

require 'benchmark'

h = {:a => 1}

Benchmark.bm(10) do |x|
  x.report('empty loop'){ 10000000.times { nil  } }
  x.report('hash lookup'){ 10000000.times { h[:a]  } }
end

And found that Ruby (on my not so fast MacBook) can do 10 million hash lookups in under half a second. So I don't think this causes any performance problems.

Rafael Mendonça França
Owner

Hey. As you do not did the cherry pick, could we add a simple if clause instead of the hash lookup?

E.g:

def content_tag_string(name, content, options, escape = true)
  tag_options = tag_options(options, escape) if options
  pre_content_string = name == "textarea" ? '\n' : nil
  "<#{name}#{tag_options}>#{pre_content_strings}#{escape ? ERB::Util.h(content) : content}</#{name}>".html_safe
end
James Coleman

I made that change (and squashed the commits.)

That seems like more of a hack, but it should allay performance concerns I suppose.

Rafael Mendonça França
Owner

Hey @jcoleman sorry to make you change your original commit. I did one benchmark and the ternary operator is 50% slower than the hash lookup. You can revert your change to the original one.

Really really sorry.

Here the benchmark:

require 'benchmark'

h = { :a => '\n' }

Benchmark.bm(10) do |x|
  x.report('if')   { 10_000_000.times { :a == :a ? '\n' : nil } }
  x.report('hash') { 10_000_000.times { h[:a] } }
end
                 user     system      total        real
if           2.060000   0.010000   2.070000 (  2.070319)
hash         1.100000   0.000000   1.100000 (  1.106086)

                 user     system      total        real
if           2.110000   0.000000   2.110000 (  2.115755)
hash         1.040000   0.000000   1.040000 (  1.047637)

                 user     system      total        real
if           2.770000   0.000000   2.770000 (  2.783767)
hash         1.020000   0.010000   1.030000 (  1.017420)
James Coleman jcoleman Don't break Haml with textarea newline fix.
See issue #393, issue #4000, issue #5190, and issue #5191. Adds a newline after the textarea opening tag based on @codykrieger's original patch so that we don't cause regressions in Haml-using apps. The regression caused textarea tags to add newlines to the field unintentionally (each update/save added an extra newline.)

Also fix 6 more tests that didn't yet have the newline expectation.
a7a422e
James Coleman

All right, I've gone back to the initial hash lookup. As an improvement to @codykrieger's original patch, I've converted this to use symbols rather than strings. This should optimize it further, as well as ensuring (with the .to_sym call I make) that you can correctly use the helpers whether you pass :textarea as the tests do or you pass "textarea" as many people do.

I also noticed that there were 6 tests which didn't yet have the newline expectation for textarea tags, so I fixed them.

Incidentally, that actually represents a bug in the initial fix as opposed to the current hash-lookup fix. @Fjan: the original fix means that content_tag is broken if you use it directly as opposed to calling the textarea tag helper.

Rafael Mendonça França
Owner

Ok. I think that we can merge it.

@spastorino this need to be backported to 3-2-3

Santiago Pastorino spastorino merged commit d82e4c8 into from
Jan M. Faber
Fjan commented

@jcoleman OK, I agree that called the content_tag directly having a different behavior than calling the text_area tag is a bit odd.

The only thing that worries me about this patch is that it calls to_sym on everything passed in as a tag, potentially polluting the symbol table a bit, but the number of tags should typically be limited. Anyway, thanks for the hard work.

James Coleman

The symbol table pollution worried me at first too, but since calling it with the symbol seems to be the expected use (based on the tests) and the fact that symbol lookup/usage will be faster (not to mention to unnecessarily creating string object) it seemed like the better way to go.

Of course someone could intentionally call this in such a way that caused the symbol table to grow infinitely, but that would be intentionally malicious code...so that's not really at stake here.

Philip Arndt

Possible regression in resolve/refinerycms-blog#216 ?

Commented on refinery cms issue.

Andrew Hubbs

I am still seeing this problem using rails 3.2.3 and HAML 3.1.4. Has this issue reappeared or do I have a different problem?

Rafael Mendonça França
Owner

Now the issue is with HAML

Andrew Hubbs

Thanks. I will inquire there.

James Coleman

@andrewhubbs See https://github.com/nex3/haml/issues/497 for the issue, and a fix on my branch. There isn't an official version yet that fixes it (due to an impending maintainer transition.)

Obie Fernandez
obie commented

The new location of that HAML issue is haml/haml#497

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Mar 27, 2012
James Coleman jcoleman Don't break Haml with textarea newline fix.
See issue #393, issue #4000, issue #5190, and issue #5191. Adds a newline after the textarea opening tag based on @codykrieger's original patch so that we don't cause regressions in Haml-using apps. The regression caused textarea tags to add newlines to the field unintentionally (each update/save added an extra newline.)

Also fix 6 more tests that didn't yet have the newline expectation.
a7a422e
This page is out of date. Refresh to see the latest.
2  actionpack/lib/action_view/helpers/form_helper.rb
@@ -1072,7 +1072,7 @@ def to_text_area_tag(options = {})
1072 1072 options["cols"], options["rows"] = size.split("x") if size.respond_to?(:split)
1073 1073 end
1074 1074
1075   - content_tag("textarea", "\n#{options.delete('value') || value_before_type_cast(object)}", options)
  1075 + content_tag("textarea", options.delete('value') || value_before_type_cast(object), options)
1076 1076 end
1077 1077
1078 1078 def to_check_box_tag(options = {}, checked_value = "1", unchecked_value = "0")
6 actionpack/lib/action_view/helpers/tag_helper.rb
@@ -17,6 +17,10 @@ module TagHelper
17 17 autofocus novalidate formnovalidate open pubdate).to_set
18 18 BOOLEAN_ATTRIBUTES.merge(BOOLEAN_ATTRIBUTES.map {|attribute| attribute.to_sym })
19 19
  20 + PRE_CONTENT_STRINGS = {
  21 + :textarea => "\n"
  22 + }
  23 +
20 24 # Returns an empty HTML tag of type +name+ which by default is XHTML
21 25 # compliant. Set +open+ to true to create an open tag compatible
22 26 # with HTML 4.0 and below. Add HTML attributes by passing an attributes
@@ -125,7 +129,7 @@ def escape_once(html)
125 129
126 130 def content_tag_string(name, content, options, escape = true)
127 131 tag_options = tag_options(options, escape) if options
128   - "<#{name}#{tag_options}>#{escape ? ERB::Util.h(content) : content}</#{name}>".html_safe
  132 + "<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name.to_sym]}#{escape ? ERB::Util.h(content) : content}</#{name}>".html_safe
129 133 end
130 134
131 135 def tag_options(options, escape = true)
12 actionpack/test/template/form_tag_helper_test.rb
@@ -216,19 +216,19 @@ def test_select_tag_with_prompt_and_include_blank
216 216
217 217 def test_text_area_tag_size_string
218 218 actual = text_area_tag "body", "hello world", "size" => "20x40"
219   - expected = %(<textarea cols="20" id="body" name="body" rows="40">hello world</textarea>)
  219 + expected = %(<textarea cols="20" id="body" name="body" rows="40">\nhello world</textarea>)
220 220 assert_dom_equal expected, actual
221 221 end
222 222
223 223 def test_text_area_tag_size_symbol
224 224 actual = text_area_tag "body", "hello world", :size => "20x40"
225   - expected = %(<textarea cols="20" id="body" name="body" rows="40">hello world</textarea>)
  225 + expected = %(<textarea cols="20" id="body" name="body" rows="40">\nhello world</textarea>)
226 226 assert_dom_equal expected, actual
227 227 end
228 228
229 229 def test_text_area_tag_should_disregard_size_if_its_given_as_an_integer
230 230 actual = text_area_tag "body", "hello world", :size => 20
231   - expected = %(<textarea id="body" name="body">hello world</textarea>)
  231 + expected = %(<textarea id="body" name="body">\nhello world</textarea>)
232 232 assert_dom_equal expected, actual
233 233 end
234 234
@@ -239,19 +239,19 @@ def test_text_area_tag_id_sanitized
239 239
240 240 def test_text_area_tag_escape_content
241 241 actual = text_area_tag "body", "<b>hello world</b>", :size => "20x40"
242   - expected = %(<textarea cols="20" id="body" name="body" rows="40">&lt;b&gt;hello world&lt;/b&gt;</textarea>)
  242 + expected = %(<textarea cols="20" id="body" name="body" rows="40">\n&lt;b&gt;hello world&lt;/b&gt;</textarea>)
243 243 assert_dom_equal expected, actual
244 244 end
245 245
246 246 def test_text_area_tag_unescaped_content
247 247 actual = text_area_tag "body", "<b>hello world</b>", :size => "20x40", :escape => false
248   - expected = %(<textarea cols="20" id="body" name="body" rows="40"><b>hello world</b></textarea>)
  248 + expected = %(<textarea cols="20" id="body" name="body" rows="40">\n<b>hello world</b></textarea>)
249 249 assert_dom_equal expected, actual
250 250 end
251 251
252 252 def test_text_area_tag_unescaped_nil_content
253 253 actual = text_area_tag "body", nil, :escape => false
254   - expected = %(<textarea id="body" name="body"></textarea>)
  254 + expected = %(<textarea id="body" name="body">\n</textarea>)
255 255 assert_dom_equal expected, actual
256 256 end
257 257

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.