Introduce new, focused on UX, methods for getting selected HTML and deleting it

[Piotr Jasiun]

Short description - we need methods which will retrieve and extract HTML in a way correct from user experience POV. For example when selection is inside <strong>, we need not only text that's selected, but also information about the style applied in this location, so that <strong> tag.

Outdated description:

Because of custom implementation of drag & drop (#11460) we will use selection.getSelectedHTML and range.deleteContents more often to read and remove dragged HTML. Current implementation of selection.getSelectedHTML can not be based on range.cloneContents because it changes range (#11586) and works not as we could expected.

For this reasons we need to reimplement selection.getSelectedHTML, range.cloneContents and range.deleteContents.

range.cloneContents and range.deleteContents should behave more as user could expect than simple clone or delete. For example:

• for 'xx<a href="foo">b[a]r</a>xx' range.cloneContents should return '<a href="foo">a</a>',
• for '<p>[foo]</p>' range.deleteContents' should remove whole paragraph,
• for '<table><tr>[<td>foo</td><td>b]ar</td></tr></table>' range.deleteContents should change contents to the '<table><tr><td></td><td>ar</td></tr></table>'.

selection.getSelectedHTML should be based on range.cloneContents.

[Piotr Jasiun]

During the testing phase for D&D two issues related to this ticket were found: Bug 4 and Bug 7.

​​https://docs.google.com/document/d/1hG4H0r21MXNkRd3amDEOBPygJe3ehBAXFWAal2DptGQ/edit?usp=sharing

[Olek Nowodziński]

cc

[Piotrek Koszuliński]

I reviewed tests and have some ideas for new ones and, unfortunately, so changes to existing ones.

New:

[<br>] | <br>
x{<br><br>}y | <br><br>
<br>[<br>]<br> | <br>
{foo<br>]@ | foo<br>
<p>{foo<br>]@</p> | <p>foo<br>@</p> (not sure if we need bogus in this case)

<p>[<b>y</b>]</p> | <b>y</b>
<p>x{<b>y}</b>x</p> | <b>y</b>

// Table row and entire table:

<table><tbody><tr><td>{x}</td></tr></tbody></table> | entire table | entire table is removed
<table><tbody><tr><td>{x</td><td>y}</td></tr></tbody></table> | entire table (+ case with table's attributes - they must be copied) | entire table is removed
<table><tbody><tr><td>1</td><td>2</td></tr><tr><td>{x</td><td>y}</td></tr><tr><td>1</td><td>2</td></tr></tbody></table> | entire row (wrapped with <table><tbody>) | entire row is removed
<table><tbody><tr><td>{1</td><td>2</td></tr><tr><td>x</td><td>y}</td></tr><tr><td>1</td><td>2</td></tr></tbody></table> | two rows (wrapped) | two rows are removed

Corrected:

// Line breaks selection is tricky. I believe that the only way to mark them is to use
// a special attribute.
// Additionally, we need integration with insertHtml() - it will be handled in separate ticket.
// In all these cases block should be merged (similar to backspace).

<p>x{</p><p>}x</p> | <br data-cke-newline="1">
<p>y{x</p><p>}x</p> | x<br data-cke-newline="1">
<p>y{x</p><p>z</p><p>}x</p> | x<p>z</p><br data-cke-newline="1">
<p>y{x</p><p>x}z</p> | x<br data-cke-newline="1">x
<h1>{x</h1><p>x}</p> | <h1>x</h1><p>x</p>
<p>ab{</p><p>x</p><p>}cd</p> | <br data-cke-newline="1"><p>x</p><br data-cke-newline="1">

// Unfortunately, I changed my mind regarding entire single line selection. We strip the block when
// only one block is being inserted anyway, so it doesn't make sense to return it. It shouldn't be removed too (unless
// line breaks are selected)
// Note: block must not be removed when it's content is removed and bogus may need to be inserted.

<p>x</p><p>{a}</p><p>z</p> | a | <p>x</p><p>^@</p><p>z</p>
<p>x</p><p><b>{a}</b></p><p>z</p> | <b>a</b> | <p>x</p><p><b>^</b>@</p><p>z</p>
<h1>{x}</h1> | x | <h1>^@</h1>

[Olek Nowodziński]

It's been a long while since we got some fun there ​git:1e58ae970c.

[Piotr Jasiun]

Might be related: #12262 (getSelectedText).

[Piotr Jasiun]

Also might be related: #11975 (D&D for tables), #11976 (D&D for lists), #11977 (D&D for images).

[Piotrek Koszuliński]

Related: ​https://dev.ckeditor.com/ticket/10969#comment:5

[Piotrek Koszuliński]

After having lots of fun I managed to stabilise the functions. I don't think this code may be thoroughly reviewed in a reasonable time, so I would rather recommend not wasting too much time on details. This code will still be heavily used after being plugged into the clipboard system.

[Piotrek Koszuliński]

Pushed ​branch:t/11636.

[Piotrek Koszuliński]

One thing for the future reference - we are now handling only the first range of the selection. That was a conscious decision made by us, because of the amount of work. It means that on Firefox the UX related to tables will not be perfect, but it's not a new situation - insertHtml has this limitation for a long time as well. Moreover, this limitiation is also reflected in the API, because the "base" methods (e.g. getHtmlFromRange, insertHtmlIntoRange) accept a single range.

The plan is that all these methods must be fixed at once if one day we'll work on better support for multi-range selections in tables. The most outer methods like insertHtml or getSelectedHtml will start using new methods called getHtmlFromRanges and insertHtmlIntoRanges. These new methods will handle the special cases related to tables or will use their single-range equivalents if only one range was passed or the specific case is not implemented (e.g. multi range selection outside table).

[Piotr Jasiun]

I rebased branch on the newest major and pushed some commits to fix tests. These methods are really cool, they handle extracting HTML far better then default methods, but I have some comments:

Code covarage:

• Code covarage is very good, but there are still paths which are not covered: in 4 places else path is not covered and in 3 if path. Even if there is single if in the code (without else) the else path should be covered. Otherwise there are 2 options: there is no code which can reach the else path (so the if closure is not needed) or there is a specific case which is not tested. Of course it may happen that the case is that specific that it is pointless to write tests, but very often these else cases are reasons of the future bugs.
• There are no tests for getSelectedHtml and extractSelectedHtml methods. They are pretty simple, but need simple tests to check if toString parameter works properly.

Helpers methods:

• Does we need ^ operator in walker.empty method? I proposed change: ​https://github.com/cksource/ckeditor-dev/commit/b7c1d41de843bbe3ec7b0585185b12ca5af52b52
• documentFragment.getHtml should use createHTMLDocument (it works on all browsers except IE8).

  var frag = new CKEDITOR.dom.documentFragment( CKEDITOR.document ),
  	img = new CKEDITOR.dom.element( 'img' );
  frag.append( img );
  img.setAttribute( 'onerror' , 'alert();' );
  img.setAttribute( 'src' , 'x' );
  frag.getHtml(); // Code is executed every time getHtml is called.
  

To avoid this, solution similar to this should be used: ​https://github.com/cksource/ckeditor-dev/blob/841e2dcf20fffef944d0dd8a106b6963b752bf35/plugins/uploadimage/plugin.js#L61-L62

Code style:

• I am not sure about function name: [get|extract]SelectedHtmlFromRange. "Selected" suggest that it depends on selection, but as far as I know it is not. Is not better: [get|extract]HtmlFromRange?

• method order is wrong: [get|extract]SelectedHtml should go before [get|extract]SelectedHtmlFromRange. General first, specific later.
• I do not like how [get|extract]SelectedHtmlFromRange code is organized. The public part (enter point) is at the very end of methods while they should be at the begging (public first, private later). What is even worst this enter point set some properties which are used by methods above them (e.g. this.range). ​https://github.com/cksource/ckeditor-dev/blob/t/11636/core/editable.js#L1324
• I think that constructions like this are overcomplicated: ​https://github.com/cksource/ckeditor-dev/blob/t/11636/core/editable.js#L976-L987 Is there any reason to create functions this way?

Bugs:

• When I select whole header or blockquote and call extractSelectedHtml only the content of the header or blockquote is selected and empty element is not removed:

  	<h1>[foo]</h1>
  	Expected extracted HTML: "<h1>foo</h1>"
  	Actual extracted HTML: "<h1>foo</h1>"
  	Expected result HTML: ""
  	Actual result HTML: "<h1></h1>"
  

  It is strange, especially because whole idea of those method was to handle similar case for inline elements. Inline elements works perfectly as expected while block elements do not.

• Tables: if one full row of the table is selected and part of the second then broken table is extracted:

  	<table>
  		<tr><td>[1</td><td>2</td></tr>
  		<tr><td>3]</td><td>4</td></tr>
  	</table>
  
  	Expected extracted HTML: 
  	<table>
  		<tr><td>1</td><td>2</td></tr>
  		<tr><td>3</td><td></td></tr>
  	</table>
  
  	Actual extracted HTML:
  	<table>
  		<tr><td>1</td><td>2</td></tr>
  		<tr><td>3</td></tr> // Only one cell in the second row.
  	</table>
  

I understand that it may be complex to handle all cases which use colspan and rowspan properties, but the simplest case should be handled.

[Piotrek Koszuliński]

Replying to pjasiun:

I rebased branch on the newest major and pushed some commits to fix tests. These methods are really cool, they handle extracting HTML far better then default methods, but I have some comments:

Code covarage:

• Code covarage is very good, but there are still paths which are not covered: in 4 places else path is not covered and in 3 if path. Even if there is single if in the code (without else) the else path should be covered. Otherwise there are 2 options: there is no code which can reach the else path (so the if closure is not needed) or there is a specific case which is not tested. Of course it may happen that the case is that specific that it is pointless to write tests, but very often these else cases are reasons of the future bugs.

Turned out that in these not-covered fragments I found a bug and some unnecessary code :D. Good catch.

• There are no tests for getSelectedHtml and extractSelectedHtml methods. They are pretty simple, but need simple tests to check if toString parameter works properly.

Done.

Helpers methods:

• Does we need ^ operator in walker.empty method? I proposed change: ​https://github.com/cksource/ckeditor-dev/commit/b7c1d41de843bbe3ec7b0585185b12ca5af52b52

Right.

• documentFragment.getHtml should use createHTMLDocument (it works on all browsers except IE8).

  var frag = new CKEDITOR.dom.documentFragment( CKEDITOR.document ),
  	img = new CKEDITOR.dom.element( 'img' );
  frag.append( img );
  img.setAttribute( 'onerror' , 'alert();' );
  img.setAttribute( 'src' , 'x' );
  frag.getHtml(); // Code is executed every time getHtml is called.
  

To avoid this, solution similar to this should be used: ​https://github.com/cksource/ckeditor-dev/blob/841e2dcf20fffef944d0dd8a106b6963b752bf35/plugins/uploadimage/plugin.js#L61-L62

I've checked that the onerror is executed whenever you add such image to any document fragment, as well as when you are cloning it. This onerror would be executed at some point anyway, so securing getHtml doesn't make sense. You simply should not have this onerror there.

Code style:

• I am not sure about function name: [get|extract]SelectedHtmlFromRange. "Selected" suggest that it depends on selection, but as far as I know it is not. Is not better: [get|extract]HtmlFromRange?

Done.

• method order is wrong: [get|extract]SelectedHtml should go before [get|extract]SelectedHtmlFromRange. General first, specific later.
• I do not like how [get|extract]SelectedHtmlFromRange code is organized. The public part (enter point) is at the very end of methods while they should be at the begging (public first, private later). What is even worst this enter point set some properties which are used by methods above them (e.g. this.range). ​https://github.com/cksource/ckeditor-dev/blob/t/11636/core/editable.js#L1324

Done.

• I think that constructions like this are overcomplicated: ​https://github.com/cksource/ckeditor-dev/blob/t/11636/core/editable.js#L976-L987 Is there any reason to create functions this way?

Done.

Bugs:

• When I select whole header or blockquote and call extractSelectedHtml only the content of the header or blockquote is selected and empty element is not removed:

  	<h1>[foo]</h1>
  	Expected extracted HTML: "<h1>foo</h1>"
  	Actual extracted HTML: "<h1>foo</h1>"
  	Expected result HTML: ""
  	Actual result HTML: "<h1></h1>"
  

  It is strange, especially because whole idea of those method was to handle similar case for inline elements. Inline elements works perfectly as expected while block elements do not.

This is how it works in FF. Chrome behaves differently but we chose the FF way which is simpler.

• Tables: if one full row of the table is selected and part of the second then broken table is extracted:

  	<table>
  		<tr><td>[1</td><td>2</td></tr>
  		<tr><td>3]</td><td>4</td></tr>
  	</table>
  
  	Expected extracted HTML: 
  	<table>
  		<tr><td>1</td><td>2</td></tr>
  		<tr><td>3</td><td></td></tr>
  	</table>
  
  	Actual extracted HTML:
  	<table>
  		<tr><td>1</td><td>2</td></tr>
  		<tr><td>3</td></tr> // Only one cell in the second row.
  	</table>
  

I understand that it may be complex to handle all cases which use colspan and rowspan properties, but the simplest case should be handled.

This is not only complex, but also I wouldn't be sure what's the expected result. If you selected part of a table what do you actually want to do?

Anyway, I think that we are now doing this in the good way leaving only these cells that were selected. This situation can then be handled while pasting depending for instance on the selection. Example - user selects 3+1 cells and copies them and pastes in the same place. If in the clipboard we had 3+3 cells what would we do when inserting that pasted content? :D Enormously tricky, isn't it? :D

I pushed ​branch:t/11636 with fixes.

[Piotr Jasiun]

So it is done (​git:8470d94). I do not like how some edge cases works, but I have to agree that there will be always room for improvements here and these methods work far better then native. Well done!

[Olek Nowodziński]

Replying to pjasiun:

So it is done (​git:8470d94). I do not like how some edge cases works, but I have to agree that there will be always room for improvements here and these methods work far better then native. Well done!

It feels like Christmas!