Summary: | Preserve styling elements in DeleteSelectionCommand | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alice Cheng <alice_cheng> | ||||||||||||||||||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Alice Cheng <alice_cheng> | ||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, darin, dglazkov, enrica, eric, gyuyoung.kim, mifenton, rakuco, rniwa, webkit.review.bot | ||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Alice Cheng
2012-08-09 13:11:05 PDT
Created attachment 157530 [details]
patch
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. Created attachment 157535 [details]
patch
Fixed up styles
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.
Created attachment 157538 [details]
patch
fixed up styles again
Comment on attachment 157538 [details] patch Attachment 157538 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13470363 Comment on attachment 157538 [details] patch Attachment 157538 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13473112 Comment on attachment 157538 [details] patch Attachment 157538 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13463483 Created attachment 157597 [details]
Patch for buildbot
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 on attachment 157597 [details] Patch for buildbot Attachment 157597 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13475068 Created attachment 157606 [details]
patch for build bot
Added #include in EditingAllInOne.cpp
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 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 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 Created attachment 157741 [details]
patch for buildbot
Added file to WebCore.exp.in
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 on attachment 157741 [details] patch for buildbot Attachment 157741 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13478173 Created attachment 157792 [details]
patch for build bot
fixed the spaces in GNUmakefile.list.am and styles in ChangeLog
Created attachment 157794 [details]
patch for buildbot
Created attachment 157849 [details]
patch to resolve conflicts
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 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. (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 Created attachment 158019 [details]
patch for build bot
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 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 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 (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. 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.
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 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.
Created attachment 158111 [details]
proposed patch for style bot
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 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 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.
(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. 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. 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 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 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
Created attachment 158440 [details]
proposed patch to fix test failures
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 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 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 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 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. (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. (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. 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 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". 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 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 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.
Created attachment 158845 [details]
proposed patch for style bot
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. Created attachment 158918 [details]
proposed patch
Fixed Ryosuke's last review
did not use hasAttributesWithoutUpdate, because that function does not take parameters..
(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 on attachment 158918 [details] proposed patch Clearing flags on attachment: 158918 Committed r125955: <http://trac.webkit.org/changeset/125955> All reviewed patches have been landed. Closing bug. |