Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#12621 closed Bug (fixed)

Can't remove Bold/Italic/Underline in empty line.

Reported by: Jakub Ś Owned by: Piotrek Koszuliński
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.4.6
Component: Core : Styles Version: 4.4.5
Keywords: Blink Webkit Support Cc:

Description (last modified by Piotrek Koszuliński)

This is happening mainly in Mac but some variation can be observed in Windows.

  1. Open replacebycode sample
  2. Put cursor at the end of first paragraph July 21 at 02:56 UTC.^ and press enter.
  3. Press Command+I or Ctrl+I
  4. Press Command+I or Ctrl+I again and type few letters.

Results: On Mac it is impossible to remove simple style. On Windows button is still highlighted, elements path shows 'em' but when you start typing you get normal text.

Problem can be reproduced in Blink and Webkit browsers from CKEditor 4.4.5.

This issue causes #12631.

Change History (12)

comment:1 Changed 9 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 9 years ago by Piotrek Koszuliński

It might look like a regression, but actually it's not. It's an older bug in the styles system (I might have reported it some time ago).

On 4.4.4 (an earlier versions), when you do that TC, there is an empty inline element left after you press the style keystroke for the 2nd time - <p><strong></strong><br></p>. It should be removed by the styles system when removing the style, but it's not.

In 4.4.5 in #12423 we changed a behaviour of a method important for the styles and selection systems (so it's hard to say what it could affect). And I think that this change might actually be good, because it aligned the method behaviour on Webkit/Blink to Firefox. And on Firefox this problems isn't reproducible. This may mean that there's some other part of code wrong on Chrome/Blink (e.g. the code that affects the getStartElement() method).

Anyway, if we fix the issue in the styles system, then this issue (reported here) won't be reproducible. However, the getStartElement() behaviour must be checked too.

Version 0, edited 9 years ago by Piotrek Koszuliński (next)

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

It might look like a regression, but actually it's not. It's an older bug in the styles system (I might have reported it some time ago).

Nope, I haven't reported it.

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

Component: GeneralCore : Styles

comment:5 Changed 9 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:6 Changed 9 years ago by Piotrek Koszuliński

Priority: NormalHigh

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

I have some more details about this issue (examples for removing selection from <p>foo<strong>\u200b{}</strong></p>):

  1. Result of style.removeFromRange is correct. The range is placed in: <p>foo<strong>\u200b</strong>[]</p>.
  2. When selection.selectRanges is executed it checks whether the selection needs a filling char. Note: by "correct" I mean that it has not changed recently, although it is a bug that the removeFromRange() method does not remove the empty inline element.
  3. In this case the algorithm (implemented by the rangeRequiresFix function) should find that the range is placed right after <strong>\u200b</strong> and return true - the range requires the filling char.
  4. For some odd reason the algorithm uses the walker.invisible() method. Apparently it did return false for the <strong>\u200b</strong> element. But it does not do this anymore.

Plan:

  1. We need tests for walker.invisible() to verify that behaviour of this method really changed for the above scenario. The expected result is true, so if the method returns this value, then it should not be touched.
  2. We need to fix the rangeRequiresFix function which should not be based on previous quirky behaviour of the walker.invisible() method.
Last edited 9 years ago by Piotrek Koszuliński (previous) (diff)

comment:8 Changed 9 years ago by Piotrek Koszuliński

Edit: I updated the comment:7 because I messed up walker.invisible()'s return values.

In git:d31d2cf6 I added a test that verifies the case described in comment:7. Before #12423 it was returning false and now it returns true. Therefore, the behaviour of rangeRequiresFix must be fixed, because it cannot rely on walker.invisble().

comment:9 Changed 9 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedreview

Pushed branch:t/12621.

I needed to ignore the new tests on IE8, because it doesn't pass them. Am I right that it's impossible to set selection at that position on IE8?

comment:10 Changed 9 years ago by Piotrek Koszuliński

The patch fixes also #12631.

comment:11 Changed 9 years ago by Artur Delura

Resolution: fixed
Status: reviewclosed

Fixed on master with git:457224e.

comment:12 Changed 9 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.6
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy