Closed Bug 1306532 Opened 8 years ago Closed 7 years ago

Copy/paste table in body [contenteditable] will be missed the original format

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Version:
47 Branch
Platform:
x86_64
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 - wontfix
firefox51 - wontfix
firefox52 + wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: 814425296, Assigned: wchen)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

2.png
(deleted), image/png
Details
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node. v2
(deleted), patch
masayuki
: review-
Details | Diff | Splinter Review
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node. v3
(deleted), patch
masayuki
: review+
Details | Diff | Splinter Review
Attached image 2.png (deleted) — 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

The test table Link: http://www.w3school.com.cn/tiy/t.asp?f=html_table_test
steps are as follows:
1# use mouse to select the right side table. then copy
2# update tab<body contenteditable="true"> in left side. then remove the code <table></table>,then submit code
3# paste 1#,you will see a text and a table.


Actual results:

copy table, it showed text & table after paste it.


Expected results:

copy table, it should be showed table after paste it.

BTW, this bug showed from version 47.0.x. Thanks for your efforts to fix it. It's a big trouble.
OS: Unspecified → Windows
Hardware: Unspecified → x86_64
Group: firefox-core-security
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22424f6eeb30b9bea2a2818178e72ef46871ebbc&tochange=542d4cda794bb670b1483e9151c34fb06b8ce8fb

William Chen — Bug 1247483 - Only replace nodes in nsHTMLEditor::ReplaceOrphanedStructure if all nodes in node list are descendants of replacement node. r=ehsan
Blocks: 1247483
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Component: Untriaged → Editor
Ever confirmed: true
Flags: needinfo?(wchen)
Keywords: regression
Product: Firefox → Core
Summary: copy table, then paste table in firebox will be missed the original format → Copy/paste table in body [contenteditable] will be missed the original format
Version: 49 Branch → 47 Branch
Depends on: 1277113
For the record, <table> lost after pasting it in contenteditable is bug 1250705.
Tracking 52+ for this regression.
tracking-firefox52: ? → +
As we shipped several releases with this issue, I don't think it is worth tracking for 50 and 51.
However, William, if you see a safe fix, we would be happy to take an uplift to aurora & beta.
status-firefox49: affected → wontfix
status-firefox50: affected → fix-optional
status-firefox51: affected → fix-optional
tracking-firefox50: ? → -
tracking-firefox51: ? → -
Priority: -- → P2
William, did you have a chance to look at this issue?
Definately a regression from bug 1247483. The fix to that bug was too restrictive (only allowing a replacement node when all the nodes in aNodeList are descendants). I reverted that change and took a slightly different approach to fixing bug 1247483 that doesn't cause the regression in this bug. The original buggy code only removed descendants of the replacement node from aNodeList up until the first non-descendant node. This patch removes all the descendants of the replacment node from aNodeList.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
Attachment #8821701 - Flags: review?(ehsan)
Comment on attachment 8821701 [details] [diff] [review]
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node.

Review of attachment 8821701 [details] [diff] [review]:
-----------------------------------------------------------------

Masayuki reviews editor code these days.
Attachment #8821701 - Flags: review?(ehsan) → review?(masayuki)
Comment on attachment 8821701 [details] [diff] [review]
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node.

Hmm, I need to read around here because it's difficult to understand what the method tries to do but unfortunately, I'm in vacation, I'll be back on 6th Jan. So, I'm not sure when I can review this, but I'll try to do it.

Anyway, could you add mochitest to test this (and perhaps, list case too)? Currently, we don't have much tests for editor. So, we should add automated tests when we fix bugs in editor as far as possible.
Added test
Attachment #8821701 - Attachment is obsolete: true
Attachment #8821701 - Flags: review?(masayuki)
Attachment #8825098 - Flags: review?(masayuki)
Comment on attachment 8825098 [details] [diff] [review]
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node.

Although, this is not a scope of this bug, we should rename some methods around this and document the purpose of each method in the header file because it's very unclear what each method does.

