Merge pull request #90 from alphagov/image-attachments

Image attachments

govspeak.gemspec

@@ -27,7 +27,7 @@ library for use in the UK Government Single Domain project}
   s.executables   = s.files.grep(%r{^bin/}) { |f| File.basename(f) }
   s.require_paths = ["lib"]
 
-  s.add_dependency 'kramdown', '~> 1.10.0'
+  s.add_dependency 'kramdown', '~> 1.12.0'
   s.add_dependency 'htmlentities', '~> 4'
   s.add_dependency "sanitize", "~> 2.1.0"
   s.add_dependency 'nokogiri', '~> 1.5'
@@ -42,4 +42,5 @@ library for use in the UK Government Single Domain project}
   s.add_development_dependency 'minitest', '~> 5.8.3'
   s.add_development_dependency 'simplecov'
   s.add_development_dependency 'simplecov-rcov'
+  s.add_development_dependency 'pry-byebug'
 end

lib/govspeak.rb

@@ -197,24 +197,44 @@ def insert_strong_inside_p(body, parser=Govspeak::Document)
       ERB.new(content).result(binding)
     end
 
-    extension('attachment inline', /\[embed:attachments:inline:([0-9a-f-]+)\]/) do |content_id, body|
+    extension('attachment inline', /\[embed:attachments:inline:([0-9a-f-]+)\]/) do |content_id|
       attachment = attachments.detect { |a| a[:content_id].match(content_id) }
       next "" unless attachment
       attachment = AttachmentPresenter.new(attachment)
       span_id = attachment.id ? %{ id="attachment_#{attachment.id}"} : ""
       # new lines inside our title cause problems with govspeak rendering as this is expected to be on one line.
