Refactoring and addressing review comments

and additional spec

changelogs/unreleased/26375-markdown-footnotes-not-working.yml

@@ -1,5 +1,5 @@
 ---
-title: Footnotes now work render properly in markdown
+title: Footnotes now render properly in markdown
 merge_request: 24168
 author:
 type: fixed

lib/banzai/filter/footnote_filter.rb

@@ -16,19 +16,24 @@ module Banzai
     # can be used for a single render). So you get `id=fn1-4335` and `id=fn2-4335`.
     #
     class FootnoteFilter < HTML::Pipeline::Filter
-      INTEGER_PATTERN = /\A\d+\z/.freeze
+      INTEGER_PATTERN                 = /\A\d+\z/.freeze
+      FOOTNOTE_ID_PREFIX              = 'fn'.freeze
+      FOOTNOTE_LINK_ID_PREFIX         = 'fnref'.freeze
+      FOOTNOTE_LI_REFERENCE_PATTERN   = /\A#{FOOTNOTE_ID_PREFIX}\d+\z/.freeze
+      FOOTNOTE_LINK_REFERENCE_PATTERN = /\A#{FOOTNOTE_LINK_ID_PREFIX}\d+\z/.freeze
+      FOOTNOTE_START_NUMBER           = 1
 
       def call
-        return doc unless first_footnote = doc.at_css('ol > li[id=fn1]')
+        return doc unless first_footnote = doc.at_css("ol > li[id=#{fn_id(FOOTNOTE_START_NUMBER)}]")
 
         # Sanitization stripped off the section wrapper - add it back in
         first_footnote.parent.wrap('<section class="footnotes">')
         rand_suffix = "-#{random_number}"
 
         doc.css('sup > a[id]').each do |link_node|
-          ref_num       = link_node[:id].delete_prefix('fnref')
+          ref_num       = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX)
-          footnote_node = doc.at_css("li[id=fn#{ref_num}]")
+          footnote_node = doc.at_css("li[id=#{fn_id(ref_num)}]")
-          backref_node  = footnote_node.at_css("a[href=\"#fnref#{ref_num}\"]")
+          backref_node  = footnote_node.at_css("a[href=\"##{fnref_id(ref_num)}\"]")
 
           if ref_num =~ INTEGER_PATTERN && footnote_node && backref_node
             link_node[:href]    += rand_suffix
@@ -50,6 +55,14 @@ module Banzai
       def random_number
         @random_number ||= rand(10000)
       end
+
+      def fn_id(num)
+        "#{FOOTNOTE_ID_PREFIX}#{num}"
+      end
+
+      def fnref_id(num)
+        "#{FOOTNOTE_LINK_ID_PREFIX}#{num}"
+      end
     end
   end
 end

lib/banzai/filter/sanitization_filter.rb

@@ -8,10 +8,8 @@ module Banzai
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       include Gitlab::Utils::StrongMemoize
 
-      UNSAFE_PROTOCOLS                = %w(data javascript vbscript).freeze
+      UNSAFE_PROTOCOLS        = %w(data javascript vbscript).freeze
-      TABLE_ALIGNMENT_PATTERN         = /text-align: (?<alignment>center|left|right)/.freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/.freeze
-      FOOTNOTE_LINK_REFERENCE_PATTERN = /\Afnref\d+\z/.freeze
-      FOOTNOTE_LI_REFERENCE_PATTERN   = /\Afn\d+\z/.freeze
 
       def whitelist
         strong_memoize(:whitelist) do
@@ -47,10 +45,9 @@ module Banzai
         whitelist[:attributes][:all].delete('name')
         whitelist[:attributes]['a'].push('name')
 
-        # Allow any protocol in `a` elements...
+        # Allow any protocol in `a` elements
+        # and then remove links with unsafe protocols
         whitelist[:protocols].delete('a')
-
-        # ...but then remove links with unsafe protocols
         whitelist[:transformers].push(self.class.remove_unsafe_links)
 
         # Remove `rel` attribute from `a` elements
@@ -60,10 +57,9 @@ module Banzai
         whitelist[:transformers].push(self.class.remove_unsafe_table_style)
 
         # Allow `id` in a and li elements for footnotes
+        # and remove any `id` properties not matching for footnotes
         whitelist[:attributes]['a'].push('id')
         whitelist[:attributes]['li'] = %w(id)
-
-        # ...but remove any `id` properties not matching for footnotes
         whitelist[:transformers].push(self.class.remove_non_footnote_ids)
 
         whitelist
@@ -129,8 +125,8 @@ module Banzai
             return unless node.name == 'a' || node.name == 'li'
             return unless node.has_attribute?('id')
 
-            return if node.name == 'a' && node['id'] =~ FOOTNOTE_LINK_REFERENCE_PATTERN
+            return if node.name == 'a' && node['id'] =~ Banzai::Filter::FootnoteFilter::FOOTNOTE_LINK_REFERENCE_PATTERN
-            return if node.name == 'li' && node['id'] =~ FOOTNOTE_LI_REFERENCE_PATTERN
+            return if node.name == 'li' && node['id'] =~ Banzai::Filter::FootnoteFilter::FOOTNOTE_LI_REFERENCE_PATTERN
 
             node.remove_attribute('id')
           end

spec/lib/banzai/filter/footnote_filter_spec.rb

@@ -9,16 +9,30 @@ describe Banzai::Filter::FootnoteFilter do
   # [^1]: one
   # [^second]: two
   let(:footnote) do