>diff --git a/editor/libeditor/HTMLEditorDataTransfer.cpp b/editor/libeditor/HTMLEditorDataTransfer.cpp
>   // If we found substructure, paste it instead of its descendants.
>-  // Only replace with the substructure if all the nodes in the list are
>-  // descendants.
>-  bool shouldReplaceNodes = true;
>+  // Postprocess list to remove any descendants of this node so that we don't
>+  // insert them twice.
>   for (uint32_t i = 0; i < aNodeArray.Length(); i++) {
>     uint32_t idx = aStartOrEnd == StartOrEnd::start ?
>       i : (aNodeArray.Length() - i - 1);
>     OwningNonNull<nsINode> endpoint = aNodeArray[idx];
>-    if (!EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
>-      shouldReplaceNodes = false;
>-      break;
>+    if (endpoint == replaceNode || EditorUtils::IsDescendantOf(endpoint, replaceNode)) {

nit: Too long, please break after ||. There are only 80 characters at most in each line.

>+      aNodeArray.RemoveElementAt(idx--);

I don't understand why you decrement |idx| here. |idx| is a local variable of the |for| block. So, it will be reset at next loop and not referred from outside of the |for| loop.

On the other hand, you remove some items from aNodeArray even if you scan it from its first item to the last one. So, I think you need to store removed item count and |idx| should be computed with it.

>     }

Hmm, and you killed the change to quite from the loop earlier. I'm afraid that causes too slow handling at pasting too big HTML fragment. If I understand correctly, cannot we quite the loop when we meet a node which is neither replaceNode nor a descendant of it? So, isn't this enough?

if (endpoint != replaceNode &&
    !EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
  return;
}
aNodeArray.RemoveElementAt(idx);


>   }
> 
>-  if (shouldReplaceNodes) {
>-    // Now replace the removed nodes with the structural parent
>-    aNodeArray.Clear();
>-    if (aStartOrEnd == StartOrEnd::end) {
>-      aNodeArray.AppendElement(*replaceNode);
>-    } else {
>-      aNodeArray.InsertElementAt(0, *replaceNode);
>-    }
>+  // Now replace the removed nodes with the structural parent
>+  if (aStartOrEnd == StartOrEnd::end) {
>+    aNodeArray.AppendElement(*replaceNode);
>+  } else {
>+    aNodeArray.InsertElementAt(0, *replaceNode);
>   }
> }
> 
> } // namespace mozilla

>diff --git a/editor/libeditor/tests/mochitest.ini b/editor/libeditor/tests/mochitest.ini
>+[test_bug1306532.html]

I think that you need to add |subsuite = clipboard| for this section because the test uses clipboard (bug 1270962).

>diff --git a/editor/libeditor/tests/test_bug1306532.html b/editor/libeditor/tests/test_bug1306532.html
>+  is(pasteContainer.querySelector("#headingone").textContent, "Month", "First heading should be 'Month'.");
>+  is(pasteContainer.querySelector("#headingtwo").textContent, "Savings", "Second heading should be 'Savings'.");
>+  is(pasteContainer.querySelector("#cellone").textContent, "January", "First cell should be 'January'.");
>+  is(pasteContainer.querySelector("#celltwo").textContent, "$100", "Second cell should be '$100'.");

