Bug 93643 - Preserve styling elements in DeleteSelectionCommand
: Preserve styling elements in DeleteSelectionCommand
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-08-09 13:11 PST by
Modified: 2012-08-17 17:31 PST (History)


Attachments
patch (15.22 KB, patch)
2012-08-09 13:35 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
patch (15.21 KB, patch)
2012-08-09 13:46 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
patch (15.20 KB, patch)
2012-08-09 13:51 PST, Alice Cheng
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch for buildbot (30.16 KB, patch)
2012-08-09 17:54 PST, Alice Cheng
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch for build bot (30.74 KB, patch)
2012-08-09 18:31 PST, Alice Cheng
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch for buildbot (31.46 KB, patch)
2012-08-10 08:58 PST, Alice Cheng
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch for build bot (31.46 KB, patch)
2012-08-10 12:38 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
patch for buildbot (31.46 KB, patch)
2012-08-10 12:45 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
patch to resolve conflicts (31.61 KB, patch)
2012-08-10 18:34 PST, Alice Cheng
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch for build bot (31.65 KB, patch)
2012-08-13 09:37 PST, Alice Cheng
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
proposed patch (6.40 KB, patch)
2012-08-13 14:21 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch for style bot (6.38 KB, patch)
2012-08-13 14:44 PST, Alice Cheng
darin: review-
Review Patch | Details | Formatted Diff | Diff
Proposed Patch (12.42 KB, patch)
2012-08-14 15:25 PST, Alice Cheng
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (366.66 KB, application/zip)
2012-08-14 16:12 PST, WebKit Review Bot
no flags Details
proposed patch to fix test failures (12.43 KB, patch)
2012-08-14 16:30 PST, Alice Cheng
darin: review-
Review Patch | Details | Formatted Diff | Diff
proposed patch (8.02 KB, patch)
2012-08-15 17:39 PST, Alice Cheng
rniwa: review-
rniwa: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
proposed patch (6.62 KB, patch)
2012-08-16 09:46 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch for style bot (6.61 KB, patch)
2012-08-16 09:53 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (6.74 KB, patch)
2012-08-16 15:08 PST, Alice Cheng
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-09 13:11:05 PST
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.
------- Comment #1 From 2012-08-09 13:11:24 PST -------
<rdar://problem/12040676>
------- Comment #2 From 2012-08-09 13:35:11 PST -------
Created an attachment (id=157530) [details]
patch
------- Comment #3 From 2012-08-09 13:38:18 PST -------
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.
------- Comment #4 From 2012-08-09 13:46:30 PST -------
Created an attachment (id=157535) [details]
patch

Fixed up styles
------- Comment #5 From 2012-08-09 13:48:52 PST -------
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.
------- Comment #6 From 2012-08-09 13:51:34 PST -------
Created an attachment (id=157538) [details]
patch

fixed up styles again
------- Comment #7 From 2012-08-09 14:22:48 PST -------
(From update of attachment 157538 [details])
Attachment 157538 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470363
------- Comment #8 From 2012-08-09 15:04:14 PST -------
(From update of attachment 157538 [details])
Attachment 157538 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13473112
------- Comment #9 From 2012-08-09 15:27:00 PST -------
(From update of attachment 157538 [details])
Attachment 157538 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13463483
------- Comment #10 From 2012-08-09 17:54:33 PST -------
Created an attachment (id=157597) [details]
Patch for buildbot
------- Comment #11 From 2012-08-09 18:16:49 PST -------
(From update of attachment 157597 [details])
Attachment 157597 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13461704
------- Comment #12 From 2012-08-09 18:23:02 PST -------
(From update of attachment 157597 [details])
Attachment 157597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13475068
------- Comment #13 From 2012-08-09 18:31:56 PST -------
Created an attachment (id=157606) [details]
patch for build bot

Added #include in EditingAllInOne.cpp
------- Comment #14 From 2012-08-09 18:34:32 PST -------
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.
------- Comment #15 From 2012-08-09 18:49:47 PST -------
(From update of attachment 157606 [details])
Attachment 157606 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13464532
------- Comment #16 From 2012-08-09 18:56:44 PST -------
(From update of attachment 157606 [details])
Attachment 157606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13458916
------- Comment #17 From 2012-08-10 08:58:26 PST -------
Created an attachment (id=157741) [details]
patch for buildbot

Added file to WebCore.exp.in
------- Comment #18 From 2012-08-10 09:00:19 PST -------
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.
------- Comment #19 From 2012-08-10 09:58:04 PST -------
(From update of attachment 157741 [details])
Attachment 157741 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13478173
------- Comment #20 From 2012-08-10 12:38:58 PST -------
Created an attachment (id=157792) [details]
patch for build bot

