Patch for when rowspan or colspan equals 0

[Germain Garand]

Le samedi 4 avril 2009, Carlos Licea a écrit :
> Dear list,
> 	I've been working on my patch for implementing proper colspan and rowspan
> behaviour.
> 	Forget almost completely my previous mail, here's a new way of doing it,
> way better than the old one, which has no known corner-cases, it even
> handles properly when both colspan and rowspan equals 0 in the same cell,
> whereas the last one did but I couldn't catch them till recently.

Hi Carlos,
nice work so far!

> 	In essence the method now is:
> 	->If we found a colspan = 0 we insert the cell and take ownership of all
> the cells to the right that exist so far, update the colspan properly and
> insert it in the QMap as such QMap<nextColWhenUpdateIsNeeded, cell>.
> 	->Similarly for rowspan = 0, except that since the rows are sequential
> whenever we find a row with such rowspan it has to be from that point on,
> it's inserted similarly: QMap< nextRowWhenUpdateIsNeeded, cell>.
> 	->On the next pass we check whether we're in an unhandled column for any
> of the colspan=0 cells, if so, update properly (update its colspan and mark
> as used the cell). We check the same for rowspans and store the column in
> which no other cell should be placed.
> 	->Process cell as usual, except if we're in a column in which no cell
> should be inserted then advance to the next column.
> The only corner case for both colspan and rowspan cells happens when it's a
> new column in which we are dealing, that is we are at a position in which
> no column has been inserted so far. Of course it is properly handled
> inserting a new column.
>

OK.

> So the algorithm seems to work quite well actually, except for one problem,
> for rowspan > 2 for some reason it stops growing the cell even though we
> set the rowspan properly! (I believe but my believings have been proven
> wrong in the past ;).

errm I'm getting Qt assert failures (index out of range) in QVector's [] 
operator with all your rowspan testcases, so some logic is indeed wrong.

it sounds like this could all stem from:

+    ensureRows( cRow + (rSpan)? rSpan : 1 );

? surely, you meant:

+    ensureRows( cRow + (rSpan? rSpan : 1) );

hmm, no, doesn't seem to be enough to fix it.
anyway, some comments on the patch so far:

- the html/html_tableimpl.cpp bit about changing the lowest allowed value for 
the span attribute of the col element's :

@@ -964,7 +964,7 @@
     {
     case ATTR_SPAN:
         _span = attr->val() ? attr->val()->toInt() : 1;
-        if (_span < 1) _span = 1;
+        if (_span < 0) _span = 1;

looks wrong actually... the HTMl 4.01 specification mandates that this value
be > 0

- could you setup a boolean flag for when no zero rowspan/colspan has been 
encountered yet, and then skip entirely the map lookup logic and effColToCol 
calculation as an optimization?

- you should clear your two maps on recalculation (up in recalcCells() 
probably)

- this does not cover exactly the 4.01 specification for colspan,
where the spanning of colspan=0 cells should be limited to the current 
colgroup.
Figuring the extent of the current colgroup is not really straightforward 
however, as colgroup (and col) are sort of descriptive tags that have no real 
rendered  counterpart.

Yet we have (zero dimensions) render objects for them in the render tree.

e.g:
<table>
  <colgroup span=2></colgroup>
  <colgroup span=3></colgroup>
  <colgroup>
     <col span=2>
     <col>
  </colgroup>
</table>

defining 3 colgroups spanning 8 logical cells in total,
will give you a render tree as such:

    RenderTable: <table>
      RenderTableCol: <colgroup>
      RenderTableCol: <colgroup>
      RenderTableCol: <colgroup>
        RenderTableCol:<col>
        RenderTableCol:<col>


See for instance RenderTable::colElement for a method that's walking this 
structure.


A full table matching that colgroup definition would be for instance:
<table>
  <colgroup span=2></colgroup>
  <colgroup span=3></colgroup>
  <colgroup>
     <col span=2>
     <col>
  </colgroup>

  <tr>
    <td colspan=2>1st colgroup</td>
    <td>2nd</td><td>col</td><td>group</td>
    <td colspan=3>3rd colgroup</td>
  </tr>

</table>

> 	So we're this time _really_ nearly there. I'm attaching you the bunch of
> test cases I've covered, and passed, so far (the previous ones had some
> misspellings and I didn't realize till after sending the previous mail so
> it lead me to the wrong conclusion that the previous algorithm worked, now
> I believe they're actually right), also the test in
> http://bugsfiles.kde.org/attachment.cgi?id=8334 is passed too except for
> the fact that rowspan doesn't seem to want to grow greater than 2.

> 	I'll let you know if I could solve the rowspan > 2 problem, if so then the
> patch wouldn't have any known problems, and would be waiting just for the
> regression test to be commited.

Thanks,
Germain