Better match browser behavior for deletions
authorAryeh Gregor <AryehGregor+gitcommit@gmail.com>
Sun, 24 Jul 2011 11:38:06 -0600
changeset 448 68aaba1b5fd4
parent 447 79d78ab07506
child 449 c0dda58e445c
Better match browser behavior for deletions

We were always stripping any left-behind wrappers, but sometimes it
turns out we don't want to.
editcommands.html
implementation.js
source.html
tests.js
--- a/editcommands.html	Sun Jul 24 09:46:34 2011 -0600
+++ b/editcommands.html	Sun Jul 24 11:38:06 2011 -0600
@@ -38,7 +38,7 @@
 <body class=draft>
 <div class=head id=head>
 <h1>HTML Editing Commands</h1>
-<h2 class="no-num no-toc" id=work-in-progress-&mdash;-last-update-22-july-2011>Work in Progress &mdash; Last Update 22 July 2011</h2>
+<h2 class="no-num no-toc" id=work-in-progress-&mdash;-last-update-24-july-2011>Work in Progress &mdash; Last Update 24 July 2011</h2>
 <dl>
  <dt>Editor
  <dd>Aryeh Gregor &lt;<a href=mailto:[email protected]>ay[email protected]</a>&gt;
@@ -3653,7 +3653,8 @@
 <h3 id=deleting-the-contents-of-a-range><span class=secno>8.4 </span>Deleting the contents of a range</h3>
 
 <p>To <dfn id=delete-the-contents>delete the contents</dfn> of a <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-range title=concept-range>range</a> <var title="">range</var>, given a
-<var title="">block merging</var> flag that defaults to true:
+<var title="">block merging</var> flag that defaults to true and a <var title="">strip
+wrappers</var> flag that defaults to true:
 <!-- TODO: Consider what should happen for block merging in corner cases like
 display: inline-table. -->
 
@@ -3879,11 +3880,11 @@
 
     <li>Remove <var title="">node</var> from <var title="">parent</var>.
 
-    <li>While <var title="">parent</var> is an <a href=#editable>editable</a> <a href=#inline-node>inline
-    node</a> with <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-node-length title=concept-node-length>length</a> 0, let <var title="">grandparent</var> be the
-    <a class=external data-anolis-spec=domcore href=http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-tree-parent title=concept-tree-parent>parent</a> of <var title="">parent</var>, then remove <var title="">parent</var> from
-    <var title="">grandparent</var>, then set <var title="">parent</var> to
-    <var title="">grandparent</var>.
+    <li>If <var title="">strip wrappers</var> is true, while <var title="">parent</var> is an
+    <a href=#editable>editable</a> <a href=#inline-node>inline node</a> with <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-node-length title=concept-node-length>length</a> 0, let
+    <var title="">grandparent</var> be the <a class=external data-anolis-spec=domcore href=http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-tree-parent title=concept-tree-parent>parent</a> of <var title="">parent</var>, then remove
+    <var title="">parent</var> from <var title="">grandparent</var>, then set <var title="">parent</var>
+    to <var title="">grandparent</var>.
 
     <li>If <var title="">parent</var> is <a href=#editable>editable</a> or an <a href=#editing-host>editing
     host</a>, is not an <a href=#inline-node>inline node</a>, and has no <a class=external data-anolis-spec=domcore href=http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-tree-child title=concept-tree-child>children</a>,
@@ -5361,6 +5362,13 @@
 <h3 id=the-delete-command><span class=secno>8.10 </span><dfn>The <code title="">delete</code> command</dfn></h3>
 
 <p><a href=#action>Action</a>:
+<!--
+For all the deletions here, Firefox 7.0a2 will remove wrapper elements like <b>
+only if they're selected, like {<b>foo</b>}.  IE9, Chrome 14 dev, and Opera
+11.50 will all remove them even if only their contents are selected, like
+<b>[foo]</b>.  Gecko's behavior in the latter case leaves things like <b>{}</b>
+in the DOM, which is unhelpful, so I don't.
+-->
 
 <ol>
   <li>If the <a href=#active-range>active range</a> is not <code class=external data-anolis-spec=domrange title=dom-Range-collapsed><a href=http://html5.org/specs/dom-range.html#dom-range-collapsed>collapsed</a></code>, <a href=#delete-the-contents>delete the contents</a>