-      link = attachment.link(attachment.title.gsub("\n", " "), attachment.url)
+      title = (attachment.title || "").tr("\n", " ")
+      link = attachment.link(title, attachment.url)
       attributes = attachment.attachment_attributes.empty? ? "" : " (#{attachment.attachment_attributes})"
       %{<span#{span_id} class="attachment-inline">#{link}#{attributes}</span>}
     end
 
-    def render_image(url, alt_text, caption = nil)
+    extension('attachment image', /\[embed:attachments:image:([0-9a-f-]+)\]/) do |content_id|
+      attachment = attachments.detect { |a| a[:content_id].match(content_id) }
+      next "" unless attachment
+      attachment = AttachmentPresenter.new(attachment)
+      title = (attachment.title || "").tr("\n", " ")
+      render_image(attachment.url, title, nil, attachment.id)
+    end
+
+    # As of version 1.12.0 of Kramdown the block elements (div & figcaption)
+    # inside this html block will have it's < > converted into HTML Entities
+    # when ever this code is used inside block level elements.
+    #
+    # To resolve this we have a post-processing task that will convert this
+    # back into HTML (I know - it's ugly). The way we could resolve this
+    # without ugliness would be to output only inline elements which rules
+    # out div and figcaption
+    #
+    # This issue is not considered a bug by kramdown: https://github.com/gettalong/kramdown/issues/191
+    def render_image(url, alt_text, caption = nil, id = nil)
+      id_attr = id ? %{ id="attachment_#{id}"} : ""
       lines = []
-      lines << '<figure class="image embedded">'
-      lines << %Q{  <div class="img"><img alt="#{encode(alt_text)}" src="#{encode(url)}" /></div>}
-      lines << %Q{  <figcaption>#{encode(caption.strip)}</figcaption>} if caption && !caption.strip.empty?
+      lines << %{<figure#{id_attr} class="image embedded">}
+      lines << %Q{<div class="img"><img src="#{encode(url)}" alt="#{encode(alt_text)}"></div>}
+      lines << %Q{<figcaption>#{caption.strip}</figcaption>} if caption && !caption.strip.empty?
       lines << '</figure>'
-      lines.join "\n"
+      lines.join
     end
 
     wrap_with_div('summary', '$!')

lib/govspeak/post_processor.rb

@@ -38,5 +38,31 @@ def output
         el[:class] = "last-child"
       end
     end
+
+    # This "fix" here is tied into the rendering of images as one of the
+    # pre-processor tasks. As images can be created inside block level elements
+    # it's possible that their block level elements can be HTML entity escaped
+    # to produce "valid" HTML.
+    #
+    # This sucks for us as we spit the user out HTML elements.
+    #
+    # This fix reverses this, and of course, totally sucks because it's tightly
+    # coupled to the `render_image` code and it really isn't cool to undo HTML
+    # entity encoding.
+    extension("fix image attachment escaping") do |document|
+      document.css("figure.image").map do |el|
+        xml = el.children.to_s
+        next unless xml =~ /<div class="img">|<figcaption>/
+        el.children = xml
+          .gsub(
+            %r{<(div class="img")>(.*?)<(/div)>},
+            "<\\1>\\2<\\3>"
+          )
+          .gsub(
+            %r{&lt;(figcaption)&gt;(.*?)&lt;(/figcaption&)gt;},
+            "<\\1>\\2<\\3>"
+          )
+      end
+    end
   end
 end

test/govspeak_attachments_image_test.rb

@@ -0,0 +1,90 @@
+# encoding: UTF-8
+
+require 'test_helper'
+
+class GovspeakAttachmentsImageTest < Minitest::Test
+  def build_attachment(args = {})
+    {
+      content_id: "2b4d92f3-f8cd-4284-aaaa-25b3a640d26c",
+      id: 456,
+      url: "http://example.com/attachment.jpg",
+      title: "Attachment Title",
+    }.merge(args)
+  end
+
+  def render_govspeak(govspeak, attachments = [])
+    Govspeak::Document.new(govspeak, attachments: attachments).to_html
+  end
+
+  def compress_html(html)
+    html.gsub(/[\n\r]+[\s]*/, '')
+  end
+
+  test "renders an empty string for an image attachment not found" do
+    assert_match("", render_govspeak("[embed:attachments:image:1fe8]", [build_attachment]))
+  end
+
+  test "wraps an attachment in a figure with the id if the id is present" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: 10, content_id: "1fe8")]
+    )
+    assert_match(/<figure id="attachment_10" class="image embedded">/, rendered)
+  end
+
+  test "wraps an attachment in a figure without the id if the id is not present" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: nil, content_id: "1fe8")]
+    )
+    assert_match(/<figure class="image embedded">/, rendered)
+  end
+
+  test "has an image element to the file" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: nil, url: "http://a.b/c.jpg", content_id: "1fe8")]
+    )
+    assert_match(%r{<img.*src="http://a.b/c.jpg"}, rendered)
+  end
+
+  test "renders the image title as an alt tag" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: nil, title: "My Title", content_id: "1fe8")]
+    )
+    assert_match(%r{<img.*alt="My Title"}, rendered)
+  end
+
+  test "can render a nil image title" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: nil, title: nil, content_id: "1fe8")]
+    )
+    assert_match(%r{<img.*alt=""}, rendered)
+  end
+
+  test "a full image attachment rendering looks correct" do
+    rendered = render_govspeak(
+      "[embed:attachments:image:1fe8]",
+      [build_attachment(id: 10, url: "http://a.b/c.jpg", title: "My Title", content_id: "1fe8")]
+    )
+    expected_html_output = %{
+      <figure id="attachment_10" class="image embedded">
+        <div class="img"><img src="http://a.b/c.jpg" alt="My Title"></div>
+      </figure>
+    }
+    assert_match(compress_html(expected_html_output), compress_html(rendered))
+  end
+
+  # test inserted because divs can be stripped inside a table
+  test "can be rendered inside a table" do
+    rendered = render_govspeak(
+      "| [embed:attachments:image:1fe8] |",
+      [build_attachment(content_id: "1fe8", id: nil)]
+    )
+
+    regex = %r{<td><figure class="image embedded"><div class="img">(.*?)</div></figure></td>}
+    assert_match(regex, rendered)
+  end
+end

test/govspeak_attachments_inline_test.rb

@@ -25,15 +25,15 @@ def render_govspeak(govspeak, attachments = [])
       "[embed:attachments:inline:1fe8]",
       [build_attachment(id: 10, content_id: "1fe8")]
     )
