Highlight works fine with the font size plugin

[pomek]

Suggested merge commit message (convention)

Fix: Highlight plugin will render the highlighted text properly when it's combined with the FontSize plugin. Closes ckeditor/ckeditor5#2605.

[pomek]

I forgot to do some internal checks. I'm giving R- now.

[coveralls]

Coverage remained the same at 100.0% when pulling fb1ed3f on t/17 into 8d4298e on master.

[pomek]

Should be fine now. Let's wait for @coveralls report.

[jodator]

Looks like it is working:

[jodator]

LGTM

[Reinmar]

must stick

[Reinmar]

Shouldn't we increase the priority of the font-size feature instead of patching the highlight? What about font-size + any other feature which changes text background?

I think that this is a very similar case than the link feature. It got itself the highest priority.

cc @pjasiun

[jodator]

Shouldn't we increase the priority of the font-size feature

Did you mean to lower font-size priority? Here we had higher priority for Highlight so to make it work in Font feature I assume that it have to have lower priority then Highlight (or other features) to properly change wrapped elements - ie be the latest one to convert.

[pjasiun]

Shouldn't we increase the priority of the font-size feature instead of patching the highlight? What about font-size + any other feature which changes text background?

I think that this is a very similar case than the link feature. It got itself the highest priority.

cc @pjasiun

TBH, I don't know the context, but if a problem can be solved by view attribute elements priorities we should do it, instead of hacking.

[Reinmar]

The latest one to convert will be the link feature.

Here, we have to decide whether the highlight feature requires a proper treatment. Or whether it's the font-size feature.

My point was that I'm unsure whether highlight has to be the innermost tag. Perhaps it's the font-size which should be close-to-outermost. There's, in fact, a difference in these two solutions. This PR will make the highlight tags rendered as close to the text as possible, which also involves moving it inside <strong>, <i>, etc. Does that make sense?

[Reinmar]

TBH, I don't know the context, but if a problem can be solved by view attribute elements priorities we should do it, instead of hacking.

That's exactly what's proposed ;) The question is – should we increase highlight's priority or decrease font-size's priority?

[jodator]

I'm thinking that what causes problems here is font-size changing feature + background coloring feature combo.

As for now I can see:

• font-size + highlight
• font-size + link-highlight (the link highlight has the highest priority of them all)

In future we might have font-background plugin that will have to behave similarly to highlight. So probably making the font-size outer most (lower its priority) will be more future proof. WDYT?

[pjasiun]

As you said:

this is a very similar case than the link feature. It got itself the highest priority.

So in this case highlight should also get a higher priority. It should work well with all features which change formatting.

[Reinmar]

In future we might have font-background plugin that will have to behave similarly to highlight. So probably making the font-size outer most (lower its priority) will be more future proof. WDYT?

👍

[Reinmar]

We agreed to first add the ability to specify the view attribute element priority in two-way converters: https://github.com/ckeditor/ckeditor5-engine/issues/1408.

[pomek]

After introducing https://github.com/ckeditor/ckeditor5-engine/issues/1408, this PR is no longer valid. See ckeditor/ckeditor5-font#16.