Bug 26483 - select-all, copy, paste of specialAncestorElements (e.g. pre, h1, etc) nests the element inside itself
Summary: select-all, copy, paste of specialAncestorElements (e.g. pre, h1, etc) nests ...
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
: 24009 65591 (view as bug list)
Depends on: 34564 44220 65804 66396 66416 66420 66440 66443 66448
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-17 10:36 PDT by Ojan Vafai
Modified: 2011-11-25 16:37 PST (History)
10 users (show)

See Also:


Attachments
Don't treat editable elements as special ancestor blocks. (6.64 KB, patch)
2009-06-17 11:01 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Reduced HTML sample to reproduce (1.31 KB, text/html)
2011-05-31 03:59 PDT, Garry Yao
no flags Details
not comlete but fixes the bug in many cases (17.63 KB, patch)
2011-08-03 14:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress (13.39 KB, patch)
2011-08-16 20:00 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2009-06-17 10:36:12 PDT
<pre contentEditable id=bar>foo</pre>

If you select all, then copy/paste the above, you get:
<pre contentEditable id=bar><span ...><pre contentEditable id=bar>foo</pre></span></pre>

Which is clearly wrong. :) You get the same for headers and a few other tags.
Comment 1 Ojan Vafai 2009-06-17 11:01:10 PDT
Created attachment 31423 [details]
Don't treat editable elements as special ancestor blocks.

 7 files changed, 88 insertions(+), 10 deletions(-)
Comment 2 Ojan Vafai 2009-07-06 18:07:01 PDT
Actually, I think this is the wrong change. There's a more general bug that should be fixed and then this would go away. 

<div contentEditable>
  a
  <pre id=foo>bar</pre>
  b
</div>

Select "bar". Then copy and paste over it. You get 

<div contentEditable>
  a
  <pre id="foo"><span class="Apple-style-span" style="font-family: verdana; white-space: normal; "><pre id="foo">bar</pre></span></pre>
  b
</div>

Which is also wrong. The contents should be unmodified if you copy and then paste over what you just copied.
Comment 3 Ojan Vafai 2009-07-06 18:07:33 PDT
Comment on attachment 31423 [details]
Don't treat editable elements as special ancestor blocks.

Doesn't fix the general case.
Comment 4 Ojan Vafai 2009-08-01 09:04:35 PDT
Comment on attachment 31423 [details]
Don't treat editable elements as special ancestor blocks.

Removing r+ to get out of approved patches queue.
Comment 5 Ryosuke Niwa 2009-08-14 11:32:10 PDT
I haven't looked into the bug, but my intuition is that copying special ancestor block is fine.  It's just that we need to delete them when we are pasting.  i.e. we shouldn't be pasting pre/h2 when the same element wraps the selection.
Comment 6 Ryosuke Niwa 2010-03-21 01:47:46 PDT
+tony since this bug seems to be related to the bug 26937 and the bug 34564.
Comment 7 Garry Yao 2011-05-31 03:59:37 PDT
Created attachment 95414 [details]
Reduced HTML sample to reproduce
Comment 8 Garry Yao 2011-05-31 04:02:23 PDT
Comment on attachment 95414 [details]
Reduced HTML sample to reproduce

Sorry, head to the wrong ticket.
Comment 9 Ryosuke Niwa 2011-08-03 11:55:39 PDT
*** Bug 65591 has been marked as a duplicate of this bug. ***
Comment 10 Ryosuke Niwa 2011-08-03 14:08:42 PDT
Created attachment 102823 [details]
not comlete but fixes the bug in many cases
Comment 11 WebKit Review Bot 2011-08-03 14:50:26 PDT
Comment on attachment 102823 [details]
not comlete but fixes the bug in many cases

Attachment 102823 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306097

New failing tests:
editing/pasteboard/paste-text-011.html
editing/pasteboard/paste-pre-001.html
editing/pasteboard/paste-pre-002.html
Comment 12 Ryosuke Niwa 2011-08-03 14:55:42 PDT
(In reply to comment #11)
> (From update of attachment 102823 [details])
> Attachment 102823 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9306097
> 
> New failing tests:
> editing/pasteboard/paste-text-011.html
> editing/pasteboard/paste-pre-001.html
> editing/pasteboard/paste-pre-002.html

These are failing because they need rebaselines.
Comment 13 Ryosuke Niwa 2011-08-03 14:56:14 PDT
(In reply to comment #12)
> These are failing because they need rebaselines.

Chromium Windows/Linux specific rebaselines.
Comment 14 Enrica Casucci 2011-08-03 16:32:18 PDT
Comment on attachment 102823 [details]
not comlete but fixes the bug in many cases

The change looks good to me
Comment 15 Ryosuke Niwa 2011-08-03 16:36:53 PDT
(In reply to comment #14)
> (From update of attachment 102823 [details])
> The change looks good to me

Thanks for the review!
Comment 16 Ryosuke Niwa 2011-08-03 16:42:30 PDT
Committed r92330: <http://trac.webkit.org/changeset/92330>
Comment 17 Ryosuke Niwa 2011-08-03 16:42:59 PDT
I'll really appreciate if you could also review my patch for https://bugs.webkit.org/show_bug.cgi?id=34564.  The patch will eliminate the wrapping span in most cases and pave the way to get rid of Apple style span.
Comment 18 Ryosuke Niwa 2011-08-05 18:38:02 PDT
Oops, I realized that this patch caused a bunch of regression while I was working on the bug 34564 :(

e.g. It will ignore styles set on block elements such as div. In the following example, if you copy "world" into "hello", the text is no longer aligned to the right.
http://simple-rte.rniwa.com/?editor=%3Cp%20style%3D%22text-align%3A%20right%3B%22%3E%3Cbr%3E%3C/p%3E%0A%3Cp%3Eworld%3C/p%3E&designmode=false&script=document.getElementById%28%27editor%27%29.focus%28%29%3B

I'm going to rollout the patch, and I think about how to fix it properly.
Comment 19 Ryosuke Niwa 2011-08-05 18:41:44 PDT
Basically, we need a function like below to make sure the element we're splitting doesn't have attributes to preserve.

static bool hasIdenticalWrapper(Node* startNode, HTMLElement* element)
{
    if (!startNode)
        return false;

    for (Node* wrapper = startNode; wrapper; wrapper = wrapper->firstChild() == wrapper->lastChild() ? wrapper->firstChild() : 0) {
        if (!wrapper->hasTagName(element->tagQName()) || !wrapper->isHTMLElement())
            continue;

        NamedNodeMap* attributesOnElement = element->attributeMap();
        if (!attributesOnElement)
            return true;

        bool checkStyle = false;
        unsigned i;
        for (i = 0; i < attributesOnElement->length(); i++) {
            Attribute* attribute = attributesOnElement->attributeItem(i);
            if (attribute->name() == styleAttr) {
                checkStyle = true;
                continue;
            }
            if (attribute->value() != element->getAttribute(attribute->name()))
                break;
        }
        if (i >= attributesOnElement->length()) {
            if (checkStyle) {
                RefPtr<CSSMutableStyleDeclaration> styleOfWrapper = static_cast<HTMLElement*>(wrapper)->inlineStyleDecl();
                RefPtr<CSSMutableStyleDeclaration> styleOfElement = element->getInlineStyleDecl();
                if (styleOfWrapper && styleOfWrapper->length()) {
                    if (!styleOfElement || styleOfElement->length())
                        continue;
                    styleOfWrapper = styleOfWrapper->copy();
                    styleOfElement->diff(styleOfWrapper.get());
                    if (styleOfWrapper->length())
                        continue;
                }
            }
            return true;
        }
    }

    return false;
}
Comment 20 Ryosuke Niwa 2011-08-05 18:47:31 PDT
Now that I think about it more carefully, the trick I thought I can use to fix this bug was based on a false assumption, and this bug indeed depends on the bug 34564.
Comment 21 Ryosuke Niwa 2011-08-16 19:56:34 PDT
*** Bug 24009 has been marked as a duplicate of this bug. ***
Comment 22 Ryosuke Niwa 2011-08-16 20:00:52 PDT
Created attachment 104141 [details]
work in progress

Now that the bug 34564 has been fixed, I can tackle this bug again.   I still have to add logic to preserve styles of removed block elements, and about 15 tests still fail but this is a good start.
Comment 23 Eric Seidel (no email) 2011-09-06 15:29:36 PDT
Comment on attachment 102823 [details]
not comlete but fixes the bug in many cases

Cleared Enrica Casucci's review+ from obsolete attachment 102823 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 24 Ryosuke Niwa 2011-11-25 16:37:22 PST
I believe this bug has been fixed by my other patches.