Opened 4 years ago

Closed 4 years ago

#12403 closed Bug (fixed)

Changing the same font styles should not lead to nesting them

Reported by: Jakub Ś Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.6
Component: Core : Styles Version: 4.0
Keywords: Support Cc:

Description

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.

Change History (30)

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

Milestone: CKEditor 4.4.5
Status: newconfirmed

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 occurs 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 and line breaks.

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.

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

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

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

Last edited 4 years ago by Frederico Caldeira Knabben (previous) (diff)

comment:4 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:5 Changed 4 years ago by 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>
Last edited 4 years ago by Artur Delura (previous) (diff)

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

comment:7 in reply to:  5 Changed 4 years ago by Frederico Caldeira Knabben

Replying to a.delura:

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

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

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

Owner: changed from Artur Delura to Piotrek Koszuliński

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

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

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

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

Milestone: CKEditor 4.4.5CKEditor 4.4.6

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

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

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

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

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

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

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

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

Component: GeneralCore : Styles

comment:20 Changed 4 years ago by Wiktor Walc

Keywords: Support added

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

Status: assignedreview

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

comment:22 Changed 4 years ago by Artur Delura

Browser: Chrome

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

Actual result: Caret size is like 72.

Expected result: Caret size is like 24.

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

comment:24 in reply to:  22 Changed 4 years ago by Frederico Caldeira Knabben

Replying to a.delura:

Browser: Chrome

  1. Open editor and clear it's content.
  2. Type some text: abcdefg
  3. Put caret here: ab^cdefg
  4. Select font size: 72
  5. 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.

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

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

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

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

Summary: Changing same inline styles should not lead to nesting them.Changing font style should not lead to nesting them

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

Summary: Changing font style should not lead to nesting themChanging the same font styles should not lead to nesting them

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:7471d06.

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