Changing the same font styles should not lead to nesting them

[Jakub Ś]

Steps to reproduce:

1. Try Chrome because it gives nice visual result for this issue.
2. Open replcebycode sample and remove editor contents
3. Change font size to 36 and type some text
4. Change font size to 10 and type some text.
5. At the moment cursor has 36 height
6. Press enter and type some text

Result: Cursor is still big because both spans were transferred to second line. The general problem here is that spans got nested.

When typing with one inline style value (e.g. 30px) and then changing this value to something else (e.g. 10px), first span should be closed and other opened.

NOTES:

• I think this should only work for same style properties because otherwise it wouldn't be possible to have e.g. text with size 36px and red background.

[Piotrek Koszuliński]

Theoretically, the most simple workaround would be to try remove duplicated styles in the new block created on enter. However, this is hard for few reasons - first of all, the code would need to land in enterkey plugin which simply does not handle such things, so it would need to reuse parts of style system... simply wrong. Edit: see comment:3 why this is a totally incorrect idea.

Therefore it's better to handle this issue more generally. First thing to notice is that the problem is earlier - when a second span with the same style is created inside the previous one. If we avoid this, we will avoid the problem with enter.

How to do that? My idea is that what we want to do when applying font size for the second time, is removing the currently existing font size style. To do that we could use the removeStyleFromRange feature from style system. So far so good. Now, the problem starts with a styles matching algorithm because font-size:10px is not the same as font-size:30px. So we need a wildcard as a value. This is not supported, but I hope it won't be so hard to implement. When we have this part, we are at home.

[Piotrek Koszuliński]

BTW. I'm slightly worried that this may be a too big change for a minor release. We'll make a decision later on.

[Frederico Caldeira Knabben]

Replying to Reinmar:

Theoretically, the most simple workaround would be to try remove duplicated styles in the new block created on enter.

Note that this doesn't depend on ENTER. It happens as well if the line gets too long and automatically wraps to a second line.

[Artur Delura]

From my investigation everything happened in ​here. Because range for which we want to apply style is collapsed, there is created new span with style and appended right in selection. So we can introduce new parameter (to keep backward compability) which will be a wildcard or something similar, and if that wildcard will match style it will be applied to selection parent node instead of making new one and appending it.

So in practice we have following content with selection:

<p><span style="font-size:36">Hello[]</span></p>

then, we are executing applyStyle method with font-size set to 10.

As result should be:

<p><span style="font-size:10">Hello[]</span></p>

As result should be:

<p><span style="font-size:36">Hello</span><span style="font-size:10" >[]</span></p>

Instead of(current behaviour):

<p><span style="font-size:36">Hello<span style="font-size:10">[]</span></span></p>

---

For:

<p><span style="font-size:36">He[ll]o</span></p>

Then applyStyle with font-size:10px.

Result:

<p><span style="font-size:10">He[ll]o</span></p>

Result:

<p><span style="font-size:36">He</span><span style="font-size:10">[ll]</span><span style="font-size:36">o</span></p>

---

For:

<p><span style="font-size:36;background:red;">Hello[]</span></p>

Then applyStyle with font-size:10px

Result:

<p><span style="font-size:10;background:red;">Hello[]</span></p>

Result:

<p><span style="font-size:10;background:red;">Hello</span><span style="font-size:36;background:red">[]</span></p>

[Marek Lewandowski]

Guys, this case is tough.

Both of you @a.delura and @Reinmar are proposing the same solution, which would:

• search existing formatting element (span) and change its font size to the new one.

I'm afraid that this will confuse lots of casual users. With presented fix approach, they won't be able to select some part of a line and change its size.

Lets consider following case:

<p><span style="font-size:48px">foo [bar] baz</span></p>

And I want my bar to be at diffrent size, lets say 72. If you transform closest "formatting span" then, you take away the ability to format particular parts.

Splitting elements

Other possible solution, would involve splitting the span:

<p><span style="font-size:48px">foo [bar] baz</span></p>

