187955 - tables: "create table from selection" gives incorrect result

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Scott McAuley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212

In Composer (also in Mail/News Compose), I tried to create tables from delimited
text and didn't get the right result. With L lines of F fields, instead of
getting an LxF table I see a Lx1 table tihe embedded sub-tables for fields F2, etc.

Reproducible: Always

Steps to Reproduce:
1. In Composer or Mail/Compose, enter three text lines "row,column" and
"r1c1,r1c2" and "r2c1,r2c2"
2. Highlight the lines and choose "Table-Create table from selection"
3. Get wrong result -- 3 rows / 1 column rather than 3 rows / 2 columns.

Actual Results:  
Composer shows 3 rows / 1 column rather than 3 rows / 2 columns.

Expected Results:  
3x2 table.

Comment 1

[Kathleen :Brade]

What options did you check in the dialog?
Did you choose space or comma or other?
Did you choose to delete the delimiting character or leave it?

Comment 2

[Scott McAuley]

1. I chose comma
2. I chose to delete
==
Out of curiosity, I just tried 4 alternatives (comma/delete, comma/keep,
space,delete, space/keep) in message composer. All give the same (wrong)
displayed result. When saved as a draft, all 4 tables display correctly, but
when I tried "Edit as new" on the template I got the inoorrect display again.

Comment 3

[Kathleen :Brade]

confirming; we're generating this:

<table border="1" width="100%" cellpadding="2" cellspacing="2">
  <tr>
    <td>row<td>column</td>
    </td>
  </tr>
  <tr>
    <td>r1c1<td>r1c2</td>
    </td>
  </tr>
  <tr>
    <td>r2c1<td>r2c2</td>
    </td>
  </tr>
</table>

This is a serious regression.

Comment 4

[rnorberg]

The table FIXED itself while I was looking at it?!!!!
I was writing a simple testcase in the composer.  I recreated the problem.  Then
while going back and forth between composer tabs 'normal' and 'html source' my
bad html source 'fixed itself'!!

copy this sample text into the composer, select all, then convert to table:
row1col1,row1col2,row1col3,row1col4,
row2col1,row2col2,row2col3,row2col4,

first produced :
<table border="1" width="100%" cellpadding="2" cellspacing="2">
  <tr>
    <td>row1col1,<td>row1col2,<td>row1col3,<td>row1col4,<td><br>
    </td>
    </td>
    </td>
    </td>
    </td>
  </tr>
  <tr>
    <td>row2col1,<td>row2col2,<td>row2col3,<td>row2col4,<td><br>
    </td>
    </td>
    </td>
    </td>
    </td>
  </tr>
</table>

then automatically changed to :
<table border="1" width="100%" cellpadding="2" cellspacing="2">
  <tbody>
    <tr>
      <td>row1col1 </td>
      <td>row1col2 </td>
      <td>row1col3 </td>
      <td>row1col4</td>
    </tr>
    <tr>
      <td>row2col1 </td>
      <td>row2col2 </td>
      <td>row2col3 </td>
      <td>row2col4</td>
    </tr>
  </tbody>
</table>

Comment 5

[Kathleen :Brade]

This worked fine in a build from 11/11/2002 (OS9)
Checking 12/27 build on OSX it is broken there.
Checking 12/4 build on OSX it is broken there.
Checking 11/21 build on OSX it works there.
Checking 11/27 build on OSX it is broken there.

It seems that it broke sometime between 11/21 and 11/27.  I haven't had a chance
to look at checkins in that range nor narrow it down further.

Comment 6

[Kathleen :Brade]

*** Bug 188895 has been marked as a duplicate of this bug. ***

Comment 7

[Kathleen :Brade]

with OSX mozilla builds I've narrowed the window down between 11/21/02 8amPST
and 11/22/02 3amPST.

Joe changed some core editor things during that timeframe (CFHTML paste landed
then).  jst and bratell landed some serializer changes... nothing obvious at a
glance :-(

Comment 8

[Akkana Peck]

Data point: A 2002-11-21-21-trunk linux build shows the problem, just as
described (nested td tags, /td in the wrong place).

Comment 9

[Kathleen :Brade]

I think this was unknowingly regressed by Joe.
It looks like the JS that generates the table is broken because it is trying to
insert something like this:
 <table> <tr> <td> cellOne <td> cellTwo </tr> </table>

Probably the regression came from one of Joe's checkins.

We should have a separate bug about the JS that generates the wrong html string:
http://lxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdConvertToTable.js#297

-->joe

Comment 10

[Akkana Peck]

I'm not seeing anything wrong with that generated html.  Closing td, th and tr
tags are optional: see http://www.w3.org/TR/html4/struct/tables.html (in fact,
their examples on that page omit the close tags).

Looks to me like the html-generating JS is right but the table-insert code is
broken in that it doesn't close the current td/th before opening the next one.

Comment 11

[Joe Francis]

