Issue with nested Font size and background color

[Damian]

It seems that there is a special case required to be handled by the font style features. The issue is that when the size of a default text is set to a large size e.g. 72px, then a background color is applied to the same selection, the background style will not have the right font-size set. This results in the background color appearing as a narrow band behind the large font.
E.g.

<span style="background-color: rgb(238, 130, 238);"><span style="font-size: 72px;">Test</span></span>

[Tobiasz Cudnik]

I'm proposing use of inline-block.

For every browser other than FF2 (including IE6):

<p>
	<span style="display: inline-block; background-color: rgb(238,130,238)"><span style="font-size: 72px">Test</span></span>inline</p>

For FF2

<p>
	<span style="display: -moz-box; background-color: rgb(238, 130, 238);"><span style="font-size: 72px;">Test</span></span> inline</p>

Does anyone see anything agains it ?

[Garry Yao]

How about:

<span style="font-size: 72px;"><span style="background-color: rgb(238, 130, 238);">Test</span></span>

[Alfonso Martínez de Lizarrondo]

The best solution would be really to fix #2203, so the output is

<span style="background-color: rgb(238, 130, 238); font-size: 72px;">Test</span>

Anything else is adding special cases just for this ticket but other situations will remain broken, for example look at #1646

[Tobiasz Cudnik]

Both solutions from garry.yao and alfonsoml includes scanning tree to the root and cloning the background property for each font-affected nodes. When background is changed in some node it will be needed to scan tree from modified node to each leaf and change the actually modified background to new one.

While inline-block is css2.1-correct way of dealing with such issues and doesn't require any of mentioned operations.

[Frederico Caldeira Knabben]

There is no real bug in the editor, but in the browsers (and probably the HTML specifications). After all, we're talking about an HTML feature. Of course, we understand the end user impact of it, so we should try finding a way to workaround it.

@garry.yao: easy to say, difficult to have. But it looks like the only solution, if we would ever try to fix this. The idea would be having a special stylying case (possibly identified by the style definition), which specifies that the style is to be applied to text nodes and self closing inline elements (<img>) only. In this way it would not embrace existing tags.

@alfonsoml: consider that the font style may be available in any kind of tag, including <font> or even specially styled elements, <strong class="my_strong_class">.

@tobiasz: we're talking about the content generated by the editor, so we cannot have different solutions for different browsers. We may consider this as a temporary fix, ignoring FF2.

[Tobiasz Cudnik]

Replying to fredck:

@tobiasz: we're talking about the content generated by the editor, so we cannot have different solutions for different browsers. We may consider this as a temporary fix, ignoring FF2.

Maybe we can use this as solution:

<p>
  <span style="display: -moz-box; background-color: rgb(238,130,238)">
    <span style="display: inline-block; background-color: rgb(238,130,238)">
      <span style="font-size: 72px">Test</span>
    </span>
  </span>
  inline
</p>

We could use it only for content generated by the editor, like you sad. The editor itself will have browser-specific solution and only one SPAN.

[Garry Yao]

Thanks to #4772 which make the fix pretty simple.

[Garry Yao]

Fixed with [5221].

[Alfonso Martínez de Lizarrondo]

Now when you apply again a background color it will create a new span, so if you test several times you end with

<span style="background-color: rgb(255, 0, 0);"><span style="background-color: rgb(47, 79, 79);"><span style="background-color: rgb(64, 224, 208);">using </span></span></span>

and previously it kept only the latest change.

I'm not sure if the new problem is worth the fix or we should leave it this way and wait for the "span compaction" in a future fix.

[Garry Yao]

Excellent review! The customizable style rule functionality made this bug, where similar style (same background-color property with different values in the above sample) couldn't get removed before applying the new style.
Fortunately this situation bring by this new approach is still under our control, with my new patch. While it has successfully passed all design tests of the styles plugin, we should still test it throughly.

[Garry Yao]

Please give a continual review to this problem.

[Alfonso Martínez de Lizarrondo]

Remember to adjust the indenting to one tab and put the spaces for "if( " statements.

Instead of candidates1 and candidates2 you could use elementCandidates and overrideCandidates names, and avoid the count1 and 2 variables by doing the loops downwards:

for ( i = elementCandidates.length - 1 ; i >= 0 ; i-- )

[Garry Yao]

Thanks for the prompt.
Fixed with [5251].

[Garry Yao]

We've seen regression at #5462 and #5472, which proves [5251] is much error prone, we decide to revert it and the related [5343] with [5347].

[Garry Yao]

Proposing another approach for fixing the bug raised by alfonsoml.

[Frederico Caldeira Knabben]

Please do not keep re-opening already closed tickets.

[Garry Yao]

Fixed with [5348] and [5350].