From d243b82fb3c9ac3944a549f329fd3522a4747e5d Mon Sep 17 00:00:00 2001
From: Gerhard Schlager <mail@gerhard-schlager.at>
Date: Mon, 12 Mar 2018 16:52:06 +0100
Subject: [PATCH] FIX: Calculation of text length for <details> in excerpt was
 wrong

---
 lib/excerpt_parser.rb                  | 37 +++++++++++++++++++-------
 spec/components/excerpt_parser_spec.rb | 29 +++++++++++++++-----
 spec/components/pretty_text_spec.rb    |  2 +-
 3 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/lib/excerpt_parser.rb b/lib/excerpt_parser.rb
index 295515ea2a4..96400ade31c 100644
--- a/lib/excerpt_parser.rb
+++ b/lib/excerpt_parser.rb
@@ -49,7 +49,8 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
   end
 
   def include_tag(name, attributes)
-    characters("<#{name} #{attributes.map { |k, v| "#{k}=\"#{escape_attribute(v)}\"" }.join(' ')}>", false, false, false)
+    characters("<#{name} #{attributes.map { |k, v| "#{k}=\"#{escape_attribute(v)}\"" }.join(' ')}>",
+               truncate: false, count_it: false, encode: false)
   end
 
   def start_element(name, attributes = [])
@@ -130,12 +131,12 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
     case name
     when "a"
       unless @strip_links
-        characters("</a>", false, false, false)
+        characters("</a>", truncate: false, count_it: false, encode: false)
         @in_a = false
       end
     when "p", "br"
       if @keep_newlines
-        characters("<br>", false, false, false)
+        characters("<br>", truncate: false, count_it: false, encode: false)
       else
         characters(" ")
       end
@@ -144,18 +145,31 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
     when "details"
       @in_details_depth -= 1
       if @in_details_depth == 0
-        full = "<details><summary>#{clean(@summary_contents)}</summary>#{clean(@detail_contents)}</details>"
-        if @current_length + full.length > @length
-          @excerpt << "<details class='disabled'><summary>#{@summary_contents[0..@length]}</summary></details>"
+        @summary_contents = clean(@summary_contents)
+        @detail_contents = clean(@detail_contents)
+
+        if @current_length + @summary_contents.length >= @length
+          characters(@summary_contents,
+                     encode: false,
+                     before_string: "<details class='disabled'><summary>",
+                     after_string: "</summary></details>")
         else
-          @excerpt << full
+          characters(@summary_contents,
+                     truncate: false,
+                     encode: false,
+                     before_string: "<details><summary>",
+                     after_string: "</summary>")
+
+          characters(@detail_contents,
+                     encode: false,
+                     after_string: "</details>")
         end
       end
     when "summary"
       @in_summary = false if @in_details_depth == 1
     when "div", "span"
       throw :done if @start_excerpt
-      characters("</span>", false, false, false) if @in_spoiler
+      characters("</span>", truncate: false, count_it: false, encode: false) if @in_spoiler
       @in_spoiler = false
     end
   end
@@ -164,7 +178,7 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
     ERB::Util.html_escape(str.strip)
   end
 
-  def characters(string, truncate = true, count_it = true, encode = true)
+  def characters(string, truncate: true, count_it: true, encode: true, before_string: nil, after_string: nil)
     return if @in_quote
 
     # we call length on this so might as well ensure we have a string
@@ -178,15 +192,20 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
       return
     end
 
+    @excerpt << before_string if before_string
+
     encode = encode ? lambda { |s| ERB::Util.html_escape(s) } : lambda { |s| s }
     if count_it && @current_length + string.length > @length
       length = [0, @length - @current_length - 1].max
       @excerpt << encode.call(string[0..length]) if truncate
       @excerpt << (@text_entities ? "..." : "&hellip;")
       @excerpt << "</a>" if @in_a
+      @excerpt << after_string if after_string
       throw :done
     end
+
     @excerpt << encode.call(string)
+    @excerpt << after_string if after_string
     @current_length += string.length if count_it
   end
 end
diff --git a/spec/components/excerpt_parser_spec.rb b/spec/components/excerpt_parser_spec.rb
index 45e6468cbb7..8410b9d5e45 100644
--- a/spec/components/excerpt_parser_spec.rb
+++ b/spec/components/excerpt_parser_spec.rb
@@ -4,17 +4,32 @@ require "excerpt_parser"
 describe ExcerptParser do
 
   it "handles nested <details> blocks" do
-    html = <<~HTML
+    html = <<~HTML.strip
       <details>
-        <summary>FOO</summary>
-        <details>
-          <summary>BAR</summary>
-          <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Fusce ultrices, ex bibendum vestibulum vestibulum, mi velit pulvinar risus, sed consequat eros libero in eros. Fusce luctus mattis mauris, vitae semper lorem sodales quis. Donec pellentesque lacus ac ante aliquam, tincidunt iaculis risus interdum. In ullamcorper cursus massa ut lacinia. Donec quis diam finibus, rutrum odio eu, maximus leo. Nulla facilisi. Nullam suscipit quam et bibendum sagittis. Praesent sollicitudin neque at luctus ornare. Maecenas tristique dapibus risus, ac dictum ipsum gravida aliquam. Phasellus vehicula eu arcu sed imperdiet. Vestibulum ornare eros a nisi faucibus vehicula. Quisque congue placerat nulla, nec finibus nulla ultrices vitae. Quisque ac mi sem. Curabitur eu porttitor justo. Etiam dignissim in orci iaculis congue. Donec tempus cursus orci, a placerat elit varius nec.</p>
-        </details>
+      <summary>
+      FOO</summary>
+      <details>
+      <summary>
+      BAR</summary>
+      <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Fusce ultrices, ex bibendum vestibulum vestibulum, mi velit pulvinar risus, sed consequat eros libero in eros. Fusce luctus mattis mauris, vitae semper lorem sodales quis. Donec pellentesque lacus ac ante aliquam, tincidunt iaculis risus interdum. In ullamcorper cursus massa ut lacinia. Donec quis diam finibus, rutrum odio eu, maximus leo. Nulla facilisi. Nullam suscipit quam et bibendum sagittis. Praesent sollicitudin neque at luctus ornare. Maecenas tristique dapibus risus, ac dictum ipsum gravida aliquam. Phasellus vehicula eu arcu sed imperdiet. Vestibulum ornare eros a nisi faucibus vehicula. Quisque congue placerat nulla, nec finibus nulla ultrices vitae. Quisque ac mi sem. Curabitur eu porttitor justo. Etiam dignissim in orci iaculis congue. Donec tempus cursus orci, a placerat elit varius nec.</p>
+      </details>
       </details>
     HTML
 
-    expect(ExcerptParser.get_excerpt(html, 200, strip_links: true)).to eq(%{<details class='disabled'><summary>FOO</summary></details>})
+    expect(ExcerptParser.get_excerpt(html, 50, {})).to match_html(<<~HTML)
+      <details><summary>FOO</summary>BAR
+      Lorem ipsum dolor sit amet, consectetur adi&hellip;</details>
+    HTML
+
+    expect(ExcerptParser.get_excerpt(html, 6, {})).to match_html('<details><summary>FOO</summary>BAR&hellip;</details>')
+    expect(ExcerptParser.get_excerpt(html, 3, {})).to match_html('<details class="disabled"><summary>FOO</summary></details>')
   end
 
+  it "respects length parameter for <details> block" do
+    html = '<details><summary>foo</summary><p>bar</p></details>'
+    expect(ExcerptParser.get_excerpt(html, 100, {})).to match_html('<details><summary>foo</summary>bar</details>')
+    expect(ExcerptParser.get_excerpt(html, 5, {})).to match_html('<details><summary>foo</summary>ba&hellip;</details>')
+    expect(ExcerptParser.get_excerpt(html, 3, {})).to match_html('<details class="disabled"><summary>foo</summary></details>')
+    expect(ExcerptParser.get_excerpt(html, 2, {})).to match_html('<details class="disabled"><summary>fo&hellip;</summary></details>')
+  end
 end
diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb
index 347f035e95f..6e3e0f1a504 100644
--- a/spec/components/pretty_text_spec.rb
+++ b/spec/components/pretty_text_spec.rb
@@ -389,7 +389,7 @@ describe PrettyText do
       end
 
       it "should keep details if too long" do
-        expect(PrettyText.excerpt("<details><summary>expand</summary><p>hello</p></details>", 30)).to match_html "<details class='disabled'><summary>expand</summary></details>"
+        expect(PrettyText.excerpt("<details><summary>expand</summary><p>hello</p></details>", 6)).to match_html "<details class='disabled'><summary>expand</summary></details>"
       end
 
       it "doesn't disable details if short enough" do