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

Tue Aug 14 16:58:37 PDT 2012

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

--- Comment #47 from Alice Cheng <alice_cheng at apple.com>  2012-08-14 16:59:02 PST ---
(From update of attachment 158440)
View in context: https://bugs.webkit.org/attachment.cgi?id=158440&action=review

Thanks Darin for the review! I have some questions.

>>> Source/WebCore/editing/AppendNodeCommand.cpp:46
>>> +    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?
> 
> I said isEditable() above where I meant isContentEditable().

isContentEditable will call rendererIsEditable, however <head> does not have a renderer. Is it okay to keep the string compare?

Will fix it into a helper function with a "why" comment.

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

Will fix it with RefPtr

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:430
>> +            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.

I did a clone because undoApply of AppendNodeCommand will simply remove the appended node

I did not remove because it is inside selection, and the rest of DeleteSelectionCommand will remove it. Is it okay to reuse the existing DeleteSelectionCommand to remove? Maybe I do not understand it, i will consult Enrica.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:443
>> +    // Move styling elements to prevent style loss
> 
> Sentence style comment should have a period in it.

will fix

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