[Webkit-unassigned] [Bug 93643] Preserve styling elements in DeleteSelectionCommand

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93643


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #158440|review?                     |review-
               Flag|                            |




--- Comment #45 from Darin Adler <darin at apple.com>  2012-08-14 16:41:15 PST ---
(From update of attachment 158440)
View in context: https://bugs.webkit.org/attachment.cgi?id=158440&action=review

> Source/WebCore/editing/AppendNodeCommand.cpp:46
> -    ASSERT(m_parent->rendererIsEditable() || !m_parent->attached());
> +    ASSERT(m_parent->rendererIsEditable() || !m_parent->attached() || (m_parent->hasTagName(HTMLNames::headTag) && ((HTMLElement*)m_parent.get())->contentEditable() == "true"));

This logic is long enough that I think it should go into a helper function now, rather than being repeated three times.

The style of that cast is wrong, but you won’t need a typecast at all. Something more like this:

    ASSERT(m_parent->rendererIsEditable() || !m_parent->attached() || (m_parent->hasTagName(HTMLNames::headTag) && m_parent->isEditable()));

But also, you need a “why” comment. Why is the head element a special case?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:425
> +    if (!head || head->contentEditable() != "true")
> +        return;

The right way to do this is just:

    if (!head || !head->isContentEditable())
        return;

You should use the contentEditable function that returns a string.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:426
> +    for (Node* node = range->firstNode(); node && node != range->pastLastNode(); node = node->traverseNextNode()) {

It’s not safe to point to a node with a raw pointer while editing a document. Mutation events could run and cause the node to be deleted. So you need the node to be a RefPtr<Node>.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:430
> +            RefPtr<Node> clone = node->cloneNode(true);
> +            clone->setParentOrHostNode(0);
> +            appendNode(clone, head);

You should not have to clone the node. Instead you should do a removeNode followed by an appendNode to undoably move the node.

However, doing that also requires changing the way you walk from each node to the next. Before removing a node you need to call traverseNextSibling to advance to a node that won’t be removed.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:443
> +    // Move styling elements to prevent style loss

Sentence style comment should have a period in it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.