Make Emoji img attributes configurable

[JuanitoFatas]

Resolves #234

This Pull Request makes img tag attributes configuarable:

• Keep old behavior
• Clear attribute you don't want, set to nil ("height" => nil, "width" => nil, "align" => nil)
• Override / Clear img tag attributes
  • Override by passing hash, e.g. "draggable"=> false
  • Attribute value also accepts proc-like object with name argument
• patch version bump because we're compatible with old behavior => 2.4.2
• These deprecated img attributes (height='20' width='20' align='absmiddle') can be removed when there is a next major / minor version bump

@jch What do you think?

[jch]

I'm wary of this change because unsafe user input could be injected here. Could you look whitelisting only attributes that make sense for a img element?

[JuanitoFatas]

I'm wary of this change because unsafe user input could be injected here.

This is not from user input. It is context option used by developer.

Could you look whitelisting only attributes that make sense for a img element?

ditto, let's leave this to developer?

[jch]

Good point. I missed that.

• Could you update the documentation at the top of the class explaining this new context option?
• What do you think about simplifying the methods? The new methods are not part of the public interface, and I feel they add more layers than necessary. What about doing this new work in the existing emoji_image_tag method?

edit noticed the new methods are private, but still feel they add too many layers.

[JuanitoFatas]

@jch Thanks for the review, updated in 1b3e5e4 :)

[jch]

This method can be inlined as well. Similar reasoning as before. Here, we're mutating the context hash by adding more elements to it. While that's doable, it makes it difficult to debug pipelines because it's unclear which filter modified a context. Instead, we can leave the context as passed in options and use it's values. I haven't tested this, but I imagine something like:

default_attrs = default_img_attrs(name)

# This avoids creating an extra empty hash for each invocation and only merges if user passed something in
merged_attrs = context[:img_attrs] ? default_attrs.merge(context[:img_attrs]) : default_attrs

# This builds the attributes and handles procs. In your implementation, you remove nils, but I think that should be the responsibility of the caller. Nils would print as an empty value, which would still be valid.
html_attrs = merged_attrs.map do |k, v| 
  "#{k}='#{v.try(:call) || v}'"
end

"<img #{html_attrs.join(' '}>"

[JuanitoFatas]

This method can be inlined as well. Similar reasoning as before. Here, we're mutating the context hash by adding more elements to it. While that's doable, it makes it difficult to debug pipelines because it's unclear which filter modified a context

👍 Awesome!

Instead, we can leave the context as passed in options and use it's values. I haven't tested this, but I imagine something like:

I adapt your imagination as f1c1618.

In your implementation, you remove nils, but I think that should be the responsibility of the caller. Nils would print as an empty value, which would still be valid.

This is because #234, user of HTML::Pipeline need to clear some unwanted default attributes to pass the W3C validations.

Another change I added is indifferent access to context[:img_attrs] hash .

Why? Because a user may be tempted to use { height: nil, width: nil } to clear height and width attributes. Without indifferent access, the keys of Hash must be String. Better developer experience in my opinion.

[JuanitoFatas]

If you think this is mergable, I will squash commits, merge into master. Add an changelog item and release a patch version :)

[jch]

I will squash commits, merge into master. Add an changelog item and release a patch version :)

Sounds good! Thanks for tackling this.

[JuanitoFatas]

Merged as 3518bbb.

[kbrock]

Thank you @JuanitoFatas - this was a much needed feature and looks great