Might be better to use innerHTML for the check, but up to you.
Attachment #8825098 - Flags: review?(masayuki) → review-
(In reply to Masayuki Nakano [:masayuki] from comment #10)
> Comment on attachment 8825098 [details] [diff] [review]
> Post process node array in ReplaceOrphanedStructure to remove all
> descendants of replacement node.
> 
> nit: Too long, please break after ||. There are only 80 characters at most
> in each line.
done
> I don't understand why you decrement |idx| here. |idx| is a local variable
> of the |for| block. So, it will be reset at next loop and not referred from
> outside of the |for| loop.
> 
> On the other hand, you remove some items from aNodeArray even if you scan it
> from its first item to the last one. So, I think you need to store removed
> item count and |idx| should be computed with it.
Oops, you're right. I meant to decrement |i| in order to adjust the index from removing the element.
> Hmm, and you killed the change to quite from the loop earlier. I'm afraid
> that causes too slow handling at pasting too big HTML fragment. If I
> understand correctly, cannot we quite the loop when we meet a node which is
> neither replaceNode nor a descendant of it? So, isn't this enough?
> 
> if (endpoint != replaceNode &&
>     !EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
>   return;
> }
> aNodeArray.RemoveElementAt(idx);
That code wouldn't work because we could end up removing descendant nodes from |aNodeArray| and early return without inserting the |replaceNode|. I don't think we can leave the loop early because |aNodeArray| may contain nodes that are descendants of |replaceNode| as well as nodes that are not descendants and we want to remove all the descendants. If we break early after seeing a non-descendant node, we may miss descendant nodes later in the array. If we break early we will regress bug 1247483.
> >diff --git a/editor/libeditor/tests/mochitest.ini b/editor/libeditor/tests/mochitest.ini
> >+[test_bug1306532.html]
> 
> I think that you need to add |subsuite = clipboard| for this section because
> the test uses clipboard (bug 1270962).
done
Attachment #8825098 - Attachment is obsolete: true
Attachment #8828522 - Flags: review?(masayuki)
(In reply to William Chen [:wchen] from comment #11)
> (In reply to Masayuki Nakano [:masayuki] from comment #10)
> > Hmm, and you killed the change to quite from the loop earlier. I'm afraid
> > that causes too slow handling at pasting too big HTML fragment. If I
> > understand correctly, cannot we quite the loop when we meet a node which is
> > neither replaceNode nor a descendant of it? So, isn't this enough?
> > 
> > if (endpoint != replaceNode &&
> >     !EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
> >   return;
> > }
> > aNodeArray.RemoveElementAt(idx);
> That code wouldn't work because we could end up removing descendant nodes
> from |aNodeArray| and early return without inserting the |replaceNode|. I
> don't think we can leave the loop early because |aNodeArray| may contain
> nodes that are descendants of |replaceNode| as well as nodes that are not
> descendants and we want to remove all the descendants. If we break early
> after seeing a non-descendant node, we may miss descendant nodes later in
> the array. If we break early we will regress bug 1247483.

Ah, right. Although, I still worry about the performance, but I don't have any better idea. So, go ahead with your patch for now.
Comment on attachment 8828522 [details] [diff] [review]
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node. v2

>   for (uint32_t i = 0; i < aNodeArray.Length(); i++) {
>     uint32_t idx = aStartOrEnd == StartOrEnd::start ?
>       i : (aNodeArray.Length() - i - 1);
>     OwningNonNull<nsINode> endpoint = aNodeArray[idx];
>-    if (!EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
>-      shouldReplaceNodes = false;
>-      break;
>+    if (endpoint == replaceNode ||
>+        EditorUtils::IsDescendantOf(endpoint, replaceNode)) {
>+      aNodeArray.RemoveElementAt(idx);
>+      i--;

I don't think that this is right fix. If removing it from aNodeArray from end to start, this is not necessary. And if i is 0, it becomes UINT32_MAX. So, the for loop will be broken (even if i won't be 0 here, why do you need to run it even when i is 0).

So, the simplest approach must be declare |uint32_t removedCount = 0;| outside the for loop and idx should be |i - removedCount| when |aStartOrEnd == StartOrEnd::start| is true.

Otherwise, looks good to me.
Attachment #8828522 - Flags: review?(masayuki) → review-
(In reply to Masayuki Nakano [:masayuki] from comment #13)
> Comment on attachment 8828522 [details] [diff] [review]
> Post process node array in ReplaceOrphanedStructure to remove all
> descendants of replacement node. v2
> 
> I don't think that this is right fix. If removing it from aNodeArray from
> end to start, this is not necessary. And if i is 0, it becomes UINT32_MAX.
> So, the for loop will be broken (even if i won't be 0 here, why do you need
> to run it even when i is 0).
We still need to adjust the index for removed nodes when removing elements in aNodeArray from end to start, otherwise the index will be wrong in the next iteration after removing.

In the case where i == 0, it decrements i to UINT32_MAX, but at the end of the iteration, i is incremented by 1 and per C++ spec i becomes 0 again (which is the correct index for the next iteration of the loop). We need to run the loop when i == 0 because we may need to remove the first element of the array.
> 
> So, the simplest approach must be declare |uint32_t removedCount = 0;|
> outside the for loop and idx should be |i - removedCount| when |aStartOrEnd
> == StartOrEnd::start| is true.
In the v3 patch, I've refactored to use a removedCount variable. It's functionally equivalent to the v2 patch. I'll leave it up to you to decide which one you think is more clear.
Attachment #8829663 - Flags: review?(masayuki)
Comment on attachment 8829663 [details] [diff] [review]
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node. v3

I like this better, thanks.
Attachment #8829663 - Flags: review?(masayuki) → review+
Pushed by wchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f782128e5d
Post process node array in ReplaceOrphanedStructure to remove all descendants of replacement node. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/48f782128e5d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Hi William, should we consider uplifting this fix to Beta52 and Aurora53? I noticed it while reviewing 52+ tracked bugs.
Flags: needinfo?(wchen)
(In reply to Ritu Kothari (:ritu) from comment #18)
> Hi William, should we consider uplifting this fix to Beta52 and Aurora53? I
> noticed it while reviewing 52+ tracked bugs.

No, I don't think we should uplift. There is a chance that the patch could cause performance issues so I'd rather see it ride the train.
Flags: needinfo?(wchen)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: