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.

Sign up for GitHub

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

Jump to bottom

Make Emoji img attributes configurable #258

Closed
wants to merge 3 commits into from
Closed

Make Emoji img attributes configurable #258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e5cd67
Make Emoji img attributes configurable
JuanitoFatas Jun 28, 2016
1b3e5e4
Add docs and remove indirections
JuanitoFatas Jun 29, 2016
f1c1618
Put all logics into the emoji_image_tag method
JuanitoFatas Jun 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion lib/html/pipeline/emoji_filter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,42 @@ def asset_path(name)

# Build an emoji image tag
def emoji_image_tag(name)
"<img class='emoji' title=':#{name}:' alt=':#{name}:' src='#{emoji_url(name)}' height='20' width='20' align='absmiddle' />"
"<img #{img_html_attrs(name)}>"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@jch jch Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

end

def img_html_attrs(name)
img_attrs(name).map { |attr, value| %(#{attr}="#{value}") }.join(" ")
end

def img_attrs(name)
user_overrides = customized_attrs(name).select { |k, v| !v.nil? }
excluded_keys = customized_attrs(name).select { |k, v| v.nil? }.keys
result = default_img_attrs(name).merge!(user_overrides)
result.except(*excluded_keys)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this to just merge the hashes and iterate through the values inline? This method creates several intermediate structures that aren't necessary and increase memory usage.


def default_img_attrs(name)
{
"class" => "emoji",
"title" => ":#{name}:",
"alt" => ":#{name}:",
"src" => "#{emoji_url(name)}",
"height" => "20",
"width" => "20",
"align" => "absmiddle",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for keeping this backwards compatible

}
end

def customized_attrs(name)
return {} unless context[:img_attrs]

@_custom_img_attributes ||= begin
custom_img_attributes = context[:img_attrs]

custom_img_attributes.each do |key, value|
custom_img_attributes[key] = value.call(name) if value.respond_to?(:call)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

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(' '}>"

Copy link
Contributor Author

@JuanitoFatas JuanitoFatas Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

end

def emoji_url(name)
Expand Down
22 changes: 22 additions & 0 deletions test/html/pipeline/emoji_filter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,26 @@ def test_not_emojify_in_custom_multiple_tags_foo_and_bar
doc = filter.call
assert_equal body, doc.to_html
end

def test_img_tag_attributes
body = ":shipit:"
filter = EmojiFilter.new(body, {:asset_root => 'https://foo.com'})
doc = filter.call
assert_equal %(<img class="emoji" title=":shipit:" alt=":shipit:" src="https://foo.com/emoji/shipit.png" height="20" width="20" align="absmiddle">), doc.to_html
end

def test_img_custom_tag_attributes
body = ":shipit:"
filter = EmojiFilter.new(body, {:asset_root => 'https://foo.com', img_attrs: Hash("draggable"=> false, "height" => nil, "width" => nil, "align" => nil)})
doc = filter.call
assert_equal %(<img class="emoji" title=":shipit:" alt=":shipit:" src="https://foo.com/emoji/shipit.png" draggable=\"false\">), doc.to_html
end

def test_img_attr_accept_proclike_object
remove_colons = ->(name) { name.gsub(":", "") }
body = ":shipit:"
filter = EmojiFilter.new(body, {:asset_root => 'https://foo.com', img_attrs: Hash("title" => remove_colons)})
doc = filter.call
assert_equal %(<img class="emoji" title="shipit" alt=":shipit:" src="https://foo.com/emoji/shipit.png" height="20" width="20" align="absmiddle">), doc.to_html
end
end