Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11861 closed Bug (fixed)

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

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 3.0
Keywords: Webkit Blink Cc: joel.peltonen@…

Description (last modified by Piotrek Koszuliń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>
    

Change History (21)

comment:1 Changed 4 years ago by Olek Nowodziński

Description: modified (diff)

comment:2 Changed 4 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: newassigned

comment:3 Changed 4 years ago by Piotrek Koszuliński

cc

comment:4 Changed 4 years ago by Olek Nowodziński

Keywords: Webkit Blink added
Summary: [Webkit] Span elements created while joining adjacent elements[Blink/Webkit] Span elements created while joining adjacent elements

comment:5 Changed 4 years ago by Olek Nowodziński

Description: modified (diff)

comment:6 Changed 4 years ago by Olek Nowodziński

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

comment:7 Changed 4 years ago by Piotrek Koszuliński

Owner: changed from Olek Nowodziński to Piotrek Koszuliński

comment:8 Changed 4 years ago by 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:

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

comment:9 Changed 4 years ago by 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.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 4 years ago by 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

comment:11 in reply to:  9 ; Changed 4 years ago by 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.

comment:12 in reply to:  11 Changed 4 years ago by 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).

comment:13 in reply to:  10 Changed 4 years ago by 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.

comment:14 Changed 4 years ago by 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?

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:15 Changed 4 years ago by 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.

comment:16 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by Joel

Cc: joel.peltonen@… added

cc

comment:18 Changed 4 years ago by Olek Nowodziński

Owner: changed from Piotrek Koszuliński to Olek Nowodziński
Status: assignedreview

I pushed a minor commit to dev.

comment:19 Changed 4 years ago by Olek Nowodziński

Status: reviewreview_passed

...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 ;)

comment:20 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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.

comment:21 Changed 4 years ago by Piotrek Koszuliński

Description: modified (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy