236546 - newlines not added when copying table into clipboard with CTRL+mouse

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect)

Description

[Romano]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040224 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040224 Firefox/0.8.0+

when copying the table with CTRL holding down, and then pasting it into some
other program, the new lines are lost (replaced with tabs, several-row-table
changes into one-row of data only)

opera's doing it right way (in case my english is not understandable ;)

Reproducible: Always
Steps to Reproduce:
1. select several table rows with mouse, holding CTRL key
2. then copy it into clipboard with CTRL+C
3. paste it (CTRL+V) into some text editor

Actual Results:  
</TR>s are replaced with tabs (\t)


Expected Results:  
</TR>s should be replaced by new lines (\t)

it's more logical and helps to paste table content into another program,
spreadsheet for example (now your table changes into bunch or records in one row
only, and that doesn't make sense)


checked this behaviour on windows only

Comment 1

[Romano]

sorry a typo (should be):

Expected Results:  
</TR>s should be replaced by new lines (\n)

Comment 2

[Jesse Ruderman]

-> Core, DOM-to-text conversion (because this also happens in the Mozilla suite)

Comment 3

[Jesse Ruderman]

I don't understand your comment that "Opera gets this right".  If you hold the
Ctrl key to select table cells, you're using a feature that Opera 7.50 does not
have.  If you select without Ctrl, Mozilla and Opera both have reasonable behavior.

However, I agree with you that selecting table cells with Ctrl and pasting
should add line breaks between cells that aren't in the same row.  I've
encountered this problem myself.

Comment 4

[Kathleen :Brade]

Is this only a problem on certain web pages?  What page can I reproduce this on?
 Any?

Maybe the fix in bug 94176 wasn't quite enough for certain pages?

Comment 5

[Romano]

> Is this only a problem on certain web pages?  What page can I reproduce this on?
 Any?

I think that you can reproduce it anywhere. Please check
http://sweb.cz/kokot/bug.htm - a sample page with more detailed description (and
a picture too! :)

> Maybe the fix in bug 94176 wasn't quite enough for certain pages?

Although this bug is quite similiar (same maybe), it's a bug in a new feature
added into firefox so i don't think that it has something to do with 94176.

Comment 6

[(not currently active) Ted Mielczarek]

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

Comment 7

[Jeff Schiller]

Attachment: Simple testcase with instructions

Comment 8

[Jeff Schiller]

Seems like an "easy" fix would be to put in newlines every time a </tr> is detected, but the problem with that is what do you do when the selected cells are non-contiguous?  Is it good enough?

Comment 9

[Jeff Schiller]

Attachment: Proposed patch

Here's a patch, might be kinda hacky.  Who do I need to review it?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 259431 [details] [diff] [review]
Proposed patch

jst:

can you review this?

Comment 11

[Jeff Schiller]

Sorry, who is jst?

Comment 12

[Jesse Ruderman]