@@ -6309,8 +6317,14 @@
 
 <ol>
   <li><a href=#delete-the-contents>Delete the contents</a> of the <a href=#active-range>active range</a>.
-  <!-- Chrome 14 dev and Opera 11.11 do this even if the value is empty.
-  Firefox 5.0a2 throws an exception. -->
+  <!--
+  Chrome 14 dev and Opera 11.11 do this even if the value is empty.  Firefox
+  5.0a2 throws an exception.
+
+  Firefox 7.0a2 and Chrome 14 dev do strip wrappers here, so inserting HTML in
+  the place of <b>[foo]</b> will remove the <b>.  Opera 11.50 keeps the
+  wrappers.  I follow the majority.
+  -->
 
   <li>If the <a href=#active-range>active range</a>'s <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-range-start title=concept-range-start>start</a> <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-boundary-point-node title=concept-boundary-point-node>node</a> is neither
   <a href=#editable>editable</a> nor an <a href=#editing-host>editing host</a>, abort these steps.
@@ -6393,7 +6407,17 @@
 
   <li>Let <var title="">range</var> be the <a href=#active-range>active range</a>.
 
-  <li><a href=#delete-the-contents>Delete the contents</a> of <var title="">range</var>.
+  <li><a href=#delete-the-contents>Delete the contents</a> of <var title="">range</var>, with <var title="">strip
+  wrappers</var> false.
+  <!--
+  Firefox 7.0a2 seems to strip the wrapper or not depending on the exact
+  positioning of the selection: <b>{foo}</b> yes, <b>[foo]</b> no.  Chrome 14
+  dev seems to strip the wrapper regardless.  Opera 11.50 seems to keep the
+  wrapper, but place the image outside it.  I didn't get IE to cooperate with
+  my tests.  I chose to leave wrappers across the board because they might be
+  meaningful: e.g., a background-color when the image is small or not fully
+  opaque.
+  -->
 
   <li>If the <a href=#active-range>active range</a>'s <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-range-start title=concept-range-start>start</a> <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-boundary-point-node title=concept-boundary-point-node>node</a> is neither
   <a href=#editable>editable</a> nor an <a href=#editing-host>editing host</a>, abort these steps.
@@ -6447,7 +6471,14 @@
 -->
 
 <ol>
-  <li><a href=#delete-the-contents>Delete the contents</a> of the <a href=#active-range>active range</a>.
+  <li><a href=#delete-the-contents>Delete the contents</a> of the <a href=#active-range>active range</a>, with
+  <var title="">strip wrappers</var> false.
+  <!--
+  IE9 doesn't strip wrappers (IE10PP2 didn't work in tests).  Firefox 7.0a2
+  strips wrappers inconsistently depending on the exact selection endpoints.
+  Chrome 14 dev strips wrappers but recreates any styles using new wrappers.
+  Opera 11.50 strips all wrappers.
+  -->
 
   <li>If the <a href=#active-range>active range</a>'s <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-range-start title=concept-range-start>start</a> <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-boundary-point-node title=concept-boundary-point-node>node</a> is neither
   <a href=#editable>editable</a> nor an <a href=#editing-host>editing host</a>, abort these steps.
@@ -6876,8 +6907,17 @@
 -->
 
 <ol>
-  <li><a href=#delete-the-contents>Delete the contents</a> of the <a href=#active-range>active range</a>.
-  <!-- Chrome 14 dev does this even if passed the empty string. -->
+  <li><a href=#delete-the-contents>Delete the contents</a> of the <a href=#active-range>active range</a>, with
+  <var title="">strip wrappers</var> false.
+  <!--
+  Chrome 14 dev does the deletion even if the value is empty.  Of course, other
+  browsers don't expose this as an execCommand(), so no one else has any
+  defined behavior in this case at all, so I follow Chrome.
+
+  IE9, Firefox 7.0a2, Chrome 14 dev, and Opera 11.50 all don't strip wrappers,
+  except that as usual, Gecko doesn't if you select the whole wrapper, like
+  {<b>foo</b>}.
+  -->
 
   <li>If the <a href=#active-range>active range</a>'s <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-range-start title=concept-range-start>start</a> <a class=external data-anolis-spec=domrange href=http://html5.org/specs/dom-range.html#concept-boundary-point-node title=concept-boundary-point-node>node</a> is neither
   <a href=#editable>editable</a> nor an <a href=#editing-host>editing host</a>, abort these steps.
