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

Wed Aug 15 17:51:16 PDT 2012

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #158671|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #52 from Ryosuke Niwa <rniwa at webkit.org>  2012-08-15 17:51:43 PST ---
(From update of attachment 158671)
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".

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