Bug 93643 – Preserve styling elements in DeleteSelectionCommand

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alice Cheng]

Style elements can appear inside editable content. To prevent accidental deletion, we provide a way for WebKit clients to move style elements from within the body to the head section.

[Alice Cheng]

<rdar://problem/12040676>

[Alice Cheng]

Created attachment 157530 [details]
patch

[WebKit Review Bot]

Attachment 157530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:51:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:54:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:65:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:69:  Extra space before ) in if  [whitespace/parens] [5]
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:74:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Cheng]

Created attachment 157535 [details]
patch

Fixed up styles

[WebKit Review Bot]

Attachment 157535 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:52:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Cheng]

Created attachment 157538 [details]
patch

fixed up styles again

[Build Bot]

Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470363

[Gyuyoung Kim]

Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13473112

[Build Bot]

Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13463483

[Alice Cheng]

Created attachment 157597 [details]
Patch for buildbot

[WebKit Review Bot]

Comment on attachment 157597 [details]
Patch for buildbot

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

[Build Bot]

Comment on attachment 157597 [details]
Patch for buildbot

Attachment 157597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13475068

[Alice Cheng]

Created attachment 157606 [details]
patch for build bot

Added #include in EditingAllInOne.cpp

[WebKit Review Bot]

Attachment 157606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/ChangeLog:20:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 157606 [details]
patch for build bot

Attachment 157606 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13464532

[WebKit Review Bot]

Comment on attachment 157606 [details]
patch for build bot

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

[Alice Cheng]

Created attachment 157741 [details]
patch for buildbot

Added file to WebCore.exp.in

[WebKit Review Bot]

Attachment 157741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 157741 [details]
patch for buildbot

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

[Alice Cheng]

Created attachment 157792 [details]
patch for build bot

fixed the spaces in GNUmakefile.list.am and styles in ChangeLog

[Alice Cheng]

Created attachment 157794 [details]
patch for buildbot

[Alice Cheng]

Created attachment 157849 [details]
patch to resolve conflicts

[WebKit Review Bot]

Comment on attachment 157849 [details]
patch to resolve conflicts

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

[Ryosuke Niwa]

Comment on attachment 157849 [details]
patch to resolve conflicts

View in context: https://bugs.webkit.org/attachment.cgi?id=157849&action=review

The patch looks good. We should probably get enrica or darin's feedback on this as well.

> Source/WebCore/WebCore.gypi:130
> +            'eidting/MoveAllStyleElementsFromBodyToHead.h',

Typo: eidting

> Source/WebCore/WebCore.gypi:2680
> +            'eidting/MoveAllStyleElementsFromBodyToHead.cpp',

Ditto.

[Alice Cheng]

