Ticket #3599 (closed Bug: fixed)

Opened 6 years ago

Last modified 5 years ago

Issue with nested Font size and background color

Reported by: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: Core : Styles Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Review+ Cc: alfonsoml

Description

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>

Attachments

3599.patch (592 bytes) - added by garry.yao 5 years ago.
3599_2.patch (3.4 KB) - added by garry.yao 5 years ago.
3599_3.patch (2.0 KB) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 6 years ago by tobiasz.cudnik

  • Status changed from new to assigned
  • Keywords Confirmed added
  • Owner set to 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 ?

comment:2 Changed 6 years ago by garry.yao

How about:

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

comment:3 Changed 6 years ago by alfonsoml

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

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

comment:5 follow-up: ↓ 6 Changed 6 years ago by fredck

  • Milestone changed from CKEditor 3.0 to CKEditor 3.1

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.

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

comment:7 Changed 5 years ago by tobiasz.cudnik

  • Keywords Discussion added

comment:8 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.1 to CKEditor 3.2

comment:9 Changed 5 years ago by fredck

  • Owner tobiasz.cudnik deleted
  • Status changed from assigned to new
  • Milestone changed from CKEditor 3.2 to CKEditor 3.3

Changed 5 years ago by garry.yao

comment:10 Changed 5 years ago by garry.yao

  • Keywords Review? added; Discussion removed
  • Status changed from new to assigned
  • Version set to SVN (CKEditor)
  • Owner set to garry.yao

Thanks to #4772 which make the fix pretty simple.

comment:11 Changed 5 years ago by alfonsoml

  • Keywords Review+ added; Review? removed

comment:12 Changed 5 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [5221].

comment:13 Changed 5 years ago by alfonsoml

  • Status changed from closed to reopened
  • Resolution fixed deleted

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.

comment:14 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review+ removed

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.

Changed 5 years ago by garry.yao

comment:15 Changed 5 years ago by garry.yao

  • Cc alfonsoml added

Please give a continual review to this problem.

comment:16 Changed 5 years ago by alfonsoml

  • Keywords Review+ added; Review? removed

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-- )

comment:17 Changed 5 years ago by garry.yao

  • Status changed from reopened to closed
  • Resolution set to fixed

Thanks for the prompt.
Fixed with [5251].

comment:18 Changed 5 years ago by garry.yao

  • Status changed from closed to reopened
  • Resolution fixed deleted

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].

Changed 5 years ago by garry.yao

comment:19 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review+ removed

Proposing another approach for fixing the bug raised by alfonsoml.

comment:20 Changed 5 years ago by fredck

  • Keywords Review+ added; Review? removed

Please do not keep re-opening already closed tickets.

comment:21 Changed 5 years ago by garry.yao

  • Status changed from reopened to closed
  • Resolution set to fixed

Fixed with [5348] and [5350].

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