Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#11861 closed Bug (fixed)

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

Reported by: a.nowodzinski Owned by: a.nowodzinski
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 3.0
Keywords: Webkit Blink Cc: joel.peltonen@…

Description (last modified by Reinmar)

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 3 years ago by a.nowodzinski

  • Description modified (diff)

comment:2 Changed 3 years ago by a.nowodzinski

  • Owner set to a.nowodzinski
  • Status changed from new to assigned

comment:3 Changed 3 years ago by Reinmar

cc

comment:4 Changed 3 years ago by a.nowodzinski

  • Keywords Webkit Blink added
  • Summary changed from [Webkit] Span elements created while joining adjacent elements to [Blink/Webkit] Span elements created while joining adjacent elements

comment:5 Changed 3 years ago by a.nowodzinski

  • Description modified (diff)

comment:6 Changed 3 years ago by a.nowodzinski

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

comment:7 Changed 3 years ago by Reinmar

  • Owner changed from a.nowodzinski to Reinmar

comment:8 Changed 3 years ago by Reinmar

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 follow-up: Changed 3 years ago by Reinmar

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 3 years ago by Reinmar (previous) (diff)

comment:10 follow-up: Changed 3 years ago by m.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 ; follow-up: Changed 3 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 3 years ago by Reinmar

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 3 years ago by Reinmar

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 3 years ago by a.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 3 years ago by a.delura (previous) (diff)

comment:15 Changed 3 years ago by Reinmar

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 3 years ago by Reinmar

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 3 years ago by req

  • Cc joel.peltonen@… added

cc

comment:18 Changed 3 years ago by a.nowodzinski

  • Owner changed from Reinmar to a.nowodzinski
  • Status changed from assigned to review

I pushed a minor commit to dev.

comment:19 Changed 3 years ago by a.nowodzinski

  • Status changed from review to review_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 3 years ago by Reinmar

  • Resolution set to fixed
  • Status changed from review_passed to closed

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 3 years ago by Reinmar

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