To:

<p><span style="font-size:48px">foo </span><span style="font-size:72px">[bar]</span><span style="font-size:48px"> baz</span></p>

But it creates a lot of extra markup.

Getting computed styles

Eventually we might detect new line creation, and instead of moving full "formatting span" structure/tree we might merge them, so the styling attributes from closest parents would be used.

Eg.

<p><span style="font-size:48px;text-decoration: underline;">foo bar <span style="font-size: 9px">baz^</span></span></p>

after new line would begin with:

<p><span style="font-size:48px;text-decoration: underline;">foo bar <span style="font-size: 9px">baz</span></span></p>
<p><span style="font-size:9px;text-decoration: underline;">^</span></p>

[Frederico Caldeira Knabben]

Replying to a.delura:

As m.lewandowski said, the styling should apply the selection, exclusively. Changing a parent element is wrong.

[Frederico Caldeira Knabben]

Replying to m.lewandowski:

Splitting elements

Other possible solution, would involve splitting the span:

<p><span style="font-size:48px">foo [bar] baz</span></p>

To:

<p><span style="font-size:48px">foo </span><span style="font-size:72px">[bar]</span><span style="font-size:48px"> baz</span></p>

But it creates a lot of extra markup.

I don't see any extra markup here. It's the exact markup we need for this case and it is the only way for it IMHO.

We don't need any wildcard here. I mean, it would work with that as well, but we don't needed a new feature for it. In the provided case it is clear that the styles are interchangeable, so it would be just a matter of searching on parents for elements and styles that match the applied style and break them accordingly.

[Piotrek Koszuliński]

We were wondering together with Fred why aren't the style's overrides used in such case when applying style:

<p><span style="font-size:48px">foo [bar] baz</span></p>

We thought that they aren't used at all when applying it, but it turned out that they are used pretty well when newly created style includes the existing one inside it:

<p>foo [bar <span style="font-size:48px">baz</span> bar] foo</p>

And overrides with style's inline style wildcard work as well:

CKEDITOR.config.fontSize_style = {
	element: 'span',
	styles: { 'font-size': '#(size)' },
	overrides: [
		{
			element: 'span', styles: { 'font-size': null }
		}
	]
};

So I need to do one thing - start using overrides on the ancestors. And this unfortunately may not be so easy. Unless... before applying a style, just as I proposed, we'll remove it from the current selection.

[Piotrek Koszuliński]

And so we've got a prototype - ​branch:t/12403. How do you like it?

[Frederico Caldeira Knabben]

Replying to Reinmar:

And so we've got a prototype - ​branch:t/12403. How do you like it?

I like the idea a lot. That's the way to go for it, because it allows full control on the behavior of it.

In the other hand, there are no tests around it, so we should not go ahead with it like that. Note that we need tests not only for the "split" case, but also for the cases where we don't want it to split.

For instance, I found an issue:

1. In the dev samples, double click the bolded "Apollo" word in the first paragraph (not the title, and without "11"). It gonna get selected.
2. Click the bold button to remote it. It works.
3. Click the bold button to apply it again. It works.
4. Click the bold button to remove it again. It DOESN'T work.

[Piotrek Koszuliński]

In the other hand, there are no tests around it, so we should not go ahead with it like that. Note that we need tests not only for the "split" case, but also for the cases where we don't want it to split.

Of course. I just had a few minutes, so I wanted to check whether this will be possible at all. The final patch will contain necessary tests, including false positives.

[Piotrek Koszuliński]

I pushed ​branch:t/12403b with more than hundred new tests for inline styles. The results are not good so far.

There are numerous failing tests in ​http://tests.ckeditor.dev:1030/tests/core/style/applyremove

The last 4 failing cases can be ignored now - they are bugs to be fixed.

The problem is with first 10 broken cases. To better understand what's happening see tests added in ​git:f61a875a. They are paired to broken or uncool cases (commented with TODO) and explain why the style application fails.

