Bug 26483

Summary: select-all, copy, paste of specialAncestorElements (e.g. pre, h1, etc) nests the element inside itself
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: HTML EditingAssignee: Ojan Vafai <ojan>
Status: RESOLVED WORKSFORME    
Severity: Normal CC: adele, darin, dglazkov, enrica, eric, justin.garcia, mitz, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Bug Depends on: 34564, 44220, 65804, 66396, 66416, 66420, 66440, 66443, 66448    
Bug Blocks:    
Attachments:
Description Flags
Don't treat editable elements as special ancestor blocks.
none
Reduced HTML sample to reproduce
none
not comlete but fixes the bug in many cases
none
work in progress none

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.