resolves #4587 only drop current row if colspan of last cell exceeds specified number of columns (PR #4588)

3 files changed, 68 insertions, 14 deletions

diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 0b4010a7..6fe7130d 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -71,6 +71,8 @@ Bug Fixes::
   * Don't leave behind empty line inside skipped preprocessor conditional (#4580)
   * Don't duplicate block attribute line above detached block that breaks a dlist; fixes duplicate role on detached block (#4565)
   * Don't crash when parsing xref shorthand if target starts with URL protocol and text is offset by space (#4570)
+  * Only drop current row if colspan of last cell exceeds specified number of columns (#4587)
+  * Drop last row if colspan of last cell in table exceeds specified number of columns (#4587)
 
 == 2.0.22 (2024-03-08) - @mojavelinux
 
diff --git a/lib/asciidoctor/table.rb b/lib/asciidoctor/table.rb
index cc95a152..87ad4c7d 100644
--- a/lib/asciidoctor/table.rb
+++ b/lib/asciidoctor/table.rb
@@ -661,23 +661,24 @@ class Table::ParserContext
           end
         end
       else
-        # QUESTION is this right for cells that span columns?
-        unless (column = @table.columns[@current_row.size])
-          logger.error message_with_context 'dropping cell because it exceeds specified number of columns', source_location: @reader.cursor_before_mark
-          return
-        end
+        column = @table.columns[@current_row.size]
       end
 
-      cell = Table::Cell.new column, cell_text, cellspec, cursor: @reader.cursor_before_mark
+      cell = Table::Cell.new column, cell_text, cellspec, cursor: (cursor_before_mark = @reader.cursor_before_mark)
       @reader.mark
       unless !cell.rowspan || cell.rowspan == 1
         activate_rowspan(cell.rowspan, (cell.colspan || 1))
       end
       @column_visits += (cell.colspan || 1)
       @current_row << cell
-      # don't close the row if we're on the first line and the column count has not been set explicitly
-      # TODO perhaps the colcount/linenum logic should be in end_of_row? (or a should_end_row? method)
-      close_row if end_of_row? && (@colcount != -1 || @linenum > 0 || (eol && i == repeat))
+      if (row_status = end_of_row?) > -1 && (@colcount != -1 || @linenum > 0 || (eol && i == repeat))
+        if row_status > 0
+          logger.error message_with_context 'dropping cell because it exceeds specified number of columns', source_location: cursor_before_mark
+          close_row true
+        else
+          close_row
+        end
+      end
     end
     @cell_open = false
     nil
@@ -689,8 +690,8 @@ class Table::ParserContext
   # Array and counter variables.
   #
   # returns nothing
-  def close_row
-    @table.rows.body << @current_row
+  def close_row drop = false
+    @table.rows.body << @current_row unless drop
     # don't have to account for active rowspans here
     # since we know this is first row
     @colcount = @column_visits if @colcount == -1
@@ -711,8 +712,10 @@ class Table::ParserContext
   end
 
   # Internal: Check whether we've met the number of effective columns for the current row.
+  #
+  # returns -1 if not at end of row, 0 if exactly at end of row, and 1 if overruns end of row
   def end_of_row?
-    @colcount == -1 || effective_column_visits == @colcount
+    @colcount == -1 ? 0 : effective_column_visits <=> @colcount
   end
 
   # Internal: Calculate the effective column visits, which consists of the number of
diff --git a/test/tables_test.rb b/test/tables_test.rb
index ae1101a1..9b0dd04c 100644
--- a/test/tables_test.rb
+++ b/test/tables_test.rb
@@ -1194,7 +1194,7 @@ context 'Tables' do
       assert_xpath '((//row)[1]/entry)[3][@namest="col_4"][@nameend="col_5"]', output, 1
     end
 
-    test 'ignores cell with colspan that exceeds colspec' do
+    test 'should drop row but preserve remaining rows after cell with colspan exceeds number of columns' do
       input = <<~'EOS'
       [cols=2*]
       |===
@@ -1208,8 +1208,57 @@ context 'Tables' do
       using_memory_logger do |logger|
         output = convert_string_to_embedded input
         assert_css 'table', output, 1
+        assert_css 'table tr', output, 1
+        assert_xpath '/table/tbody/tr/td[1]/p[text()="B"]', output, 1
+        assert_message logger, :ERROR, '<stdin>: line 3: dropping cell because it exceeds specified number of columns', Hash
+      end
+    end
+
+    test 'should drop last row if last cell in table has colspan that exceeds specified number of columns' do
+      input = <<~'EOS'
+      [cols=2*]
+      |===
+      |a 2+|b
+      |===
+      EOS
+      using_memory_logger do |logger|
+        output = convert_string_to_embedded input
+        assert_css 'table', output, 1
         assert_css 'table *', output, 0
-        assert_message logger, :ERROR, '<stdin>: line 5: dropping cell because it exceeds specified number of columns', Hash
+        assert_message logger, :ERROR, '<stdin>: line 3: dropping cell because it exceeds specified number of columns', Hash
+      end
+    end
+
+    test 'should drop last row if last cell in table has colspan that exceeds implicit number of columns' do
+      input = <<~'EOS'
+      |===
+      |a |b
+      |c 2+|d
+      |===
+      EOS
+      using_memory_logger do |logger|
+        output = convert_string_to_embedded input
+        assert_css 'table', output, 1
+        assert_css 'table tr', output, 1
+        assert_xpath '/table/tbody/tr/td[1]/p[text()="a"]', output, 1
+        assert_message logger, :ERROR, '<stdin>: line 3: dropping cell because it exceeds specified number of columns', Hash
+      end
+    end
+
+    test 'should take colspan into account when taking cells for row' do
+      input = <<~'EOS'
+      [cols=7]
+      |===
+      2+|a 2+|b 2+|c 2+|d
+      |e |f |g |h |i |j |k
+      |===
+      EOS
+      using_memory_logger do |logger|
+        output = convert_string_to_embedded input
+        assert_css 'table', output, 1
+        assert_css 'table tr', output, 1
+        assert_css 'table tr td', output, 7
+        assert_message logger, :ERROR, '<stdin>: line 3: dropping cell because it exceeds specified number of columns', Hash
       end
     end