fixed the spaces in GNUmakefile.list.am and styles in ChangeLog
------- Comment #21 From 2012-08-10 12:45:44 PST -------
Created an attachment (id=157794) [details]
patch for buildbot
------- Comment #22 From 2012-08-10 18:34:10 PST -------
Created an attachment (id=157849) [details]
patch to resolve conflicts
------- Comment #23 From 2012-08-10 19:53:22 PST -------
(From update of attachment 157849 [details])
Attachment 157849 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13483075
------- Comment #24 From 2012-08-11 12:48:11 PST -------
(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.
------- Comment #25 From 2012-08-13 09:00:15 PST -------
(In reply to comment #24)
> (From update of attachment 157849 [details] [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
------- Comment #26 From 2012-08-13 09:37:29 PST -------
Created an attachment (id=158019) [details]
patch for build bot
------- Comment #27 From 2012-08-13 09:47:57 PST -------
(From update of attachment 157849 [details])
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.
------- Comment #28 From 2012-08-13 10:23:17 PST -------
(From update of attachment 158019 [details])
Attachment 158019 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13486543
------- Comment #29 From 2012-08-13 12:59:32 PST -------
(From update of attachment 157849 [details])
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
------- Comment #30 From 2012-08-13 14:12:28 PST -------
(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.
------- Comment #31 From 2012-08-13 14:21:00 PST -------
Created an attachment (id=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.
------- Comment #32 From 2012-08-13 14:23:58 PST -------
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.
------- Comment #33 From 2012-08-13 14:24:41 PST -------
(From update of attachment 158103 [details])
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.
------- Comment #34 From 2012-08-13 14:44:57 PST -------
Created an attachment (id=158111) [details]
proposed patch for style bot
------- Comment #35 From 2012-08-14 12:52:55 PST -------
(From update of attachment 158111 [details])
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.
------- Comment #36 From 2012-08-14 12:53:42 PST -------
(From update of attachment 158111 [details])
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.
------- Comment #37 From 2012-08-14 12:54:27 PST -------
(From update of attachment 158111 [details])
Regarding Ryosuke’s other comment, maybe we should do this only if the head element is editable.
------- Comment #38 From 2012-08-14 13:53:15 PST -------
(In reply to comment #37)
> (From update of attachment 158111 [details] [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.
------- Comment #39 From 2012-08-14 13:54:49 PST -------
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.
------- Comment #40 From 2012-08-14 15:25:45 PST -------
Created an attachment (id=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 =)
------- Comment #41 From 2012-08-14 16:12:52 PST -------
(From update of attachment 158424 [details])
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
------- Comment #42 From 2012-08-14 16:12:59 PST -------
Created an attachment (id=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
------- Comment #43 From 2012-08-14 16:30:49 PST -------
Created an attachment (id=158440) [details]
proposed patch to fix test failures
------- Comment #44 From 2012-08-14 16:40:45 PST -------
(From update of attachment 158424 [details])
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.
------- Comment #45 From 2012-08-14 16:41:15 PST -------
(From update of attachment 158440 [details])
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.
------- Comment #46 From 2012-08-14 16:42:26 PST -------
(From update of attachment 158440 [details])
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.
------- Comment #47 From 2012-08-14 16:59:02 PST -------
(From update of attachment 158440 [details])
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
------- Comment #48 From 2012-08-14 17:38:05 PST -------
(From update of attachment 158440 [details])
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.
------- Comment #49 From 2012-08-15 16:01:46 PST -------
(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.
------- Comment #50 From 2012-08-15 16:12:29 PST -------
(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.
------- Comment #51 From 2012-08-15 17:39:33 PST -------
Created an attachment (id=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.
------- Comment #52 From 2012-08-15 17:51:43 PST -------
(From update of attachment 158671 [details])
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".
------- Comment #53 From 2012-08-16 09:46:40 PST -------
Created an attachment (id=158842) [details]
proposed patch

Fixed the comments from Ryosuke's review

Had a question regarding casting Node* to Element* to do hasAttribute(scopedAttr)
------- Comment #54 From 2012-08-16 09:48:45 PST -------
(From update of attachment 158671 [details])
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
------- Comment #55 From 2012-08-16 09:50:10 PST -------
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.
------- Comment #56 From 2012-08-16 09:53:55 PST -------
Created an attachment (id=158845) [details]
proposed patch for style bot
------- Comment #57 From 2012-08-16 11:39:33 PST -------
(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.
------- Comment #58 From 2012-08-16 15:08:51 PST -------
Created an attachment (id=158918) [details]
proposed patch

Fixed Ryosuke's last review
did not use hasAttributesWithoutUpdate, because that function does not take parameters..
------- Comment #59 From 2012-08-17 15:34:25 PST -------
(In reply to comment #57)
> (From update of attachment 158845 [details] [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?
------- Comment #60 From 2012-08-17 17:31:16 PST -------
(From update of attachment 158918 [details])
Clearing flags on attachment: 158918

Committed r125955: <http://trac.webkit.org/changeset/125955>
------- Comment #61 From 2012-08-17 17:31:25 PST -------
All reviewed patches have been landed.  Closing bug.