[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]

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.