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

Remove wrapping div with inline styles for hidden form fields. #14738

Merged
merged 1 commit into from Apr 17, 2014

Conversation

@tilsammans
Copy link
Contributor

tilsammans commented Apr 13, 2014

We are dropping HTML 4.01 and XHTML strict compliance since input tags directly inside a form are valid HTML5, and the absense of inline styles help in validating for Content Security Policy.

work around Firefox 12 not submitting invisible inputs. With HTML5 as the default doctype
and Firefox 12 market share at 0.21% the time has come for these to go.

Fixes #11407.

This comment has been minimized.

Copy link
@jeremy

jeremy Apr 14, 2014

Member

Omit.


The div was added for XHTML 1.0 Strict compliance and the inline styles were added to
work around Firefox 12 not submitting invisible inputs. With HTML5 as the default doctype
and Firefox 12 market share at 0.21% the time has come for these to go.

This comment has been minimized.

Copy link
@jeremy

jeremy Apr 14, 2014

Member

I called out Firefox 12 as an example, but it wasn't the only one, so staking a case on its 0.21% is flimsy. Clearer to say that we're dropping HTML 4.01 and XHTML strict compliance since it's fine in HTML5.

tags = (enforce_utf8 ? utf8_enforcer_tag : ''.html_safe) << method_tag
content_tag(:div, tags, :style => 'display:none')
if html_options.delete("enforce_utf8") { true }
utf8_enforcer_tag << method_tag

This comment has been minimized.

Copy link
@jeremy

jeremy Apr 14, 2014

Member
utf8_enforcer_tag + method_tag
if html_options.delete("enforce_utf8") { true }
utf8_enforcer_tag << method_tag
else
''.html_safe << method_tag

This comment has been minimized.

Copy link
@jeremy

jeremy Apr 14, 2014

Member

No need to concat the method tag onto an empty string. This is sufficient:

method_tag
txt << %{<input name="utf8" type="hidden" value="&#x2713;" />}
if method && !%w(get post).include?(method.to_s)
txt << %{<input name="_method" type="hidden" value="#{method}" />}
''.tap do |txt|

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 14, 2014

Member

Could you avoid to use tap? I believe the previous code is clear.

This comment has been minimized.

Copy link
@tilsammans

tilsammans Apr 14, 2014

Author Contributor

Also remove tap in the other tests? Especially the latest one looks messy to me, without it:

def hidden_fields(options = {})
  method = options[:method]
  enforce_utf8 = options.fetch(:enforce_utf8, true)

  txt = ''

  if enforce_utf8
    txt << %{<input name="utf8" type="hidden" value="&#x2713;" />}
  end

  if method && !%w(get post).include?(method.to_s)
    txt << %{<input name="_method" type="hidden" value="#{method}" />}
  end

  txt
end

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 14, 2014

Member

The last one is fine to me. Just these two that doesn't need to initialize with empty string.

We are dropping HTML 4.01 and XHTML strict compliance since input
tags directly inside a form are valid HTML5, and the absense of
inline styles help in validating for Content Security Policy.
@tilsammans
Copy link
Contributor Author

tilsammans commented Apr 14, 2014

Pushed.

@tilsammans
Copy link
Contributor Author

tilsammans commented Apr 17, 2014

@jeremy @rafaelfranca I made the changes you suggested. This is good to go?

@jeremy
Copy link
Member

jeremy commented Apr 17, 2014

Looks good. Needs a rebase against latest master, then good to merge.

@rafaelfranca
Copy link
Member

rafaelfranca commented Apr 17, 2014

I already merged manually. Thanks you @tilsammans

@rafaelfranca rafaelfranca merged commit 89ff1f8 into rails:master Apr 17, 2014
rafaelfranca added a commit that referenced this pull request Apr 17, 2014
Remove wrapping div with inline styles for hidden form fields.

Conflicts:
	actionview/CHANGELOG.md
@tilsammans tilsammans deleted the tilsammans:pull/11407 branch Apr 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.