[Blink/Webkit] Span elements created while joining adjacent elements

[Olek Nowodziński]

It's a follow-up of #9998, which covers a single ​case.

---

Example 1

1. Set data

   <p>foo</p>
   <p>^bar</p>
   

2. Backspace.
3. <p>foo<span style="line-height:1.6">bar</span></p>
4. Expected:

   <p>foo^bar</p>
   

Example 2

1. Set data

   <h1>foo</h1>
   <p>^bar</p>
   

2. Backspace.
3. <h1>foo<span style="font-size:13px; line-height:1.6">bar</span></h1>
4. Expected:

   <h1>foo^bar</h1>
   

[Piotrek Koszuliński]

cc

[Olek Nowodziński]

Development in ​branch:t/11861 (and tests).

[Piotrek Koszuliński]

I briefly reviewed Olek's fix and added missing tests. The implementation looks very good and I don't think I'll be making some significant changes to it. It'd be cool if some of you have a quick look at how it behaves.

1. Code: ​branch:t/11861.
2. Browsers: Webkit+Blink.
3. Covered cases: backspace/delete on collapsed selection, blocks merging.
4. Things to note:
   • There was already a lot of code handling backspace/delete in tables and lists. The plan was of course to not override this behaviour, so nothing should change.
   • If you look for ideas for tests you can check ​http://ckeditor4.t/dt/core/editable/keystrokes/delbackspacequirks.html

Let me know if you found any weird or clearly broken case.

[Piotrek Koszuliński]

TC1. (backspace):

<p>x</p><hr /><p>^y</p>

The <hr /> is totally skipped.

TC2. (backspace):

Open the readonly sample and set selection:

<p>x</p><p>^y</p>

Make editor read only by clicking the button. Click the editable and press backspace. Backspace will be handled. But this isn't only this handler's problem - I could reproduce the same issue with lists on 4.4.0.

[Marek Lewandowski]

Good job, common TC is handled pretty nicely.

Things which I've noticed:

we should also add this handling for shfit + backspace the same way as single backspace

offtopic: why shift + delete does not work the same way as single delete does? (it's not consistent)

we should remeber to add ctrl + backspace and ctrl + delete though it's slightly other case

[Olek Nowodziński]

Replying to Reinmar:

TC1. (backspace):

<p>x</p><hr /><p>^y</p>

The <hr /> is totally skipped.

It's a drawback of range.moveToClosestEditablePosition, I guess.

[Piotrek Koszuliński]

Replying to a.nowodzinski:

Replying to Reinmar:

TC1. (backspace):

<p>x</p><hr /><p>^y</p>

The <hr /> is totally skipped.

It's a drawback of range.moveToClosestEditablePosition, I guess.

Yes. And I wonder how should it behave in such case, because it also includes navigating next to widgets. Optimally, it should behave like a widget, so it should be selected when navigating using arrows or pressing backspace/delete. Page break works like this right now and personally I like this (though, there's no visual indicator when it's highlighted, because it's a fake selection). But I think that it may be a too strong change, especially considering the lack of visual indication of selection.

So, I think that I need to handle this as a special case and only in this case (excluding widgets).

[Piotrek Koszuliński]

Replying to m.lewandowski:

we should also add this handling for shfit + backspace the same way as single backspace

offtopic: why shift + delete does not work the same way as single delete does? (it's not consistent)

we should remeber to add ctrl + backspace and ctrl + delete though it's slightly other case

We decided to handle backspace and delete with modifiers (CTRL or SHIFT) as without modifiers. That won't of course change anything in case of SHIFT, but it will change behaviour of keystroke with CTRL. Unfortunately natively this keystroke creates crappy spans too, but we don't want to spend energy and bloat editor's code by implementing real "delete entire word" handler. Therefore, in these specific cases in which we run our custom handler, we'll also run it for backspace and delete used with modifiers.

[Artur Delura]

I think it's wrong behaviour:

  <p>foo^</p>
  <p>^bar</p>

Please note: first caret begin selection, second one ends. To achieve this put caret right before bar word. Press and hold Shift, press Up Arrow, press End, release Shift key.

And then press backspace. Result:

<p>foo<span style="line-height:1.6">bar</span></p>

It's because range.collapsed is false in algorithm. We don't handle this situation?

[Piotrek Koszuliński]

This patch fixes only the case when selection is collapsed. By selecting the line break you make it a non-collapsed selection. We'll fix this case in the next release.

[Piotrek Koszuliński]

Rebased branches, fixed TC1 from comment:9 and comment:13.

I noticed that we never scroll to the selection after delete/backspace. Not in this case and not in other handlers. So I don't think it's a big problem.

[Joel]

cc

[Olek Nowodziński]

I pushed a minor commit to dev.

[Olek Nowodziński]

...and I think our patch is production–ready. It got to be tested carefully though, as we could've missed some aspect while implementing web browser ;)

[Piotrek Koszuliński]

Fixed on master with ​git:11e6c22 on dev and 0dc5ae8 on tests.

Note: Only a case when *Backspace* or *Delete* is pressed on collapsed (empty) selection is covered by this patch. The remaining case, with a non-empty selection, will be fixed in next release.