-    <<-EOF.strip_heredoc
+    <<~EOF
-    <p>first<sup><a href="#fn1" id="fnref1">1</a></sup> and second<sup><a href="#fn2" id="fnref2">2</a></sup></p>
+      <p>first<sup><a href="#fn1" id="fnref1">1</a></sup> and second<sup><a href="#fn2" id="fnref2">2</a></sup></p>
-    <ol>
+      <ol>
-    <li id="fn1">
+      <li id="fn1">
-    <p>one <a href="#fnref1">↩</a></p>
+      <p>one <a href="#fnref1">↩</a></p>
-    </li>
+      </li>
-    <li id="fn2">
+      <li id="fn2">
-    <p>two <a href="#fnref2">↩</a></p>
+      <p>two <a href="#fnref2">↩</a></p>
-    </li>
+      </li>
-    </ol>
+      </ol>
+    EOF
+  end
+
+  let(:filtered_footnote) do
+    <<~EOF
+      <p>first<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup> and second<sup class="footnote-ref"><a href="#fn2-#{identifier}" id="fnref2-#{identifier}">2</a></sup></p>
+      <section class="footnotes"><ol>
+      <li id="fn1-#{identifier}">
+      <p>one <a href="#fnref1-#{identifier}" class="footnote-backref">↩</a></p>
+      </li>
+      <li id="fn2-#{identifier}">
+      <p>two <a href="#fnref2-#{identifier}" class="footnote-backref">↩</a></p>
+      </li>
+      </ol></section>
     EOF
   end
 
@@ -27,22 +41,8 @@ describe Banzai::Filter::FootnoteFilter do
     let(:link_node)  { doc.css('sup > a').first }
     let(:identifier) { link_node[:id].delete_prefix('fnref1-') }
 
-    it 'adds identifier to footnotes' do
+    it 'properly adds the necessary ids and classes' do
-      expect(link_node[:id]).to eq "fnref1-#{identifier}"
+      expect(doc.to_html).to eq filtered_footnote
-      expect(link_node[:href]).to eq "#fn1-#{identifier}"
-      expect(doc.css("li[id=fn1-#{identifier}]")).not_to be_empty
-      expect(doc.css("li[id=fn1-#{identifier}] a[href=\"#fnref1-#{identifier}\"]")).not_to be_empty
-    end
-
-    it 'uses the same identifier for all footnotes' do
-      expect(doc.css("li[id=fn2-#{identifier}]")).not_to be_empty
-      expect(doc.css("li[id=fn2-#{identifier}] a[href=\"#fnref2-#{identifier}\"]")).not_to be_empty
-    end
-
-    it 'adds section and classes' do
-      expect(doc.css("section[class=footnotes]")).not_to be_empty
-      expect(doc.css("sup[class=footnote-ref]").count).to eq 2
-      expect(doc.css("a[class=footnote-backref]").count).to eq 2
     end
   end
 end

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -302,21 +302,21 @@ describe Banzai::Filter::SanitizationFilter do
     end
 
     describe 'footnotes' do
-      it 'allows id property on links' do
+      it 'allows correct footnote id property on links' do
         exp = %q{<a href="#fn1" id="fnref1">foo/bar.md</a>}
         act = filter(exp)
 
         expect(act.to_html).to eq exp
       end
 
-      it 'allows id property on li element' do
+      it 'allows correct footnote id property on li element' do
         exp = %q{<ol><li id="fn1">footnote</li></ol>}
         act = filter(exp)
 
         expect(act.to_html).to eq exp
       end
 
-      it 'only allows valid footnote formats for links' do
+      it 'removes invalid id for footnote links' do
         exp = %q{<a href="#fn1">link</a>}
 
         %w[fnrefx test xfnref1].each do |id|
@@ -326,7 +326,7 @@ describe Banzai::Filter::SanitizationFilter do
         end
       end
 
-      it 'only allows valid footnote formats for li' do
+      it 'removes invalid id for footnote li' do
         exp = %q{<ol><li>footnote</li></ol>}
 
         %w[fnx test xfn1].each do |id|

spec/lib/banzai/pipeline/full_pipeline_spec.rb

@@ -25,4 +25,36 @@ describe Banzai::Pipeline::FullPipeline do
       expect(result).to include(%{data-original='\">bad things'})
     end
   end
+
+  describe 'footnotes' do
+    let(:project)    { create(:project, :public) }
+    let(:html)       { described_class.to_html(footnote_markdown, project: project) }
+    let(:identifier) { html[/.*fnref1-(\d+).*/, 1] }
+    let(:footnote_markdown) do
+      <<~EOF
+        first[^1] and second[^second]
+        [^1]: one
+        [^second]: two
+      EOF
+    end
+
+    let(:filtered_footnote) do
+      <<~EOF
+        <p dir="auto">first<sup class="footnote-ref"><a href="#fn1-#{identifier}" id="fnref1-#{identifier}">1</a></sup> and second<sup class="footnote-ref"><a href="#fn2-#{identifier}" id="fnref2-#{identifier}">2</a></sup></p>
+
+        <section class="footnotes"><ol>
+        <li id="fn1-#{identifier}">
+        <p>one <a href="#fnref1-#{identifier}" class="footnote-backref"><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        </li>
+        <li id="fn2-#{identifier}">
+        <p>two <a href="#fnref2-#{identifier}" class="footnote-backref"><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
+        </li>
+        </ol></section>
+      EOF
+    end
+
+    it 'properly adds the necessary ids and classes' do
+      expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote
+    end
+  end
 end