Bug 93643 - Preserve styling elements in DeleteSelectionCommand
: Preserve styling elements in DeleteSelectionCommand
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Alice Cheng
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-09 13:11 PDT by Alice Cheng
Modified: 2012-08-17 17:31 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Cheng 2012-08-09 13:11:05 PDT
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 Alice Cheng 2012-08-09 13:11:24 PDT
<rdar://problem/12040676>
Comment 2 Alice Cheng 2012-08-09 13:35:11 PDT
Created attachment 157530 [details]
patch
Comment 3 WebKit Review Bot 2012-08-09 13:38:18 PDT
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 Alice Cheng 2012-08-09 13:46:30 PDT
Created attachment 157535 [details]
patch

Fixed up styles
Comment 5 WebKit Review Bot 2012-08-09 13:48:52 PDT
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 Alice Cheng 2012-08-09 13:51:34 PDT
Created attachment 157538 [details]
patch

fixed up styles again
Comment 7 Build Bot 2012-08-09 14:22:48 PDT
Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470363
Comment 8 Gyuyoung Kim 2012-08-09 15:04:14 PDT
Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13473112
Comment 9 Build Bot 2012-08-09 15:27:00 PDT
Comment on attachment 157538 [details]
patch

Attachment 157538 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13463483
Comment 10 Alice Cheng 2012-08-09 17:54:33 PDT
Created attachment 157597 [details]
Patch for buildbot
Comment 11 WebKit Review Bot 2012-08-09 18:16:49 PDT
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
Comment 12 Build Bot 2012-08-09 18:23:02 PDT
Comment on attachment 157597 [details]
Patch for buildbot

Attachment 157597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13475068
Comment 13 Alice Cheng 2012-08-09 18:31:56 PDT
Created attachment 157606 [details]
patch for build bot

Added #include in EditingAllInOne.cpp
Comment 14 WebKit Review Bot 2012-08-09 18:34:32 PDT
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 Build Bot 2012-08-09 18:49:47 PDT
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
Comment 16 WebKit Review Bot 2012-08-09 18:56:44 PDT
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
Comment 17 Alice Cheng 2012-08-10 08:58:26 PDT
Created attachment 157741 [details]
patch for buildbot

Added file to WebCore.exp.in
Comment 18 WebKit Review Bot 2012-08-10 09:00:19 PDT
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 WebKit Review Bot 2012-08-10 09:58:04 PDT
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
Comment 20 Alice Cheng 2012-08-10 12:38:58 PDT
Created attachment 157792 [details]
patch for build bot

fixed the spaces in GNUmakefile.list.am and styles in ChangeLog
Comment 21 Alice Cheng 2012-08-10 12:45:44 PDT
Created attachment 157794 [details]
patch for buildbot
Comment 22 Alice Cheng 2012-08-10 18:34:10 PDT
Created attachment 157849 [details]
patch to resolve conflicts
Comment 23 WebKit Review Bot 2012-08-10 19:53:22 PDT
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
Comment 24 Ryosuke Niwa 2012-08-11 12:48:11 PDT
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.
Comment 25 Alice Cheng 2012-08-13 09:00:15 PDT
(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
Comment 26 Alice Cheng 2012-08-13 09:37:29 PDT
Created attachment 158019 [details]
patch for build bot
Comment 27 Darin Adler 2012-08-13 09:47:57 PDT
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.
Comment 28 WebKit Review Bot 2012-08-13 10:23:17 PDT
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
Comment 29 Alice Cheng 2012-08-13 12:59:32 PDT
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
Comment 30 Ryosuke Niwa 2012-08-13 14:12:28 PDT
(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 Alice Cheng 2012-08-13 14:21:00 PDT
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.
Comment 32 WebKit Review Bot 2012-08-13 14:23:58 PDT
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 Ryosuke Niwa 2012-08-13 14:24:41 PDT
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.
Comment 34 Alice Cheng 2012-08-13 14:44:57 PDT
Created attachment 158111 [details]
proposed patch for style bot
Comment 35 Darin Adler 2012-08-14 12:52:55 PDT
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.
Comment 36 Darin Adler 2012-08-14 12:53:42 PDT
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.
Comment 37 Darin Adler 2012-08-14 12:54:27 PDT
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.
Comment 38 Ryosuke Niwa 2012-08-14 13:53:15 PDT
(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.
Comment 39 Ryosuke Niwa 2012-08-14 13:54:49 PDT
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 Alice Cheng 2012-08-14 15:25:45 PDT
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 =)
Comment 41 WebKit Review Bot 2012-08-14 16:12:52 PDT
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
Comment 42 WebKit Review Bot 2012-08-14 16:12:59 PDT
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
Comment 43 Alice Cheng 2012-08-14 16:30:49 PDT
Created attachment 158440 [details]
proposed patch to fix test failures
Comment 44 Darin Adler 2012-08-14 16:40:45 PDT
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.
Comment 45 Darin Adler 2012-08-14 16:41:15 PDT
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.
Comment 46 Darin Adler 2012-08-14 16:42:26 PDT
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.
Comment 47 Alice Cheng 2012-08-14 16:59:02 PDT
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
Comment 48 Ryosuke Niwa 2012-08-14 17:38:05 PDT
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.
Comment 49 Darin Adler 2012-08-15 16:01:46 PDT
(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 Darin Adler 2012-08-15 16:12:29 PDT
(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 Alice Cheng 2012-08-15 17:39:33 PDT
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.
Comment 52 Ryosuke Niwa 2012-08-15 17:51:43 PDT
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".
Comment 53 Alice Cheng 2012-08-16 09:46:40 PDT
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)
Comment 54 Alice Cheng 2012-08-16 09:48:45 PDT
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
Comment 55 WebKit Review Bot 2012-08-16 09:50:10 PDT
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 Alice Cheng 2012-08-16 09:53:55 PDT
Created attachment 158845 [details]
proposed patch for style bot
Comment 57 Ryosuke Niwa 2012-08-16 11:39:33 PDT
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.
Comment 58 Alice Cheng 2012-08-16 15:08:51 PDT
Created attachment 158918 [details]
proposed patch

Fixed Ryosuke's last review
did not use hasAttributesWithoutUpdate, because that function does not take parameters..
Comment 59 Alice Cheng 2012-08-17 15:34:25 PDT
(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?
Comment 60 WebKit Review Bot 2012-08-17 17:31:16 PDT
Comment on attachment 158918 [details]
proposed patch

Clearing flags on attachment: 158918

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