[webkit-reviews] review denied: [Bug 81656] Backspace/delete produces combined font/span from existing inline elements : [Attachment 143818] Praposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 24 11:37:27 PDT 2012
Ryosuke Niwa <rniwa at webkit.org> has denied Parag Radke <parag at motorola.com>'s
request for review:
Bug 81656: Backspace/delete produces combined font/span from existing inline
elements
https://bugs.webkit.org/show_bug.cgi?id=81656
Attachment 143818: Praposed patch
https://bugs.webkit.org/attachment.cgi?id=143818&action=review
------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143818&action=review
This patch makes things worse, not better.
> Source/WebCore/ChangeLog:12
> + Covered by existing test, updated expected.txt.
Definitely not. r- because of the lack of the tests.
> Source/WebCore/editing/DeleteSelectionCommand.cpp:527
> + if (isTableStructureNode(node.get()))
> + removeNode(node);
> + else {
> + while (n != node) {
> + removeNode(n);
> + // see
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#delete-the-selection
point 22
> + if (!nParent->hasChildNodes() &&
isOnlyChildOfParent(nParent)) {
> + RefPtr<Node> placeholder = m_needPlaceholder
? createBreakElement(document()).get() : 0;
> + if (placeholder) {
> + if (m_sanitizeMarkup)
> + removeRedundantBlocks();
> + nParent->appendChild(placeholder.get());
> + m_needPlaceholder = false;
> + }
> + break;
> + }
> + // if nParent->isOnlyChildOfParent() is false we
are going to remove nParent.
> + if (nParent != node) {
> + n = nParent->parentNode();
> + removeNode(nParent);
> + nParent = n->parentNode();
> + continue;
> + }
> + removeNode(node);
> + break;
> + n = nParent;
> + nParent = nParent->parentNode();
> + }
> + node =0;
> + }
I don't think we can implement the spec partially like this. There are a lot of
inter-dependencies between different sections in the spec.
> LayoutTests/editing/deleting/delete-and-cleanup-expected.txt:8
> -PASS confirmedMarkup is '<b><br></b>'
> -PASS confirmedMarkup is '<b><div style="border: solid red"><br></div></b>'
> +PASS confirmedMarkup is '<b><i><br></i></b>'
> +PASS confirmedMarkup is '<b><div style="border: solid
red"><i><br></i></div></b>'
These outputs look strictly worse.
> LayoutTests/editing/deleting/delete-and-cleanup.html:41
> +testDelete("div", "<div><b><div><i>Hello</i></div></b></div>",
"<b><i><br></i></b>");
> +testDelete("div", "<div><b><div style=\"border: solid
red\"><i>Hello</i></div></b></div>", "<b><div style=\"border: solid
red\"><i><br></i></div></b>");
Ditto.
> LayoutTests/editing/style/block-style-005-expected.txt:9
> +execDeleteCommand: <div id="test" class="editing"><font
size="7">x</font></div><div id="test" class="editing"><font
size="7"><br></font></div><div id="test" class="editing"><font
size="7"><br></font></div>
Ditto.
>
LayoutTests/platform/gtk/editing/deleting/collapse-whitespace-3587601-fix-expec
ted.txt:56
> + RenderInline {SPAN} at (0,0) size 0x28
> + RenderBR {BR} at (14,14) size 0x28
Ditto.
More information about the webkit-reviews
mailing list