Skip to content

Commit

Permalink
Rejig the html5 data helper code
Browse files Browse the repository at this point in the history 
Avoid allocating an array each pass through and support String subclasses like SafeBuffers
  • Loading branch information
@NZKoz
NZKoz committed Oct 17, 2010
1 parent 5e79094 commit 2f9e880
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions actionpack/lib/action_view/helpers/tag_helper.rb
View file
Expand Up @@ -127,9 +127,11 @@ def tag_options(options, escape = true)
options.each_pair do |key, value| options.each_pair do |key, value|
if key.to_s == 'data' && value.is_a?(Hash) if key.to_s == 'data' && value.is_a?(Hash)
value.each do |k, v| value.each do |k, v|

This comment has been minimized.

Sign in to view

Copy link
@stephencelis

stephencelis Oct 18, 2010

Contributor

Good tweaks, thanks. This could also be further optimized by calling value.each_pair instead.
final_v = [String, Symbol].include?(v.class) ? v : v.to_json if !v.is_a?(String) && !v.is_a?(Symbol)
final_v = html_escape(final_v) if escape v = v.to_json

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 18, 2010

Member

Just wondering, what happens if you pass a string that contains a valid JSON literal?

Let's suppose for some reason my string contains "true", and jQuery should not pick that data value as a boolean, but as a mere string. Same if the string contains "0", "[1, 2]", etc. If I pass a string I'd expect a string in the JavaScript side.

Is that correctly handled?

This comment has been minimized.

Sign in to view

Copy link
@josevalim

josevalim Oct 18, 2010

Contributor

I believe we had a test for this in the previous commit. So it will probably work.

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 18, 2010

Member

Yeah indeed I commented here but the question is really about the previous commit, which is 5e79094

As you see, the test checks that literals of different data types map to those data types, so if I pass an integer 1 I get an integer 1. It does not seem to cover strings in the sense I mention above, ie, the string "1".

Guess jQuery or whatever library you use will need a little help to know you mean the string "1". Perhaps you need in those cases to build a real string literal? And if it was the case, would it be better to just output always a string literal?

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 18, 2010

Member