Furthermore, there are dozens of other scenarios which I added in earlier commits and which are often commented that the result of style application/removal is highly dubious. Those tests pass (all tests before ​git:5d473e pass), but the results are wrong and inconsistent.

[Piotrek Koszuliński]

I pushed ​branch:t/12403b with a state at the point when I understood that this is a road to hell. I found out that styles property in overrides' definitions is totally ignored. getOverrides checks only attributes and that's also the conculsion from tests, although that's not the only bug I encountered.

This is the point when we have to find another solution, because we don't have 2-3 weeks to treat this problem holistically.

However, let's not lose what I did so far. I pushed ​branch:t/12403c with tests plus two fixes for the dev code.

[Piotrek Koszuliński]

I pushed a font plugin specific solution to ​branch:t/12403d which... is nearly perfect. But there's a big problem. When we remove inline style from collapsed selection instead of splitting the inline element, we remove totally remove it. See the failed test on ​http://tests.ckeditor.dev:1030/tests/plugins/font/font

This means that we actually need a similar functionality which I achieved by introducing ​this argument in the previous attempt to fix this issue. Unfortunately, now it would need to become a public option or we would need a separate method. Also, if I recall correctly, that argument wasn't working when selection was at the element boundary (it was leaving empty siblings).

Unfortunately, I don't see other options than polishing that argument and exposing it as editor.removeStyle's argument or in some new style's method - e.g. style.splitElement( range ). WDYT, Fred?

[Frederico Caldeira Knabben]

Replying to Reinmar:

I pushed a font plugin specific solution to ​branch:t/12403d which... is nearly perfect.

It's simple and I like what I see. It fixed the reported issue fully.

But there's a big problem. When we remove inline style from collapsed selection instead of splitting the inline element, we remove totally remove it. See the failed test on ​http://tests.ckeditor.dev:1030/tests/plugins/font/font

This is exactly like we have it for all styles, like bold. This is the current behavior though and it is not part of this ticket, so we don't have to fix it to be able to close this one.

[Piotrek Koszuliński]

Pushed ​branch:t/12403d with the problem which I mentioned in comment:18 solved directly in the font plugin.

[Artur Delura]

Browser: Chrome

0. Open editor and clear it's content.
1. Type some text: abcdefg
2. Put caret here: ab^cdefg
3. Select font size: 72
4. Select font size: 24

Actual result: Caret size is like 72.

Expected result: Caret size is like 24.

[Frederico Caldeira Knabben]

I've just pushed yet another branch, t/12403e, which does the same as t/12403d but reusing logic already present in the style system. For that, I've added a new options.split parameter to all remove methods in the styles system, which specifies that the removal should split in case of collapsed range.

I've also reverted minor changes to the font plugin so the diff with master is now pretty minimal.

Hope we still have time to consider this.

[Frederico Caldeira Knabben]

Replying to a.delura:

Browser: Chrome

0. Open editor and clear it's content.
1. Type some text: abcdefg
2. Put caret here: ab^cdefg
3. Select font size: 72
4. Select font size: 24

Actual result: Caret size is like 72.

Expected result: Caret size is like 24.

This seems to be a side effect on Chrome but it doesn't bring any final impact, because the 72px span is not outputted in the html anyway. It's a minor annoyance that we should be ok to accept at this stage.

[Piotrek Koszuliński]

I pushed additional commits (tests + fix) to ​branch:t/12403d with solution for comment:22. Good catch, Artur. While this is a minor annoyance, I think it's actually a pretty common scenario, so not fixing this may significantly affects the reception of this entire patch.

[Piotrek Koszuliński]

I reported #12687 as a follow-up for this ticket. I'll move ​branch:t/12403e there and perhaps the earlier branches as well.

[Piotrek Koszuliński]

I extracted the improvements I made while working on the first version of the patch (​http://dev.ckeditor.com/ticket/12403#comment:16) to #12688 and merged them to master.

[Piotrek Koszuliński]

Fixed on master with ​git:7471d06.