-    assert_match(/span id="attachment_10" class="attachment-inline">/, rendered)
+    assert_match(/<span id="attachment_10" class="attachment-inline">/, rendered)
   end
 
   test "wraps an attachment in a span without the id if the id is not present" do
     rendered = render_govspeak(
       "[embed:attachments:inline:1fe8]",
       [build_attachment(id: nil, content_id: "1fe8")]
     )
-    assert_match(/span class="attachment-inline">/, rendered)
+    assert_match(/<span class="attachment-inline">/, rendered)
   end
 
   test "links to the attachment file" do
@@ -44,6 +44,14 @@ def render_govspeak(govspeak, attachments = [])
     assert_match(%r{<a href="http://a.b/f.pdf">My Pdf</a>}, rendered)
   end
 
+  test "renders with a nil title" do
+    rendered = render_govspeak(
+      "[embed:attachments:inline:1fe8]",
+      [build_attachment(content_id: "1fe8", url: "http://a.b/f.pdf", title: nil)]
+    )
+    assert_match(%r{<a href="http://a.b/f.pdf"></a>}, rendered)
+  end
+
   test "renders on a single line" do
     rendered = render_govspeak(
       "[embed:attachments:inline:2bc1]",

test/govspeak_test.rb

@@ -634,22 +634,22 @@ class GovspeakTest < Minitest::Test
   test "can reference attached images using !!n" do
     images = [OpenStruct.new(alt_text: 'my alt', url: "http://example.com/image.jpg")]
     given_govspeak "!!1", images do
-      assert_html_output %Q{
-          <figure class="image embedded">
-            <div class="img"><img alt="my alt" src="http://example.com/image.jpg"></div>
-          </figure>
-      }
+      assert_html_output(
+        %{<figure class="image embedded">} +
+        %{<div class="img"><img src="http://example.com/image.jpg" alt="my alt"></div>} +
+        %{</figure>}
+      )
   end
 end
 
 test "alt text of referenced images is escaped" do
   images = [OpenStruct.new(alt_text: %Q{my alt '&"<>}, url: "http://example.com/image.jpg")]
   given_govspeak "!!1", images do
-    assert_html_output %Q{
-        <figure class="image embedded">
-          <div class="img"><img alt="my alt '&amp;&quot;&lt;&gt;" src="http://example.com/image.jpg"></div>
-        </figure>
-        }
+    assert_html_output(
+      %{<figure class="image embedded">} +
+      %{<div class="img"><img src="http://example.com/image.jpg" alt="my alt '&amp;&quot;&lt;&gt;"></div>} +
+      %{</figure>}
+    )
     end
   end
 
@@ -663,23 +663,23 @@ class GovspeakTest < Minitest::Test
   test "adds image caption if given" do
     images = [OpenStruct.new(alt_text: "my alt", url: "http://example.com/image.jpg", caption: 'My Caption & so on')]
     given_govspeak "!!1", images do
-      assert_html_output %Q{
-          <figure class="image embedded">
-            <div class="img"><img alt="my alt" src="http://example.com/image.jpg"></div>
-            <figcaption>My Caption &amp; so on</figcaption>
-          </figure>
-        }
+      assert_html_output(
+        %{<figure class="image embedded">} +
+        %{<div class="img"><img src="http://example.com/image.jpg" alt="my alt"></div>\n} +
+        %{<figcaption>My Caption &amp; so on</figcaption>} +
+        %{</figure>}
+      )
     end
   end
 
   test "ignores a blank caption" do
     images = [OpenStruct.new(alt_text: "my alt", url: "http://example.com/image.jpg", caption: '  ')]
     given_govspeak "!!1", images do
-      assert_html_output %Q{
-          <figure class="image embedded">
-            <div class="img"><img alt="my alt" src="http://example.com/image.jpg"></div>
-          </figure>
-        }
+      assert_html_output(
+        %{<figure class="image embedded">} +
+        %{<div class="img"><img src="http://example.com/image.jpg" alt="my alt"></div>} +
+        %{</figure>}
+      )
     end
   end