By "always" I mean that if it is a string or symbol you generate a JSON string (which include quotes, that would be html_escape'd). It is really a question, I have not played with jQuery and .data yet.

This comment has been minimized.

Sign in to view

Copy link
@stephencelis

stephencelis Oct 18, 2010

Contributor

This all depends on how the external library handles it. Assuming you know how your library handles strings in data-* attributes, you would want to escape JSON-able strings by using String#to_json or #inspect. It makes the most sense for Rails to assume strings (and symbols) should not be escaped because the native HTML5 JavaScript API doesn't automatically interpret JSON from an object's dataset.

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 18, 2010

Member

This is a gotcha then in my view.

Usage is not transparent/portable enough. Do not know if anything better can be done, but perhaps this should be investigated with Prototype and jQuery and warnings documented if needed.

This comment has been minimized.

Sign in to view

Copy link
@stephencelis

stephencelis Oct 18, 2010

Contributor

It's a matter of being familiar with your library. Shouldn't jQuery provide the documentation if you're using the jQuery data() convenience function?

Consider the native JS approach:

<div data-string-test="string" data-array-test="[1,2,3]" id="data_div" />
<script>
var el = document.getElementById("data_div");
el.dataset.stringTest; // "string"
el.dataset.arrayTest;  // "[1,2,3]"
<script>

Everything is a string. We're just simplifying by converting Ruby objects to JavaScript objects. Without a JS library, you can JSON-eval whatever you want. If you need fine-grained controls in returning strings or not, you'll have to step outside of jQuery.

This comment has been minimized.

Sign in to view

Copy link
@stephencelis

stephencelis Oct 18, 2010

Contributor

I guess what I'm trying to say is this is library-agnostic code. If I missed any native JS concerns, though, please let me know.
attrs << %(data-#{k.to_s.dasherize}="#{final_v}") end
v = html_escape(v) if escape
attrs << %(data-#{k.to_s.dasherize}="#{v}")
end end
elsif BOOLEAN_ATTRIBUTES.include?(key) elsif BOOLEAN_ATTRIBUTES.include?(key)
attrs << %(#{key}="#{key}") if value attrs << %(#{key}="#{key}") if value
Expand Down

11 comments on commit 2f9e880

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 18, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, if you see that feature, and you know jQuery's .data, the first thought you have is that the feature has been programmed so that they play nicely. And that is not the case. I believe the user has to be warned.

The user has to know when is this useful, why does the feature render JSON for some data types and not others (???), and how to use it in real scenarios with real examples involving modern JavaScript APIs.

If everything matched and was transparent, this documentation would be much shorter.

A warning like "Careful, strings are just passed by. In particular they may be misinterpreted if they happen to contain JSON literals, so you should need to do this and that". Or, "Careful, this JSONifies structures, but in general you will need to handle types manually. In Prototype you would do this... in jQuery you would do that...". Only covering data- APIs, not bare JavaScript.

@stephencelis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything matches and is transparent to HTML5 spec (that is, everything is a string, and we convert non-stringlikes into JSON strings).

The gotcha is really with jQuery. If Rails documentation provided jQuery-specific examples, and jQuery changes how it decides to JSON-eval, Rails would also have to change its documentation. I think it's best to keep the documentation agnostic, with at most a sentence to urge users to check the documentation of their JavaScript library when using library-specific APIs.

Don't get me wrong, I agree that there are gotchas, but I think the confusion lies with jQuery, not necessarily Rails. If Rails needs to provide more documentation, though, it should.

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 18, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good, but I think it is not enough.

First, the provided documentation does not explain well what does this new feature do. It has only a vague "HTML5 data-* attributes can be set with a single +data+ key and a hash value of sub-attributes. Sub-attribute keys will be dasherized." No mention to what is JSONified and what not.

Second, the documentation should warn users that you can't directly use that with jQuery's new data interface, and that if you want to (which is what everyone will want to do), the recommended way is X.

@stephencelis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick look at jQuery:

<a data-escaped-true="&quot;true&quot;" data-true="true" />
<script>
$('a').data('escaped-true') // "\"true\"" (instead of "true")
$('a').data('true')         // true
</script>

Their documentation is here: http://api.jquery.com/jQuery.data/#jQuery-data2

Without digging into the jQuery source, I'm not sure if it's even possible to escape JSON objects from being eval'd (that is, a quoted string isn't eval'd, so the string jQuery returns is surrounded by extra quotation marks). jQuery also doesn't camelCase the key names, as JavaScript does for the DOMStringMap object that .dataset returns. This all seems pretty jQuery-specific to me, but I'm happy to help with more documentation if necessary.

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 18, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. At least I'd document with more detail what does the feature do. The user needs to know that most stuff is JSONfied.

There are gotchas here for sure, dataset gives you a 1 from the string attribute value "1". But I can't find by now how to pass the string "1". The specs mention an algorithm to extract the value from a name in a DOMStringMap but I can't find it. Does anybody know where's that covered?

@stephencelis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataset gives you a 1 from the string attribute value "1".

Are you sure? In WebKit:

<a data-number="1" />
>> document.getElementsByTagName('a')[0].dataset.number
=> "1"

I'm pretty sure a DOMStringMap dataset is supposed to return strings, or, according to spec, DOMStrings:

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#domstringmap.

I'll throw together a documentation patch for the JSON bit if no one else wants to take a stab at it.

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 18, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw some posts with integers like http://html5doctor.com/html5-custom-data-attributes/ (search for "leaves = 47"), but a post is a post, and the draft seems clear.

The code that converts the value in the new .data function in jQuery is:

// If nothing was found internally, try to fetch any
// data from the HTML5 data-* attribute
if ( data === undefined && this[0].nodeType === 1 ) {
    data = this[0].getAttribute( "data-" + key );

    if ( typeof data === "string" ) {
        try {
            data = data === "true" ? true :
                data === "false" ? false :
                data === "null" ? null :
                !jQuery.isNaN( data ) ? parseFloat( data ) :
                rbrace.test( data ) ? jQuery.parseJSON( data ) :
                data;
        } catch( e ) {}

    } else {
        data = undefined;
    }
}

where rbrace looks for object and array literals:

/^(?:\{.*\}|\[.*\])$/;

So the gotcha is in jQuery itself and don't see a way to pass the string "true" or "1". I mean, directly. As a workaround you could pass a dummy array with one string or some trick like that for example. Except for this inherent limitation, passing stuff with the idiom you just added is useful for that .data function. And seems the natural consumer to me in the general case, given the JSON encodings/decodings. For simple values any bare .dataset access works of course.

Cool. Thanks for the thread.

Would you like to complete the description of the feature then, to explain that it does with different datatypes?

@stephencelis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I'll get a patch together later today.

@jfirebaugh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For jQuery, if you want to get the strings "true" or "1" out instead of the interpreted values, you can just use attr('data-whatever') instead of data('whatever').

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 19, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Problem is you need to know that in advance in the JavaScript side, as with the singleton array trick. The contract of .data is almost there, it is almost a generic contract for passing values of different datatypes. Except for this edge case.

@fxn
Copy link
Member

@fxn fxn commented on 2f9e880 Oct 19, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, documentation is now http://github.com/rails/rails/compare/4120e95...de3603c

I've added it to the CHANGELOG as well.

Please sign in to comment.