Bug 26483

Created attachment 31423 [details] Don't treat editable elements as special ancestor blocks. 7 files changed, 88 insertions(+), 10 deletions(-)

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 on attachment 31423 [details] Don't treat editable elements as special ancestor blocks. Doesn't fix the general case.

Comment on attachment 31423 [details] Don't treat editable elements as special ancestor blocks. Removing r+ to get out of approved patches queue.

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.

+tony since this bug seems to be related to the bug 26937 and the bug 34564.

Created attachment 95414 [details] Reduced HTML sample to reproduce

Comment on attachment 95414 [details] Reduced HTML sample to reproduce Sorry, head to the wrong ticket.

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

Created attachment 102823 [details] not comlete but fixes the bug in many cases

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

(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.

(In reply to comment #12) > These are failing because they need rebaselines. Chromium Windows/Linux specific rebaselines.

Comment on attachment 102823 [details] not comlete but fixes the bug in many cases The change looks good to me

(In reply to comment #14) > (From update of attachment 102823 [details]) > The change looks good to me Thanks for the review!

Committed r92330: <http://trac.webkit.org/changeset/92330>

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.

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.

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; }

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.

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

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 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.

I believe this bug has been fixed by my other patches.