resolves asciidoctor#4587 only drop current row if colspan of last ce…

…ll exceeds specified number of columns (PR asciidoctor#4588)

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
 

lib/asciidoctor/table.rb

@@ -661,23 +661,24 @@ def close_cell eol = false
           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 @@ def close_cell eol = false
   # 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 @@ def activate_rowspan rowspan, colspan
   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

test/tables_test.rb

@@ -1194,7 +1194,7 @@
       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*]
       |===
@@ -1205,11 +1205,60 @@
       more C
       |===
       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()="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