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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed the single space character for Safari #14883

Merged
merged 1 commit into from Jul 10, 2014

Conversation

@chancancode
Copy link
Member

chancancode commented Apr 27, 2014

I'm still cleaning up the tests, but I want to get this out so people can help me test it

Once upon a time, a single space character was added to render nothing: true (cb0f8fd, 807df4f) to fix a bug in Safari. (Thank you @matthewd for hunting this down.)

Since this bug has been fixed in Safari itself a long time ago (based on the timestamps it should be fixed in Safari 2.x or perhaps 1.x in ~2005), it's probably a good time to remove this.

I wrote a simplified version of the webkit test case which you can access here:

(The controller doesn't actually use render nothing, but it should have the same effect)

I tested this on a variety of browsers on [browser stack(http://www.browserstack.com/)], including Safari 4.0 and Firefox 3.0 and it seems to work fine.

Ideally, I'd like to see this test case fail on an older version of safari (you might need safari 2.0 or even 1.0). If you have access to one of those super old browser, please help run the test case and let me know what happens 馃槃

Otherwise, I am fairly confident that this should be okay.

cc @dhh @jeremy @sikachu @matthewd

Fixes #14336

@dhh
Copy link
Member

dhh commented Apr 27, 2014

Thanks for tackling this. Great to rid ourselves of cruft no longer needed.

On Apr 27, 2014, at 8:43 PM, Godfrey Chan notifications@github.com wrote:

I'm still cleaning up the tests, but I want to get this out so people can help me test it

Once upon a time, a single space character was added to render nothing: true (cb0f8fd, 807df4f) to fix a bug in Safari. (Thank you @matthewd for hunting this down.)

Since this bug has been fixed in Safari itself a long time ago (based on the timestamps it should be fixed in Safari 2.x or perhaps 1.x in ~2005), it's probably a good time to remove this.

I wrote a simplified version of the webkit test case which you can access here:

鈥 xhr async version: http://lit-anchorage-8910.herokuapp.com/async (source)
鈥 xhr sync version: http://lit-anchorage-8910.herokuapp.com/sync (source)
(The controller doesn't actually use render nothing, but it should have the same effect)

I tested this on a variety of browsers on [browser stack(http://www.browserstack.com/)], including Safari 4.0 and Firefox 3.0 and it seems to work fine.

Ideally, I'd like to see this test case fail on an older version of safari (you might need safari 2.0 or even 1.0). If you have access to one of those super old browser, please help run the test case and let me know what happens

Otherwise, I am fairly confident that this should be okay.

cc @dhh @jeremy @sikachu @matthewd

Fixes #14336

You can merge this Pull Request by running

git pull https://github.com/chancancode/rails rm-single-space
Or view, comment on, or merge it at:

#14883

Commit Summary

鈥 Removed the single space character for Safari
File Changes

鈥 M actionpack/lib/action_controller/metal/rendering.rb (2)
鈥 M actionpack/test/controller/new_base/render_body_test.rb (14)
鈥 M actionpack/test/controller/new_base/render_html_test.rb (14)
鈥 M actionpack/test/controller/new_base/render_plain_test.rb (14)
鈥 M actionpack/test/controller/new_base/render_text_test.rb (14)
Patch Links:

https://github.com/rails/rails/pull/14883.patch
https://github.com/rails/rails/pull/14883.diff

Reply to this email directly or view it on GitHub.

@jeremy
Copy link
Member

jeremy commented Apr 27, 2014

馃憤

Should update these tests rather than removing them. We expect these to return an empty (unpadded) body.

@arthurnn
Copy link
Member

arthurnn commented Apr 27, 2014

should I close #14350 then? or should we go ahead with both?

@chancancode
Copy link
Member Author

chancancode commented Apr 27, 2014

I haven't dug too deep into the fix itself, I'll take a look at #14350 and pull it whatever we need 馃憤

Just want to get the test case up and have other people check it!

Sent from Mailbox for iPhone

On Sun, Apr 27, 2014 at 2:53 PM, Arthur Nogueira Neves
notifications@github.com wrote:

should I close #14350 then? or should we go ahead with both?

Reply to this email directly or view it on GitHub:
#14883 (comment)

@kaspth
Copy link
Member

kaspth commented Apr 28, 2014

I've been trying to judge how likely a Mac user is to still be using Safari 3.0 and 2.0.

The data:
OS X usage numbers from March 2014
Safari versions matched with their supporting OS'es

So...
Safari 2.0 was only on Tiger which has a market share of 1%.
Safari 3.0 was on Leopard which has a market share of 3%.
Safari 4.1.3 was the last version to support at least Tiger and was released in late 2010.

Also...
How many of those 1% and 3% even use Safari?
How likely is it these old browsers hit a Rails app, which uses renders nothing: true?

馃帀 I think we can declare that bug fixed. 馃帀

@chancancode
Copy link
Member Author

chancancode commented Apr 28, 2014

I agree that we don't really have to worry about safari < 5.

My motivation for testing this on a broken version of safari is to verify the correctness of the test cases and make sure it's testing against the right bug.

Now that I have a test case that passes everywhere it could either be that the bug is fixed everywhere, or the test case is completely wrong :)

I am betting on the former, but it would be nice to know for sure

`render nothing: true` or rendering a `nil` body no longer add a single
space to the response body.

The old behavior was added as a workaround for a bug in an early version of
Safari, where the HTTP headers are not returned correctly if the response
body has a 0-length. This is been fixed since and the workaround is no
longer necessary.

Use `render body: ' '` if the old behavior is desired.
@chancancode chancancode changed the title [WIP] Removed the single space character for Safari Removed the single space character for Safari Jul 10, 2014
@chancancode chancancode added this to the 4.2.0 milestone Jul 10, 2014
@chancancode chancancode self-assigned this Jul 10, 2014
guilleiguaran added a commit that referenced this pull request Jul 10, 2014
Removed the single space character for Safari
@guilleiguaran guilleiguaran merged commit 0d676d1 into rails:master Jul 10, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@egilburg

This comment has been minimized.

Copy link
Contributor

egilburg commented on 013c74d Jul 11, 2014

render nothing: true, layout: 'layouts/something' used to render the layout without template content, but now it renders nothing at all. Now we need to do render 'layouts/something', layout: false to get same behavior back, but it seems hacky as it treats the layout as a template.

Is this change intended?

This comment has been minimized.

Copy link
Member Author

chancancode replied Jul 11, 2014

@egilburg hm, that's definitely not intended. the scope of this change is just to remove the legacy single space padding. Do you want to submit a patch with test cases to cover the regression? 馃槃

This comment has been minimized.

Copy link
Member Author

chancancode replied Jul 11, 2014

@egilburg I reverted the commit on my machine and I couldn't reproduce the behaviour you mentioned. This new test cases passes (without this commit):

diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
index ab7b961..2caa1c4 100644
--- a/actionview/test/actionpack/controller/render_test.rb
+++ b/actionview/test/actionpack/controller/render_test.rb
@@ -363,6 +363,10 @@ class TestController < ApplicationController
     render :nothing => true
   end

+  def rendering_nothing_with_layout
+    render :nothing => true, :layout => "standard"
+  end
+
   def render_to_string_with_assigns
     @before = "i'm before the render"
     render_to_string :text => "foo"
@@ -1030,6 +1034,11 @@ class RenderTest < ActionController::TestCase
     assert_equal " ", @response.body
   end

+  def test_rendering_nothing_with_layout
+    get :rendering_nothing_with_layout
+    assert_equal " ", @response.body
+  end
+
   def test_render_to_string_doesnt_break_assigns
     get :render_to_string_with_assigns
     assert_equal "i'm before the render", assigns(:before)

Do you have a link to another test case or documentation about the behaviour you mentioned?

This comment has been minimized.

Copy link
Contributor

dmitry replied Jul 11, 2014

Is there are any statistics, how many safari browsers still have an issue (in the world) and can be broken by this commit?

This comment has been minimized.

Copy link
Member Author

chancancode replied Jul 11, 2014

So small that no one actually keeps track of it anymore :)

Our best guess is that this is either fixed in Safari 1.x or in 2.0. I asked for someone to let me try reproducing the bug if they still own one of those computers, but it appears that no one have access to them anymore. See #14883

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Dec 30, 2014
- The single space response was added due to a bug in safari
  in rails@cb0f8fd
  and
  rails@807df4f.
- This was removed from the `render nothing: true` in
  rails#14883.
- Removing it from response of :head also. As :head is more obvious
  alternative to call `render nothing:
  true`(http://guides.rubyonrails.org/layouts_and_rendering.html#using-head-to-build-header-only-responses),
  removing it from head method also.
- Closes rails#18253.
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.

9 participants
You can鈥檛 perform that action at this time.