I have a patch for this which I have to port over from angelon branch.  

Comment 12

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 13

[Joe Francis]

Attachment: patch to content/html/document/src/nsHTMLFragmentContnetSink.cpp

Comment 14

[Joe Francis]

Attachment: patch to editor/libeditor/html

above two patches taken together fix this bug.

Comment 15

[Kathleen :Brade]

Comment on attachment 112705 [details] [diff] [review]
patch to content/html/document/src/nsHTMLFragmentContnetSink.cpp

r=brade

Comment 16

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

Comment on attachment 112706 [details] [diff] [review]
patch to editor/libeditor/html

r=glazman if you remove the // printf(...)

Comment 17

[kinmoz]

Comment on attachment 112705 [details] [diff] [review]
patch to content/html/document/src/nsHTMLFragmentContnetSink.cpp

sr=[e-mail]

These changes look fine to me. Is the editor the only consumer of the
HTMLFragmentContentSink? I just want to make sure there isn't any
negative/unexpected impact to others that result from moving these
Open/CloseContainer calls up a level.

Comment 18

[kinmoz]

Comment on attachment 112706 [details] [diff] [review]
patch to editor/libeditor/html

sr=[e-mail] with the following addressed:


==== CreateTagStack() can actually fail with an out of memory error after
having allocated some strings. Should it be modified 

to free any allocated strings in this case, or should the following code call
FreeTagStackStrings() when an error occurs to 

make sure it free's any allocated strings?


+  // get the tagstack for the context
+  res = CreateTagStack(tagStack, contextLeaf);
+  NS_ENSURE_SUCCESS(res, res);



==== If for some reason we need to bail early, we still need to free the tag
stack contents. I would just move the FreeTagStackStrings() call right after
the ParseFragment() call, and before the error checks.


   // create fragment for pasted html
-  res = ParseFragment(aInputString, outFragNode);
+  res = ParseFragment(aInputString, tagStack, outFragNode);
   NS_ENSURE_SUCCESS(res, res);
   NS_ENSURE_TRUE(*outFragNode, NS_ERROR_FAILURE);

   RemoveBodyAndHead(*outFragNode);
-
+  FreeTagStackStrings(tagStack);


==== Do we want to move these declarations under  the |if| block that uses
them?


+    nsAutoString tagName;
+    PRUnichar* name = nsnull;


==== Can we get rid of the extra GetParentNode() call by collapsing this:


+    node->GetNodeType(&nodeType);
+    if (nsIDOMNode::ELEMENT_NODE == nodeType)
+    {
+      node->GetNodeName(tagName);
+      // XXX Wish we didn't have to allocate here
+      name = ToNewUnicode(tagName);
+      if (name) 
+      {
+	 aTagStack.AppendElement(name);
+	 // printf("%s\n",NS_LossyConvertUCS2toASCII(tagName).get());
+	 res = temp->GetParentNode(getter_AddRefs(node));
+	 NS_ENSURE_SUCCESS(res, res);
+      }
+      else 
+      {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }
+    }
+    else 
+    {
+      res = temp->GetParentNode(getter_AddRefs(node));
+      NS_ENSURE_SUCCESS(res, res);
+    }


to this?


+    node->GetNodeType(&nodeType);
+    if (nsIDOMNode::ELEMENT_NODE == nodeType)
+    {
+      node->GetNodeName(tagName);
+      // XXX Wish we didn't have to allocate here
+      name = ToNewUnicode(tagName);
+      if (!name) 
+	 return NS_ERROR_OUT_OF_MEMORY;
+
+      aTagStack.AppendElement(name);
+      // printf("%s\n",NS_LossyConvertUCS2toASCII(tagName).get());
+    }
+
+    res = temp->GetParentNode(getter_AddRefs(node));
+    NS_ENSURE_SUCCESS(res, res);




==== Lose the extra spaces before FreeTagStackStrings:


+  nsresult CreateTagStack(nsVoidArray &aTagStack, nsIDOMNode *aNode);
+  void 	FreeTagStackStrings(nsVoidArray &tagStack);
   nsresult GetListAndTableParents( PRBool aEnd,

Comment 19

[Michael Buckland]

Discussed in edt bug triage.  Plussing.

Comment 20

[Sébastien Desvignes]

From comment #4 :
> The table FIXED itself while I was looking at it?!!!!

From what I've experimented, the auto-fixing triggered when you do any changes
in the source view.
So, as a workaround, just go to source view, add a space somewhere, and go back
to normal view...

Comment 21

[Joe Francis]

fix landed on trunk with revisions from review.

Comment 22

[Kathleen :Brade]

*** Bug 201335 has been marked as a duplicate of this bug. ***

Comment 23

[rubydoo123]

tested using the build from [2003051308] on winXP and this as expected. I
entered the 3 rows and 2 columns of data, space separated, and selected to
create table based on selection.

verified fixed