--- a/implementation.js	Sun Jul 24 09:46:34 2011 -0600
+++ b/implementation.js	Sun Jul 24 11:38:06 2011 -0600
@@ -4084,14 +4084,17 @@
 	//
 	// 1) A single argument, which is a range.
 	//
-	// 2) Two arguments, the first being a range and the second the
-	// blockMerging flag.
+	// 2) Two arguments, the first being a range and the second flags.
 	//
 	// 3) Four arguments, the start and end of a range.
 	//
-	// 4) Five arguments, the start and end of a range plus blockMerging.
+	// 4) Five arguments, the start and end of a range plus flags.
+	//
+	// The flags argument is a dictionary that can have up to two keys,
+	// blockMerging and stripWrappers, whose corresponding values are
+	// interpreted as boolean.  E.g., {stripWrappers: false}.
 	var range;
-	var blockMerging = true;
+	var flags = {};
 
 	if (arguments.length < 3) {
 		range = arguments[0];
@@ -4101,11 +4104,14 @@
 		range.setEnd(arguments[2], arguments[3]);
 	}
 	if (arguments.length == 2) {
-		blockMerging = arguments[1];
+		flags = arguments[1];
 	}
 	if (arguments.length == 5) {
-		blockMerging = arguments[4];
-	}
+		flags = arguments[4];
+	}
+
+	var blockMerging = "blockMerging" in flags ? !!flags.blockMerging : true;
+	var stripWrappers = "stripWrappers" in flags ? !!flags.stripWrappers : true;
 
 	// "If range is null, abort these steps and do nothing."
 	if (!range) {
@@ -4317,15 +4323,17 @@
 		// "Remove node from parent."
 		parent_.removeChild(node);
 
-		// "While parent is an editable inline node with length 0, let
-		// grandparent be the parent of parent, then remove parent from
-		// grandparent, then set parent to grandparent."
-		while (isEditable(parent_)
-		&& isInlineNode(parent_)
-		&& getNodeLength(parent_) == 0) {
-			var grandparent = parent_.parentNode;
-			grandparent.removeChild(parent_);
-			parent_ = grandparent;
+		// "If strip wrappers is true, while parent is an editable inline node
+		// with length 0, let grandparent be the parent of parent, then remove
+		// parent from grandparent, then set parent to grandparent."
+		if (stripWrappers) {
+			while (isEditable(parent_)
+			&& isInlineNode(parent_)
+			&& getNodeLength(parent_) == 0) {
+				var grandparent = parent_.parentNode;
+				grandparent.removeChild(parent_);
+				parent_ = grandparent;
+			}
 		}
 
 		// "If parent is editable or an editing host, is not an inline node,
@@ -6380,7 +6388,7 @@
 		}
 
 		// "Delete the contents of range, with block merging false."
-		deleteContents(range, false);
+		deleteContents(range, {blockMerging: false});
 
 		// "If the active range's start node is neither editable nor an editing
 		// host, abort these steps."
@@ -6508,8 +6516,8 @@
 		// "Let range be the active range."
 		var range = getActiveRange();
 
-		// "Delete the contents of range."
-		deleteContents(range);
+		// "Delete the contents of range, with strip wrappers false."
+		deleteContents(range, {stripWrappers: false});
 
 		// "If the active range's start node is neither editable nor an editing
 		// host, abort these steps."
@@ -6561,8 +6569,8 @@
 //@{
 commands.insertlinebreak = {
 	action: function(value) {
-		// "Delete the contents of the active range."
-		deleteContents(getActiveRange());
+		// "Delete the contents of the active range, with strip wrappers false."
+		deleteContents(getActiveRange(), {stripWrappers: false});
 
 		// "If the active range's start node is neither editable nor an editing
 		// host, abort these steps."
@@ -6951,8 +6959,9 @@
 //@{
 commands.inserttext = {
 	action: function(value) {
-		// "Delete the contents of the active range."
-		deleteContents(getActiveRange());
+		// "Delete the contents of the active range, with strip wrappers
+		// false."
+		deleteContents(getActiveRange(), {stripWrappers: false});
 
 		// "If the active range's start node is neither editable nor an editing
 		// host, abort these steps."
--- a/source.html	Sun Jul 24 09:46:34 2011 -0600
+++ b/source.html	Sun Jul 24 11:38:06 2011 -0600
@@ -3647,7 +3647,8 @@
 <h3>Deleting the contents of a range</h3>
 <!-- @{ -->
 <p>To <dfn>delete the contents</dfn> of a [[range]] <var>range</var>, given a
-<var>block merging</var> flag that defaults to true:
+<var>block merging</var> flag that defaults to true and a <var>strip
+wrappers</var> flag that defaults to true:
 <!-- TODO: Consider what should happen for block merging in corner cases like
 display: inline-table. -->
 
@@ -3873,11 +3874,11 @@
 
     <li>Remove <var>node</var> from <var>parent</var>.
 
-    <li>While <var>parent</var> is an <span>editable</span> <span>inline
-    node</span> with [[nodelength]] 0, let <var>grandparent</var> be the
-    [[parent]] of <var>parent</var>, then remove <var>parent</var> from
-    <var>grandparent</var>, then set <var>parent</var> to
-    <var>grandparent</var>.
+    <li>If <var>strip wrappers</var> is true, while <var>parent</var> is an
+    <span>editable</span> <span>inline node</span> with [[nodelength]] 0, let
+    <var>grandparent</var> be the [[parent]] of <var>parent</var>, then remove
+    <var>parent</var> from <var>grandparent</var>, then set <var>parent</var>
+    to <var>grandparent</var>.
 
     <li>If <var>parent</var> is <span>editable</span> or an <span>editing
     host</span>, is not an <span>inline node</span>, and has no [[children]],
@@ -5365,6 +5366,13 @@
 <h3><dfn>The <code title>delete</code> command</dfn></h3>
 <!-- @{ -->
 <p><span>Action</span>:
+<!--
+For all the deletions here, Firefox 7.0a2 will remove wrapper elements like <b>
+only if they're selected, like {<b>foo</b>}.  IE9, Chrome 14 dev, and Opera
+11.50 will all remove them even if only their contents are selected, like
+<b>[foo]</b>.  Gecko's behavior in the latter case leaves things like <b>{}</b>
+in the DOM, which is unhelpful, so I don't.
+-->
 
 <ol>
   <li>If the <span>active range</span> is not <code data-anolis-spec=domrange
@@ -6316,8 +6324,14 @@
 
 <ol>
   <li><span>Delete the contents</span> of the <span>active range</span>.
-  <!-- Chrome 14 dev and Opera 11.11 do this even if the value is empty.
-  Firefox 5.0a2 throws an exception. -->
+  <!--
+  Chrome 14 dev and Opera 11.11 do this even if the value is empty.  Firefox
+  5.0a2 throws an exception.
+
+  Firefox 7.0a2 and Chrome 14 dev do strip wrappers here, so inserting HTML in
+  the place of <b>[foo]</b> will remove the <b>.  Opera 11.50 keeps the
+  wrappers.  I follow the majority.
+  -->
 
   <li>If the <span>active range</span>'s [[startnode]] is neither
   <span>editable</span> nor an <span>editing host</span>, abort these steps.
@@ -6401,7 +6415,17 @@
 
   <li>Let <var>range</var> be the <span>active range</span>.
 
-  <li><span>Delete the contents</span> of <var>range</var>.
+  <li><span>Delete the contents</span> of <var>range</var>, with <var>strip
+  wrappers</var> false.
+  <!--
+  Firefox 7.0a2 seems to strip the wrapper or not depending on the exact
+  positioning of the selection: <b>{foo}</b> yes, <b>[foo]</b> no.  Chrome 14
+  dev seems to strip the wrapper regardless.  Opera 11.50 seems to keep the
+  wrapper, but place the image outside it.  I didn't get IE to cooperate with
+  my tests.  I chose to leave wrappers across the board because they might be
+  meaningful: e.g., a background-color when the image is small or not fully
+  opaque.
+  -->
 
   <li>If the <span>active range</span>'s [[startnode]] is neither
   <span>editable</span> nor an <span>editing host</span>, abort these steps.
@@ -6455,7 +6479,14 @@
 -->
 
 <ol>
-  <li><span>Delete the contents</span> of the <span>active range</span>.
+  <li><span>Delete the contents</span> of the <span>active range</span>, with
+  <var>strip wrappers</var> false.
+  <!--
+  IE9 doesn't strip wrappers (IE10PP2 didn't work in tests).  Firefox 7.0a2
+  strips wrappers inconsistently depending on the exact selection endpoints.
+  Chrome 14 dev strips wrappers but recreates any styles using new wrappers.
+  Opera 11.50 strips all wrappers.
+  -->
 
   <li>If the <span>active range</span>'s [[startnode]] is neither
   <span>editable</span> nor an <span>editing host</span>, abort these steps.
@@ -6885,8 +6916,17 @@
 -->
 
 <ol>
-  <li><span>Delete the contents</span> of the <span>active range</span>.
-  <!-- Chrome 14 dev does this even if passed the empty string. -->
+  <li><span>Delete the contents</span> of the <span>active range</span>, with
+  <var>strip wrappers</var> false.
+  <!--
+  Chrome 14 dev does the deletion even if the value is empty.  Of course, other
+  browsers don't expose this as an execCommand(), so no one else has any
+  defined behavior in this case at all, so I follow Chrome.
+
+  IE9, Firefox 7.0a2, Chrome 14 dev, and Opera 11.50 all don't strip wrappers,
+  except that as usual, Gecko doesn't if you select the whole wrapper, like
+  {<b>foo</b>}.
+  -->
 
   <li>If the <span>active range</span>'s [[startnode]] is neither
   <span>editable</span> nor an <span>editing host</span>, abort these steps.
--- a/tests.js	Sun Jul 24 09:46:34 2011 -0600
+++ b/tests.js	Sun Jul 24 11:38:06 2011 -0600
@@ -465,6 +465,9 @@
 
 		// Uncollapsed selection
 		'foo[bar]baz',
+		'<p>foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'<p>foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'<p>foo{<span style=color:#aBcDeF>bar</span>}baz',
 
 		'foo<b>[bar]</b>baz',
 		'foo<b>{bar}</b>baz',
@@ -1268,6 +1271,9 @@
 
 		// Uncollapsed selection (should be same as delete command)
 		'foo[bar]baz',
+		'<p>foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'<p>foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'<p>foo{<span style=color:#aBcDeF>bar</span>}baz',
 
 		'foo<b>[bar]</b>baz',
 		'foo<b>{bar}</b>baz',
@@ -1660,6 +1666,9 @@
 	//@{
 		'foo[]bar',
 		'foo[bar]baz',
+		'foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'foo{<span style=color:#aBcDeF>bar</span>}baz',
 		['', 'foo[bar]baz'],
 		['\0', 'foo[bar]baz'],
 		['\x07', 'foo[bar]baz'],
@@ -1729,6 +1738,9 @@
 		'<span>foo[</span><span>]bar</span>',
 		["", 'foo[bar]baz'],
 		'foo[bar]baz',
+		'foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'foo{<span style=color:#aBcDeF>bar</span>}baz',
 
 		'foo<b>[bar]</b>baz',
 		'foo<b>{bar}</b>baz',
@@ -2026,6 +2038,10 @@
 
 		'<p>foo[]<!--bar-->',
 		'<p><!--foo-->[]bar',
+
+		'<p>foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'<p>foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'<p>foo{<span style=color:#aBcDeF>bar</span>}baz',
 	],
 	//@}
 	inserttext: [
@@ -2110,6 +2126,9 @@
 		'{}<br>',
 		'<p>{}<br>',
 		'<p><span>{}<br></span>',
+		'<p>foo<span style=color:#aBcDeF>[bar]</span>baz',
+		'<p>foo<span style=color:#aBcDeF>{bar}</span>baz',
+		'<p>foo{<span style=color:#aBcDeF>bar</span>}baz',
 		'foo<a href=http://www.google.com/><font color=black>[bar]</font></a>baz',
 	],
 	//@}