204589 - new rows inserted in a table do not inherit the class attribute from the previous row

Status: RESOLVED INVALID

(Core :: DOM: Editor, defect)

Description

[Pascal Chevrel:pascalc (PTO until April 26)]

build 2003043008 winXP

1 edit a table in composer with some cells having a class attribute
(like <tr><td></td><td class="foo"></td></tr>)
2 insert a new row from context menu (table insert/row blelow)

expected result : a new row with cells inheriting the class attribute from the
previous line
actual result : a new row without class attributes

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Very interesting, indeed.

Comment 2

[Laurent Jouanneau]

I take it.

Comment 3

[Laurent Jouanneau]

Attachment: Patch to add a new method insertTableRowExt

This patch add a new method on nsITableEditor

   void insertTableRowExt(in long  aNumber, in boolean aAfter,
                          in boolean aSameStyleAndType,
                          in boolean aSameColSpan,
                          in boolean aPreserveRowSpan);

So we can says if the new row will add same style, same colspan on its cell, and if it preserves rowspan.

And this method is called when we hit the tab key in the editor, instead of insertTableRow.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 353688 [details] [diff] [review]
Patch to add a new method insertTableRowExt


>+  /**
>+    * Insert relative to the selected cell or the 
>+    *  cell enclosing the selection anchor
>+    * The selection is collapsed and is left in the new cell
>+    *  at the same row,col location as the original anchor cell
>+    *
>+    * @param aNumber    Number of items to insert
>+    * @param aAfter     If TRUE, insert after the current row,
>+    *                     else insert before current row
>+    * @param aSameStyleAndType if TRUE, it generates same cell type (td or th)
>+    *                           and applies same style attributes (style, class, valign...)
>+    * @param aSameColSpan if TRUE, colspan are preserved
>+    * @param aPreserveRowSpan if TRUE, rowspan are preserved
>+    */
>+  void insertTableRowExt(in long  aNumber, in boolean aAfter,
>+                         in boolean aSameStyleAndType,
>+                         in boolean aSameColSpan,
>+                         in boolean aPreserveRowSpan);

Why not aPreserveStyleAndType, aPreserveColSpan and aPreserveRowSpan ?

>@@ -1414,17 +1414,17 @@ NS_IMETHODIMP nsHTMLEditor::TabInTable(P
>       return NS_OK;
>     }
>   } while (!iter->IsDone());
>   
>   if (!(*outHandled) && !inIsShift)
>   {
>     // if we havent handled it yet then we must have run off the end of
>     // the table.  Insert a new row.
>-    res = InsertTableRow(1, PR_TRUE);
>+    res = InsertTableRowExt(1, PR_TRUE, PR_TRUE, PR_TRUE, PR_TRUE);

Right, this is the only place where we want this thange for the time being.
Clients of this code like Thunderbird or Composer/BlueGriffon will decide
by themselves if they want to change their "Insert Row above/below" calls.

>+
>+  // The list of cell to "duplicate".
>+  nsVoidArray cellList;

"The list of cells"

>+            cellsInRow ++;

cellsInRow++; please with no whitespace


>+        if (!aPreserveRowSpan) {
>+          SetRowSpan(curCell, actualRowSpan);
>+        }
>+        else
>+          checkColSpan = PR_FALSE;

Please invert the two cases and base the test on aPreserveRowSpan.

>+          cellsInRow ++;

cellInRow++


>+          for (i=0; i < actualColSpan; i++) {
>+            cellList.AppendElement((void*)curCell.get());
>+          }

i = 0;

>+            cellPtr->GetAttribute(NS_LITERAL_STRING("valign"), attr);
>+            if (!attr.IsEmpty())
>+              newCell->SetAttribute(NS_LITERAL_STRING("valign"), attr);
>+            else
>+              // we remove the valign attribute added by CreateElementWithDefaults
>+              newCell->RemoveAttribute(NS_LITERAL_STRING("valign"));

Same as above.

>+            cellPtr->GetAttribute(NS_LITERAL_STRING("style"), attr);
>+            if (!attr.IsEmpty())
>+              newCell->SetAttribute(NS_LITERAL_STRING("style"), attr);
>+            else
>+              // we remove the style attribute added by CreateElementWithDefaults
>+              newCell->RemoveAttribute(NS_LITERAL_STRING("style"));

Same as above.

r=me provided the requested changes are made

Comment 5

[Laurent Jouanneau]

Attachment: version 2 of the patch

it adresses glazou's comments, and adds an improvement : attributes of the duplicated tr element were not copied on the new tr element. This is why I ask a new review :-)

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

NI myself to remind me of checking if this bug is still valid.