Johnny Stenback aka jst is a DOM module owner (http://www.mozilla.org/owners.html) and he's fixed over 900 bugs.  Since the patch is in mozilla/content (aka DOM), and he wrote large chunks of nsDocumentEncoder.cpp (see CVS blame), I think he's a reasonable choice for reviewer :)

Comment 13

[Jesse Ruderman]

> aOutputString.Append(NS_ConvertASCIItoUTF16("</tr>"));

NS_LITERAL_STRING would avoid runtime length measurement and conversion.  Maybe NS_NAMED_LITERAL_STRING since the string "</tr>" is used twice.

Comment 14

[Jeff Schiller]

Attachment: Updated with NS_NAMED_LITERAL_STRING per Jesse

Updated with NS_NAMED_LITERAL_STRING per Jesse

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 259655 [details] [diff] [review]
Updated with NS_NAMED_LITERAL_STRING per Jesse

     rv = mSelection->GetRangeCount(&count);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    nsCOMPtr<nsIDOMNode> node, prevNode;
+    nsCOMPtr<nsIDOMElement> element;
+    nsCOMPtr<nsIContent> content;
+    PRBool bTableCellsFound = PR_FALSE;
+    NS_NAMED_LITERAL_STRING(_begin_tr, "<tr>");
+    NS_NAMED_LITERAL_STRING(_end_tr, "</tr>");
+
     for (i = 0; i < count; i++) {
       mSelection->GetRangeAt(i, getter_AddRefs(range));
 
+      // Bug 236546: newlines not added when copying table cells into clipboard
+      // Each selected cell shows up as a range containing a row with a single cell
+      // get the row, compare it to previous row and emit </tr><tr> as needed
+      range->GetStartContainer(getter_AddRefs(node));
+      NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
+      element = do_QueryInterface(node);
+      if (element) {
+        content = do_QueryInterface(element);
+        if (content) {

No need to QI to nsIDOMElement first and then to nsIContent, you can simply QI node directly to nsIContent. And do_QueryInterface() is null safe, so you don't need to check whether node came back as none-null, unless there's a real reason to.

+          nsIAtom *name = content->Tag();
+          if (name == nsGkAtoms::tr) { 

This won't actually always do what we want here. In the case where someone has an XML document with a tag in it that happens to be named "tr" we don't necessarily want to treat it as a table cell. You should add a content->IsNodeOfType(nsINode::eHTML) check here, but check that after you check the atom (i.e. make it "if (name == nsGkAtoms::tr && content->IsNodeOfType(nsINode::eHTML)").

r- for now, but I'll gladly r+sr an updated fix!

Comment 16

[Jeff Schiller]

Attachment: Updated per jst's review

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 259757 [details] [diff] [review]
Updated per jst's review

+      range->GetStartContainer(getter_AddRefs(node));
+      NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
+      content = do_QueryInterface(node);
+      nsIAtom *name = content->Tag();

You'll still need a null check here (you just didn't need two of them), or you'll crash if a DOM node makes it in here that doesn't implement nsIContent. r+sr=jst with that added.

Comment 18

[Jeff Schiller]

Attachment: Final patch (hopefully)

Comment 19

[Jeff Schiller]

Ok, final patch I hope.  Not sure how to get this resolved/fixed/applied to trunk.  Do I need to be marked as Assignee also?  Thanks.

Comment 20

[(not currently active) Ted Mielczarek]

Jeff: you can reassign a bug to yourself if you're working on it.  Also, in this situation, we generally put [checkin needed] up in the status whiteboard to indicate that you're looking for someone to checkin this patch.  You might also ask on irc.mozilla.org #developers.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a time, I'll check in it soon.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in, thank you for your work.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

looks good in 2007032804 build on Win.

-> v.

Comment 24

[[e-mail]]

Umm... this causes more bugs than it fixes? Not only does it affect non-HTML encoders, but it stomps on the output string which is owned by the serializer.

Comment 25

[[e-mail]]

Oh, and it also assumes all disjoint selections are table cells.

Comment 26

[Jeff Schiller]

I still say this is the right fix.  Without this fix, the nsDocumentEncoder::EncodeToString() sets the string to 

<td>B2</td><td>B3</td><td>B4</td><td>C2</td><td>C3</td><td>C4</td>

while with this fix, the output string is:

<tr><td>B2</td><td>B3</td><td>B4</td></tr><tr><td>C2</td><td>C3</td><td>C4</td></tr>

The problem seems to be in how this string is converted into plain text for various purposes, which we can try to fix with bug 380364.  If that's not acceptable, then yes - let's back this out until we can fix it.

> it also assumes all disjoint selections are table cells.

This fix has:

  if (name == nsGkAtoms::tr && content->IsNodeOfType(nsINode::eHTML)) { ... }

Therefore it is only operating on selections of table cells, nothing else.

Comment 27

[[e-mail]]

(In reply to comment #26)
> The problem seems to be in how this string is converted into plain text
Except that when used in plain text mode this string is already plain text.

>>it also assumes all disjoint selections are table cells.
>This fix has:
>  if (name == nsGkAtoms::tr && content->IsNodeOfType(nsINode::eHTML)) { ... }
>Therefore it is only operating on selections of table cells, nothing else.
But are you justified in assuming that all the ranges will be table cells?

Comment 28

[Jeff Schiller]

> But are you justified in assuming that all the ranges will be table cells?

This is the way it works now, as far as I can tell.  Disjoint selections are limited to table cells for the present.  Please help me understand if I'm missing something, thanks.

Comment 29

[Jesse Ruderman]

Another easy way to get multiple selections is to drag-select some text, and then hold Cmd and drag-select some more text.  This feature was added in bug 73373.

Comment 30

[Jeff Schiller]

Thanks Jesse - however in playing with this, I cannot seem to get table cells selected along with any other type of element.  For instance, multi-select some paragraph text works fine via ctrl, but if i then ctrl+click on a table cell the text is de-selected.  I think this points to the fact that multiple selection ranges that contain table cells can only table cells, but please someone correct me if I'm mistaken.

Comment 31

[Jesse Ruderman]

If you do it the other way around (Cmd+click a table cell, then Cmd+drag-select some text) you can get a mix of both types of selections.

Comment 32

[Jeff Schiller]

Thanks, Jesse.  Fix for Bug 380364 seems to work for multi-select ranges of text and table cells...