(In reply to comment #24)
> (From update of attachment 157849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157849&action=review
> 
> The patch looks good. We should probably get enrica or darin's feedback on this as well.
> 
> > Source/WebCore/WebCore.gypi:130
> > +            'eidting/MoveAllStyleElementsFromBodyToHead.h',
> 
> Typo: eidting
> 
> > Source/WebCore/WebCore.gypi:2680
> > +            'eidting/MoveAllStyleElementsFromBodyToHead.cpp',
> 
> Ditto.

Thanks!! Was very confused of the chrome build failure. Now it is clear why... Thanks. On fixing it

[Alice Cheng]

Created attachment 158019 [details]
patch for build bot

[Darin Adler]

Comment on attachment 157849 [details]
patch to resolve conflicts

View in context: https://bugs.webkit.org/attachment.cgi?id=157849&action=review

review- since the build has to be fixed and we should use a RefPtr.

> Source/WebCore/ChangeLog:9
> +        Style elements can appear inside editable content. To prevent accidental deletion, we provide a way for WebKit clients to move style elements from within the body to the head section.

This seems OK. But to fully address the problem, I’d also want to fix the editing logic so that style elements would be preserved when you delete content that contains style elements. This technique of patching around the problem up front by moving the elements out of the body seems to me like only a partial fix. There’s no guarantee this move command will be done after every operation that might create a style element. I also don’t like the idea that clients have to do this manually rather than have this work automatically.

> Source/WebCore/ChangeLog:25
> +        * editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp: Added.

I don’t fully understand why we need a new command object for this rather than just having a function to call. What’s the benefit to having it be an editing command? Do we get better undo capabilities that way?

Having this be a command object has a non-negligible cost. All the build changes in this patch would be unnecessary if this was simply a function rather than a new editing command class.

> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:43
> +void MoveAllStyleElementsFromBodyToHeadCommand::doApply()

Given that this moves all style elements and all link elements from the body to the head, the name “move all style elements” doesn’t seem optimal.

Are all link elements used for styling? Is it good to move all link elements.

> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:45
> +    HTMLHeadElement* head = document()->head();

Is there some reason it is OK to hold the head element in a raw pointer while mutating the documents? It seems to me that JavaScript or mutation events could remove the head from the document and cause it to be deleted during the algorithm. So I think we need to use a RefPtr, not a raw pointer, for this.

> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:56
> +    ExceptionCode ec = 0;
> +    RefPtr<NodeList> styleNodes = document()->body()->querySelectorAll("style", ec);
> +    if (ec)
> +        return;
> +    RefPtr<NodeList> linkNodes = document()->body()->querySelectorAll("link", ec);
> +    if (ec)
> +        return;

While it’s OK to use querySelector for this, it’s not ideal. The string constants here are not good style. Normally we’d use the HTML names objects such as styleTag and linkTag.

I think that querySelector’s code path is slower than just a walk of the body tree, calling hasTagName on each element.

> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.h:43
> +        

Extra blank line here.

[WebKit Review Bot]

Comment on attachment 158019 [details]
patch for build bot

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

[Alice Cheng]

Comment on attachment 157849 [details]
patch to resolve conflicts

View in context: https://bugs.webkit.org/attachment.cgi?id=157849&action=review

Thanks Darin and Ryosuke

>> Source/WebCore/ChangeLog:9
>> +        Style elements can appear inside editable content. To prevent accidental deletion, we provide a way for WebKit clients to move style elements from within the body to the head section.
> 
> This seems OK. But to fully address the problem, I’d also want to fix the editing logic so that style elements would be preserved when you delete content that contains style elements. This technique of patching around the problem up front by moving the elements out of the body seems to me like only a partial fix. There’s no guarantee this move command will be done after every operation that might create a style element. I also don’t like the idea that clients have to do this manually rather than have this work automatically.

Fixed: Added a function in DeleteSelectionCommand instead, so that no client work is needed

>> Source/WebCore/ChangeLog:25
>> +        * editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp: Added.
> 
> I don’t fully understand why we need a new command object for this rather than just having a function to call. What’s the benefit to having it be an editing command? Do we get better undo capabilities that way?
> 
> Having this be a command object has a non-negligible cost. All the build changes in this patch would be unnecessary if this was simply a function rather than a new editing command class.

Fixed: Added a function in DeleteSelectionCommand instead, so that we no longer needs the new command class

>> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:43
>> +void MoveAllStyleElementsFromBodyToHeadCommand::doApply()
> 
> Given that this moves all style elements and all link elements from the body to the head, the name “move all style elements” doesn’t seem optimal.
> 
> Are all link elements used for styling? Is it good to move all link elements.

link elements, according to W3C, are mostly for styling, but not all. It does say that "The LINK element (<link>) is used to add this information in the header of your document in the HEAD element." here: http://www.w3.org/QA/Tips/use-links. Thus, I assume <head> is a safe place for <link>. Let me know if I get it wrong.

>> Source/WebCore/editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp:56
>> +        return;
> 
> While it’s OK to use querySelector for this, it’s not ideal. The string constants here are not good style. Normally we’d use the HTML names objects such as styleTag and linkTag.
> 
> I think that querySelector’s code path is slower than just a walk of the body tree, calling hasTagName on each element.

Fixed: Used HTMLName instead, and did a walk of the selection in DeleteSelection

[Ryosuke Niwa]

(In reply to comment #29)
> Fixed: Added a function in DeleteSelectionCommand instead, so that no client work is needed
> 
> >> Source/WebCore/ChangeLog:25
> >> +        * editing/MoveAllStyleElementsFromBodyToHeadCommand.cpp: Added.
> > 
> > I don’t fully understand why we need a new command object for this rather than just having a function to call. What’s the benefit to having it be an editing command? Do we get better undo capabilities that way?
> > 
> > Having this be a command object has a non-negligible cost. All the build changes in this patch would be unnecessary if this was simply a function rather than a new editing command class.
> 
> Fixed: Added a function in DeleteSelectionCommand instead, so that we no longer needs the new command class

FYI, if you're fixing DeleteSelectionCommand, we shouldn't be moving style and link elements to head element since this command is exposed to the Web.

[Alice Cheng]

Created attachment 158103 [details]
proposed patch

Styling elements (<link> and <style>) can appear inside editable content. To prevent accidental deletion, we move styling elements to <head> in DeleteSelectionCommand.

[WebKit Review Bot]

Attachment 158103 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/editing/DeleteSelectionCommand.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/editing/DeleteSelectionCommand.cpp:427:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 158103 [details]
proposed patch

I don't think we should do this. We'll end up modifying non-editable region of the document. Furthermore, by moving these nodes directly using DOM methods and thereby not adding it to the undo stack, we're breaking undo/redo of deletion because each undo step relies on the previous undo step to revert the exact DOM change that had happened.

[Alice Cheng]

Created attachment 158111 [details]
proposed patch for style bot

[Darin Adler]

Comment on attachment 158111 [details]
proposed patch for style bot

View in context: https://bugs.webkit.org/attachment.cgi?id=158111&action=review

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

Should be:

    // Move styling elements to prevent style loss.

[Darin Adler]

Comment on attachment 158111 [details]
proposed patch for style bot

Oops, I see Ryosuke’s comments now.

I think we should move these with undoable actions, not direct DOM manipulation. Not sure about his other comment.

[Darin Adler]

Comment on attachment 158111 [details]
proposed patch for style bot

Regarding Ryosuke’s other comment, maybe we should do this only if the head element is editable.

[Ryosuke Niwa]

(In reply to comment #37)
> (From update of attachment 158111 [details])
> Regarding Ryosuke’s other comment, maybe we should do this only if the head element is editable.

That's a possible compromise but we will still break the page if the style element had "scoped" content attribute.

[Ryosuke Niwa]

I think the right solution is to update each edit command that may end up deleting style/link element to not remove those elements. I bet there are only few places where we end up deleting style & link elements since they don't have renderers and don't have visible children.

[Alice Cheng]

Created attachment 158424 [details]
Proposed Patch

Modified command to preserve "undoness" of DeleteSelectionCommand
Modified the moving, so that it only moves when <head> is content editable

Also Thank Ryosuke and Darin for the review and comments!! I do have a question on Ryosuke's comment: if we do not remove these elements in certain places, then where shall we keep them, if their parents are to be removed as well (for example their parents are in a DeleteSelectionComand's selection)? <head> was a choice, since according to W3C, it is a safer place for these elements. Maybe you have better understanding on the removing procedure than i do. Let me know =)

[WebKit Review Bot]

Comment on attachment 158424 [details]
Proposed Patch

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

New failing tests:
svg/custom/bug79798.html
svg/custom/bug78838.html
editing/execCommand/insert-list-xml.xhtml
editing/pasteboard/drag-drop-input-in-svg.svg

[WebKit Review Bot]

Created attachment 158436 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Alice Cheng]

Created attachment 158440 [details]
proposed patch to fix test failures

[Darin Adler]

Comment on attachment 158424 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158424&action=review

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

[Darin Adler]

Comment on attachment 158440 [details]
proposed patch to fix test failures

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.

[Darin Adler]

Comment on attachment 158440 [details]
proposed patch to fix test failures

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

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:425
>> +        return;
> 
> The right way to do this is just:
> 
>     if (!head || !head->isContentEditable())
>         return;
> 
> You should use the contentEditable function that returns a string.

I meant: You should not have to use the contentEditable function that returns a string.

[Alice Cheng]

Comment on attachment 158440 [details]
proposed patch to fix test failures

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

[Ryosuke Niwa]

Comment on attachment 158440 [details]
proposed patch to fix test failures

View in context: https://bugs.webkit.org/attachment.cgi?id=158440&action=review

I still think this is a wrong approach. We should modify the code that's deleting style element to leave style elements alone. If they need to be moved around, then I'd prefer moving to the beginning of the root editable element.

Also, where are those style elements coming from? Do website authors embed those style elements in editable regions? Or are they coming from the pasted contents? If we're getting style elements from the latter, then we could also push down those styles to inline styles of the elements.

> LayoutTests/editing/execCommand/delete-selection-has-style.html:9
> +Markup.dump('html', '<head> should have styling elements');

You should just pass in document instead.

[Darin Adler]

(In reply to comment #48)
> If they need to be moved around, then I'd prefer moving to the beginning of the root editable element.

That sounds OK.

We’re seeing this in the case of Mail app, and in that case the entire document is editable, so the beginning of the root editable element would be the start of the body element, I think. It might be hard to get the logic if the start of the selection happens to already be the start of the body, though. Probably not all that hard.

> Also, where are those style elements coming from?

The real world example here is where the elements are included in incoming mail messages. The common case is replying to such a message and editing the mail in the OS X Mail app, which uses WebKit. But clearly the same thing could happen in a webmail app like gmail depending on its design. So we’d want the logic to work there too.

[Darin Adler]

(In reply to comment #47)
> > 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?

Good point. We can’t use isContentEditable for this. The real fix is probably to do what Ryosuke suggests and keep this inside the root editable node rather than moving it into the head.

If we do want this, we need to use the already existing string comparisons in HTMLElement::collectStyleForAttribute, not write a separate explicit string compare here at the call site. We don't want multiple call sites that know things like whether this is a case sensitive comparison or not. But also, we'd need to implement the rules for inheritance of editability. The head element itself might not have a contenteditable attribute on it, and need not. I think this check of the attribute is what they call a "bad code smell", which indicates we are taking the wrong approach to this.

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

If we do an undoable remove of the node before the rest of DeleteSelectionCommand runs, then we’ll get exactly what we want. When undoing we’ll first undo the rest of the deletion, then undo the appending to the head, then undo the removal and end up just the way we started.

[Alice Cheng]

Created attachment 158671 [details]
proposed patch

Thanks Darin and Ryosuke for the great feebacks. 

You were both right about head should not be content editable. Here is another revised patch to move styling elements to rootEditableElement (undoably). 

It does fix the buggy behavior in our Mail application, as I tested.

[Ryosuke Niwa]

Comment on attachment 158671 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158671&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:419
> +void DeleteSelectionCommand::moveStylingElementsToHead()

Nit: function name should also be revised.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:423
> +        if (node->hasTagName(styleTag) || node->hasTagName(linkTag)) {

We should probably not move the scoped style element so we should check for the presence of "scopedAttr" and then skip those nodes.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:424
> +            RefPtr<Node> previousNode = node->traversePreviousNode();

It's probably better to compute next in advance rather than traversing the previous node, and then traversing the next node.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:427
> +            node->setParentOrHostNode(0);

We shouldn't be updating parent node like this; you're breaking undo/redo.
appendNode should be able to update the parent pointer automatically.
r- because of this.

> LayoutTests/editing/execCommand/delete-selection-has-style.html:2
> +<!DOCTYPE html><body><div id="description">This tests deleting a selection that has a styling element in it. Should move styling elements to head to prevent style loss.</div>
> +<div id="one" contentEditable="true"><div> hide styling elements in --> </div> <style> .text { color:red; } </style> <link rel="stylesheet" type="text/css" href="style.css" /> <div class = "text"> between </div> </div>

Could you insert blank lines so that they're not all clamped into two lines?

> LayoutTests/editing/execCommand/delete-selection-has-style.html:9
> +Markup.dump('document', 'styling elements have been moved');

You can just dump the "one". Please revise the id so that it's more descriptive. Something like "rootEditableElement".

[Alice Cheng]

Created attachment 158842 [details]
proposed patch

Fixed the comments from Ryosuke's review

Had a question regarding casting Node* to Element* to do hasAttribute(scopedAttr)

[Alice Cheng]

Comment on attachment 158671 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158671&action=review

Thank you for your review!

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:419
>> +void DeleteSelectionCommand::moveStylingElementsToHead()
> 
> Nit: function name should also be revised.

fixed

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:423
>> +        if (node->hasTagName(styleTag) || node->hasTagName(linkTag)) {
> 
> We should probably not move the scoped style element so we should check for the presence of "scopedAttr" and then skip those nodes.

fixed, but a question: how shall I safely cast Node* to Element* here? Does the new patch have the right way of casting?

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:424
>> +            RefPtr<Node> previousNode = node->traversePreviousNode();
> 
> It's probably better to compute next in advance rather than traversing the previous node, and then traversing the next node.

fixed, used a while loop instead

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:427
>> +            node->setParentOrHostNode(0);
> 
> We shouldn't be updating parent node like this; you're breaking undo/redo.
> appendNode should be able to update the parent pointer automatically.
> r- because of this.

fixed

>> LayoutTests/editing/execCommand/delete-selection-has-style.html:2
>> +<div id="one" contentEditable="true"><div> hide styling elements in --> </div> <style> .text { color:red; } </style> <link rel="stylesheet" type="text/css" href="style.css" /> <div class = "text"> between </div> </div>
> 
> Could you insert blank lines so that they're not all clamped into two lines?

fixed

>> LayoutTests/editing/execCommand/delete-selection-has-style.html:9
>> +Markup.dump('document', 'styling elements have been moved');
> 
> You can just dump the "one". Please revise the id so that it's more descriptive. Something like "rootEditableElement".

fixed

[WebKit Review Bot]

Attachment 158842 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/editing/DeleteSelectionCommand.cpp:431:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Cheng]

Created attachment 158845 [details]
proposed patch for style bot

[Ryosuke Niwa]

Comment on attachment 158845 [details]
proposed patch for style bot

View in context: https://bugs.webkit.org/attachment.cgi?id=158845&action=review

> Source/WebCore/ChangeLog:9
> +        Styling elements (<link> and <style>) can appear inside editable content. To prevent accidental deletion, we move styling elements to rootEditableElement in DeleteSelectionCommand undoably.

Can we wrap the line at some point? It barely fits on my 24" monitor.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:424
> +        if ((node->hasTagName(styleTag) || node->hasTagName(linkTag)) && node->isElementNode() && !((Element*)node.get())->hasAttribute(scopedAttr)) {

Since node having either styleTag or linkTag already implies that node is an element, isElementNode() check is redundant.
Furthermore, we "scoped" on link element should have no effect so this condition should be:
node->hasTagName(styleTag) && toElement(node)->hasAttributesWithoutUpdate(scopedAttr) || node->hasTagName(linkTag)

Note hasAttributesWithoutUpdate can be used here because we don't need to synchronize style or SVG animation attributes.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:431
> +            node = node->traverseNextNode();

You can declare nextNode before the if statement and then you wouldn't need this else statement.
Just always update it.

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

It seems like this comment should be removed and we should rename the function incorporate this semantics instead.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:444
> +    moveStylingElementsToEditableRoot();

makeStylingElementsDirectChildrenOfEditableRootToPreventStyleLoss() ?

> LayoutTests/editing/execCommand/delete-selection-has-style.html:6
> +        <link rel="stylesheet" type="text/css" href="style.css" />

We should probably refer to a real stylesheet. instead of a file that doesn't exist.
I've seen tests behaving flakily when referencing a non-existing file.

[Alice Cheng]

Created attachment 158918 [details]
proposed patch

Fixed Ryosuke's last review
did not use hasAttributesWithoutUpdate, because that function does not take parameters..

[Alice Cheng]

(In reply to comment #57)
> (From update of attachment 158845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158845&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Styling elements (<link> and <style>) can appear inside editable content. To prevent accidental deletion, we move styling elements to rootEditableElement in DeleteSelectionCommand undoably.
> 
> Can we wrap the line at some point? It barely fits on my 24" monitor.
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:424
> > +        if ((node->hasTagName(styleTag) || node->hasTagName(linkTag)) && node->isElementNode() && !((Element*)node.get())->hasAttribute(scopedAttr)) {
> 
> Since node having either styleTag or linkTag already implies that node is an element, isElementNode() check is redundant.
> Furthermore, we "scoped" on link element should have no effect so this condition should be:
> node->hasTagName(styleTag) && toElement(node)->hasAttributesWithoutUpdate(scopedAttr) || node->hasTagName(linkTag)
> 
> Note hasAttributesWithoutUpdate can be used here because we don't need to synchronize style or SVG animation attributes.
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:431
> > +            node = node->traverseNextNode();
> 
> You can declare nextNode before the if statement and then you wouldn't need this else statement.
> Just always update it.
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:443
> > +    // Move styling elements to prevent style loss.
> 
> It seems like this comment should be removed and we should rename the function incorporate this semantics instead.
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:444
> > +    moveStylingElementsToEditableRoot();
> 
> makeStylingElementsDirectChildrenOfEditableRootToPreventStyleLoss() ?
> 
> > LayoutTests/editing/execCommand/delete-selection-has-style.html:6
> > +        <link rel="stylesheet" type="text/css" href="style.css" />
> 
> We should probably refer to a real stylesheet. instead of a file that doesn't exist.
> I've seen tests behaving flakily when referencing a non-existing file.

Ryosuke, Do you think it is now all set? Is there anything else I can do for the patch?

[WebKit Review Bot]

Comment on attachment 158918 [details]
proposed patch

Clearing flags on attachment: 158918

Committed r125955: <http://trac.webkit.org/changeset/125955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.