[Webkit-unassigned] [Bug 93643] Add a cleanup method for WebKit clients to move style elements from <body> to <head>

Mon Aug 13 12:59:05 PDT 2012

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

--- Comment #29 from Alice Cheng <alice_cheng at apple.com>  2012-08-13 12:59:32 PST ---
(From update of attachment 157849)
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

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