Bug 121415 – The size of the background colour is not properly adjusted when the font size is being changed

• Status: NEW
• Component: HTML Editing

[Artur Moryc]

When the background colour is set to a specific colour but the font size is being changed it results in that the size of the background colour doesn't fit to any of the font sizes.

[Alexey Proskuryakov]

Do you have an example web site, or better yet a test case that you could attach here?

[Artur Moryc]

Created attachment 211876 [details]
Test to validate the background combined with the font sizes 

Steps to reproduce the bug:

In the attached file (FontBackgroundTest.html) perform the following actions:

1. Set the font size to Huge
2. Set the beckground color 
3. Insert text
4. Change the font size ( different than the previous one) 

Observed result:
The background color doesn't fit to any font sizes.

Expected result:
The background color can adjust respectively to the font sizes

[Artur Moryc]

Created attachment 211877 [details]
Expected result for the attached test

[Alexey Proskuryakov]

Thank you, I can reproduce some clearly incorrect behaviors with this test case.

[Radar WebKit Bug Importer]

<rdar://problem/15012836>

[Artur Moryc]

Created attachment 214466 [details]
Propesed solution for the font size change and backgroundcolor issue   

The proposed fix should be looked closer for the default font size case. In this solution the default font size case is considered as a text node with a span backgroundcolor parent.
The problem appears if the sibling of the default fontsize node has the font size smaller than default since its backgroundcolor is overwritten by the parent backgroundcolor.
However, If the default fontsize node would be considered as a span node with backgroundcolor the problem seem to be solved.

[Ryosuke Niwa]

Comment on attachment 214466 [details]
Propesed solution for the font size change and backgroundcolor issue   

View in context: https://bugs.webkit.org/attachment.cgi?id=214466&action=review

Thanks for the patch but I don't think the code change is sound. For one, it introduces a security bug.
It appears to me that we just need to be more careful about where we add span/font elements to specify the font if there is already background color specified somewhere.
We should be exposing the ability to compute the effective background color in EditingStyle intend of manually computing it here.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1478
> +                if (node->hasTagName(fontTag) && node->previousSibling() && node->previousSibling()->hasTagName(fontTag))
> +                    nodeFromWhichBackgrounColorCoppied = node->previousSibling()->firstChild();

What are we trying to accomplish with this code?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1486
> +                String backgroundColorValue = inlineStyle->getPropertyValue(CSSPropertyBackgroundColor);
> +                if (!backgroundColorValue.isEmpty() && !isBackgroundcolorSet) {

r- because this code only checks inline style but background color may also been specified by a CSS rule.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1488
> +                    spamElement->setAttribute(styleAttr, newCSSStyle);

r- because this breaks undo. We must use setNodeAttribute or alike that uses EditCommand to create an undo step.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1493
> +                if (node->hasTagName(spanTag) && node->firstChild()->hasTagName(fontTag) && node->parentNode() && node->parentNode() != editableRoot)

node is a raw pointer and it could have been removed by some script while calling surroundNodeRangeWithElement.
r- because this code introduces a use-after-free bug.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1494
> +                    removeNodePreservingChildren(toHTMLElement(node));

r- because node can have other styles and removing this node could alter the style. e.g. decedent selectors such as "a b" will fail to match "b" if we removed "a".

[Artur Moryc]

Created attachment 216387 [details]
Solution improvment for the font size change and backgroundcolor issue

Thank you for valuable remarks which helped to brush up the code. Functionality of backgroundColorInEffect has been used to find a parent node with backgroundcolor.
Two charts are attached that show redundant nodes and solution without them.

[Artur Moryc]

Created attachment 216388 [details]
chart with redundant nodes

Chart which shows redundant parent nodes.

[Artur Moryc]

Created attachment 216389 [details]
chart with optimzed nodes

In this case redundant nodes have been removed

[Ryosuke Niwa]

Comment on attachment 216387 [details]
Solution improvment for the font size change and backgroundcolor issue

View in context: https://bugs.webkit.org/attachment.cgi?id=216387&action=review

Thanks for the updated patch but we need more detailed descriptions in the change log.

> LayoutTests/editing/inserting/default-fontsize-inner-span-background-expected.txt:5
> +|   size="7"
> +|   style="background-color: rgb(255, 0, 0);"
> +|   <span>
> +|     style="background-color: rgb(255, 0, 0);"

I don't think we should have the inner span. r- because this markup is too verbose.

> LayoutTests/editing/inserting/default-fontsize-inner-span-background.html:20
> +        minFontSize = 1;
> +        maxFontSize = 7;
> +        digitsInBlock = 3;
> +        fontColor = '#F00';

Please declare these variables with either "var" or "const".

> LayoutTests/editing/inserting/default-fontsize-inner-span-background.html:44
> +<p>This Layout Test checks whether the height of background combined with font tag is correct. The test successes when background color is red and it exactly matches the font size.</p>

Please put this in Markup.description.
You can do Markup.description(document.querySelector('p').textContent);

> LayoutTests/editing/inserting/default-fontsize-inner-span-background.html:50
> +<script>
> +var div = document.getElementById("test");
> +div.focus();
> +changeFontSizeForOneBackgroundcolorTest();
> +</script>

Why is this script element inside #test?

> LayoutTests/editing/inserting/default-fontsize-inner-span-background.html:55
> +<script>
> +forceExpectedResult('#F00');
> +var div = document.getElementById("test");
> +Markup.dump(div);

What are we testing here?

> LayoutTests/editing/inserting/default-fontsize-outer-span-background-expected.txt:7
> +| <span>
> +|   style="background-color: rgb(255, 0, 0);"
> +|   <font>
> +|     size="7"
> +|     <span>
> +|       style="background-color: rgb(255, 0, 0);"
> +|       "777"

Why do we have background-color in the outer span here? Also, why was not font modified to specify the background color?

> Source/WebCore/ChangeLog:9
> +        The bug is observed in the editing field while the font size is being changed for one background
> +        color then the size of the background is not properly adjusted to different font sizes

Note that font size can be specified by font-size CSS property as well as font element.

> Source/WebCore/ChangeLog:12
> +        The proposed solution is to find background color of the parent node and apply
> +        such style inside of the font tag

Please don't do that.  That'll increase the size of markup very quickly, ending up setting background-color on virtually every single node.
We used to have this problem, and we've worked hard to fix that.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1467
> +        RefPtr<MutableStylePropertySet> inlineStyle = copyStyleOrCreateEmpty(toHTMLElement(spanElement.get())->inlineStyle());

We shouldn't always create a span.
When there is an appropriate span or font element, we should simply add an inline style there.
Also, we should be using EditingStyle instead of manually manipulating MutableStylePropertySet.
r- because of these two problems.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1477
> +                if (node->hasTagName(spanTag) && node->firstChild()->hasTagName(fontTag) && node->parentNode() && node->parentNode() != editableRoot)
> +                    removeNodePreservingChildren(toHTMLElement(node));

I'm not sure if I wasn't clear with my previous comment but this function should NEVER remove an element.
That'll cause crashes and infinite loops elsewhere.

> Source/WebCore/editing/EditingStyle.cpp:1347
> +RefPtr<CSSValue> EditingStyle::obtainBackgroundcolorIfNeeded(const Node *node)

What does IfNeeded in this context?

> Source/WebCore/editing/EditingStyle.cpp:1351
> +    if (node && isFontTagNode(node->parentNode()))
> +        value = backgroundColorInEffect(const_cast<Node*>(node));

Why is font element special?  The intent/purpose of this function needs to clearly explained in the change log.
r- because of this.

> Source/WebCore/editing/htmlediting.cpp:162
> +bool isFontTagNode(const Node *node)
> +{
> +    return node && node->hasTagName(fontTag);
> +}
> +

We already have isHTMLFontElement.  r- because this function duplicates that functionality.

[Artur Moryc]

Created attachment 216786 [details]
Solution enhancement for the font size change and backgroundcolor issue

Thank you very much for very comprehensive comments and explanations to the previous patches.  
In this patch background color is added to the font tag inline. However, still there are two things that can be pointed out:
1) As it can be seen in the attach chart there is still no difference between the small and normal font size. It happens since the span tag with style background color may overwrite the background color for the font tag with small size font (please have a look at the attached chart) 
2) After adding a text with default font size it results in that next nodes with different font sizes are added as child nodes. It also may influence on overwriting the beckgroundcolor for small font size.

[Artur Moryc]

Created attachment 216787 [details]
The results of the provided fix

[Darin Adler]

Comment on attachment 216786 [details]
Solution enhancement for the font size change and backgroundcolor issue

View in context: https://bugs.webkit.org/attachment.cgi?id=216786&action=review

> Source/WebCore/editing/ApplyStyleCommand.cpp:1467
> +                newCSSStyle.append(styleChange.cssStyle());

Does something guarantee that cssStyle ends with a semicolon if it’s not empty? If not, don’t we need to add a semicolon?

What if the style attribute string already has background-color in it? Will we just keep adding more? Could we add test cases that cover this?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1468
> +                newCSSStyle.append("background-color:" + backgroundColorCSSValue->cssText());

This is wrong. If we already have a string builder we should append one string at a time. This way does unnecessary allocation. Something like this:

    newCSSStyle.appendLiteral(“background-color:”);
    newCSSStyle.append(backgroundColorCSSValue->cssText());

Not sure I have the syntax exactly right.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1469
> +                setNodeAttribute(toHTMLElement(fontElement.get()), styleAttr, newCSSStyle.toString());

Code above is just calling fontElement->setAttribute. Why does this code call setNodeAttribute instead?

> Source/WebCore/editing/EditingStyle.cpp:1346
> +PassRefPtr<CSSValue> EditingStyle::backgroundColorInEffect(Node* node)

It’s ugly to have this return a CSSValue, although that’s not new to this patch. For internal use in WebKit we should not be using DOM types like this. The new caller wants only the serialized string form. The old caller wants only an RGBA value, which does not require allocation of a CSSValue object at all. The new caller wants the serialized color, and it also seems wasteful to allocate a CSSValue for this.

> Source/WebCore/editing/EditingStyle.cpp:1350
> +        if (RefPtr<CSSValue> value = ComputedStyleExtractor(ancestor).propertyValue(CSSPropertyBackgroundColor)) {
> +            if (!isTransparentColorValue(value.get()))

This is an inefficient approach. I don’t think we want to involve computed style until we reach the item that has a background color. We should answer the “does this have a non-transparent background color” using the lower level, RenderStyle. It’s wasteful to construct a CSSValue for internal use.

> Source/WebCore/editing/EditingStyle.cpp:1354
> +    return 0;

nullptr

[Darin Adler]

Comment on attachment 216786 [details]
Solution enhancement for the font size change and backgroundcolor issue

Oops, sorry, meant review-

[Ryosuke Niwa]

Comment on attachment 216786 [details]
Solution enhancement for the font size change and backgroundcolor issue

View in context: https://bugs.webkit.org/attachment.cgi?id=216786&action=review

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-expected.txt:7
> +| <span>
> +|   style="background-color: rgb(255, 0, 0);"
> +|   <font>
> +|     size="7"
> +|     style="background-color:rgb(255, 0, 0)"
> +|     "777"

We shouldn't have all these nested spans with red background.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1464
> +            RefPtr<CSSValue> backgroundColorCSSValue = m_style->EditingStyle::backgroundColorInEffect(startNode.get());

Why do we need EditingStyle::?

>> Source/WebCore/editing/ApplyStyleCommand.cpp:1468
>> +                newCSSStyle.append("background-color:" + backgroundColorCSSValue->cssText());
> 
> This is wrong. If we already have a string builder we should append one string at a time. This way does unnecessary allocation. Something like this:
> 
>     newCSSStyle.appendLiteral(“background-color:”);
>     newCSSStyle.append(backgroundColorCSSValue->cssText());
> 
> Not sure I have the syntax exactly right.

We shouldn't be doing these kinds of string manipulations to modify style.
That should be done in EditingStyle instead.

[Artur Moryc]

Created attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Hello,

thank you for your previous valuable remarks and comprehensive explanations.

[EFL EWS Bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/40248113

[EFL EWS Bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/40158131

[Build Bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/40078155

[kov's GTK+ EWS bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/40148148

[Build Bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/40098146

[Build Bot]

Comment on attachment 218111 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218111 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/40058141

[Artur Moryc]

Created attachment 218155 [details]
New solution for the font size change and backgroundcolor issue

Hi,

thanks again for previous valuable remarks and comprehensive explanations.

[WebKit Commit Bot]

Attachment 218155 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-expected.txt', u'LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-continue-style-expected.txt', u'LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-continue-style.html', u'LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment.html', u'LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style-expected.txt', u'LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/ApplyStyleCommand.cpp', u'Source/WebCore/editing/EditingStyle.cpp', u'Source/WebCore/editing/EditingStyle.h']" exit_code: 1
Source/WebCore/editing/EditingStyle.cpp:452:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 218155 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218155 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/39978363

[Build Bot]

Comment on attachment 218155 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218155 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/40428096

[Artur Moryc]

Created attachment 218161 [details]
New solution for the font size change and backgroundcolor issue

Thanks for previous valuable remarks and comprehensive explanations!

[Build Bot]

Comment on attachment 218161 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218161 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/41658030

[Build Bot]

Comment on attachment 218161 [details]
New solution for the font size change and backgroundcolor issue

Attachment 218161 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/41508049

[Artur Moryc]

Created attachment 218389 [details]
New solution for the font size change and backgroundcolor issue

Hi,
Thanks for previous valuable review remarks and very comprehensive explanations!

[Artur Moryc]

Hello,
any comments from reviewers?

[Ryosuke Niwa]

Comment on attachment 218389 [details]
New solution for the font size change and backgroundcolor issue

View in context: https://bugs.webkit.org/attachment.cgi?id=218389&action=review

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-expected.txt:3
> +| <span>
> +|   style="background-color: rgb(255, 0, 0);"

We shouldn't have this outer span with background color.

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-continue-style.html:12
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

Why does this script element have both language and type specified? Neither attribute is needed here.
Also, we need to wrap the path in quotation marks.

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-continue-style.html:18
> +    function changeFontSizeForOneBackgroundcolorTest() {

Nit: the function shouldn't be indented.

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-continue-style.html:23
> +        for(fontSize = maxFontSize-1; fontSize >= minFontSize; fontSize--) {

Nit: Space around -.

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment.html:5
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

Ditto.

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment.html:11
> +    function changeFontSizeForOneBackgroundcolorTest() {

Ditto.

> LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html:17
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

Ditto.

> LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html:24
> +        var minFontSize = 1;

Nit: Wrong indentation. Each indentation must be done by 4 spaces.

> Source/WebCore/ChangeLog:26
> +        * editing/ApplyStyleCommand.cpp:
> +        (WebCore::ApplyStyleCommand::applyInlineStyleChange):
> +        * editing/EditingStyle.cpp:
> +        (WebCore::ColorValueToRGBA):
> +        (WebCore::rgbaBackgroundColorInEffect):
> +        (WebCore::EditingStyle::init):
> +        (WebCore::EditingStyle::styleAtSelectionStart):
> +        (WebCore::EditingStyle::backgroundColorInEffect):
> +        * editing/EditingStyle.h:

You need to have per-function level comments.

> Source/WebCore/editing/EditingStyle.cpp:390
> +static RGBA32 colorValueToRGBA(Color colorValue)
> +{
> +    if (!colorValue.isValid() || !colorValue.alpha())
> +        return Color::transparent;

Why is invalid color treated as transparent?
It seems like this is specific to background color and better be delt with in rgbaBackgroundColorInEffect itself.

> Source/WebCore/editing/EditingStyle.cpp:1494
> +        color = style->visitedDependentColor(CSSPropertyBackgroundColor);

We should definitely not use visitedDependentColor.
r- because this leaks history information.

> Source/WebCore/editing/EditingStyle.cpp:-1608
> -PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
> -{

Why are you rewriting this function? This was a perfectly reasonable implementation.

[Artur Moryc]

Created attachment 223231 [details]
Updating solution for the font size and backgroundcolor changes 

Response to previous comments:
1.
> Source/WebCore/editing/EditingStyle.cpp:-1608
> -PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
> -{
>Why are you rewriting this function? This was a perfectly reasonable implementation.

Please have a look at  Darin Adler's suggestions.  

2. 
>We shouldn't have this outer span with background color.
Yes. I agree, however you were not content if redundant spans were removed. 

Current solution leads to the following results:
1. in the fontsize-varying-backgroundcolor-adjustment.html test there is proper span and font tags distribution without outer span.
2. in the fontsize-varying-backgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node (why not a sibling node?) but the result is proper.
<font>
|   <span>
|     style="background-color: rgb(0, 255, 0);"
|     "333"
|   <font>
|     size="2"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "222"

3. in the fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node and extra span tag with background is added  which 
leads to shadowing the background size by the default fontsize. If there is no going over the default fontsize then backgroundcolor is being added by css style to all nodes and it works.     
 <font>
|   class="backGround"
|   "333"
|   <font>
|     size="2"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "222"
|   <font>
|     size="1"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "111<#selection-caret>"

Can you give any hints how to deal with that?

[WebKit Commit Bot]

Attachment 223231 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/EditingStyle.cpp:411:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Artur Moryc]

Created attachment 223238 [details]
Updating solution for the font size and backgroundcolor changes

Response to previous comments:
1.
> Source/WebCore/editing/EditingStyle.cpp:-1608
> -PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
> -{
>Why are you rewriting this function? This was a perfectly reasonable implementation.

Please have a look at  Darin Adler's suggestions.  

2. 
>We shouldn't have this outer span with background color.
Yes. I agree, however you were not content if redundant spans were removed. 

Current solution leads to the following results:
1. in the fontsize-varying-backgroundcolor-adjustment.html test there is proper span and font tags distribution without outer span.
2. in the fontsize-varying-backgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node (why not a sibling node?) but the result is proper.
<font>
|   <span>
|     style="background-color: rgb(0, 255, 0);"
|     "333"
|   <font>
|     size="2"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "222"

3. in the fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node and extra span tag with background is added  which 
leads to shadowing the background size by the default fontsize. If there is no going over the default fontsize then backgroundcolor is being added by css style to all nodes and it works.     
 <font>
|   class="backGround"
|   "333"
|   <font>
|     size="2"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "222"
|   <font>
|     size="1"
|     <span>
|       style="background-color: rgb(0, 255, 0);"
|       "111<#selection-caret>"

Can you give any hints how to deal with that?

[Artur Moryc]

Created attachment 223239 [details]
Updating solution for the font size and backgroundcolor changes

[Artur Moryc]

Created attachment 223336 [details]
Updating solution for backgroundcolor and fontsize issue

[Ryosuke Niwa]

Comment on attachment 223336 [details]
Updating solution for backgroundcolor and fontsize issue 

View in context: https://bugs.webkit.org/attachment.cgi?id=223336&action=review

Please review our contribution guide: http://www.webkit.org/coding/contributing.html
and address my review comments.

Keep posting patches wouldn't change my mind as long as they have the same problems I've pointed out thus far.

> LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style-expected.txt:23
> +|   <font>
> +|     size="2"
> +|     <span>
> +|       style="background-color: rgb(0, 255, 0);"
> +|       "222"

We shouldn't be creating a nested font/span structure like that.
"background-color" property should have been added on the font element.

> Source/WebCore/editing/EditingStyle.cpp:1506
> +        color = style->visitedDependentColor(CSSPropertyBackgroundColor);

Calling visitedDependentColor here is WRONG as I have repeatedly pointed out.
visited dependent color leaks history information so we can't do this. r-.

[Ryosuke Niwa]

(In reply to comment #34)
> 2. 
> >We shouldn't have this outer span with background color.
> Yes. I agree, however you were not content if redundant spans were removed. 

Yes. We should strive to generate the simplest markup possible.

> Current solution leads to the following results:
> 1. in the fontsize-varying-backgroundcolor-adjustment.html test there is proper span and font tags distribution without outer span.
> 2. in the fontsize-varying-backgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node (why not a sibling node?) but the result is proper.
> <font>
> |   <span>
> |     style="background-color: rgb(0, 255, 0);"
> |     "333"
> |   <font>
> |     size="2"
> |     <span>
> |       style="background-color: rgb(0, 255, 0);"
> |       "222"
> 
> 3. in the fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html test when it comes to going from the default fontsize the next font tag is being created as a child node and extra span tag with background is added  which 
> leads to shadowing the background size by the default fontsize. If there is no going over the default fontsize then backgroundcolor is being added by css style to all nodes and it works.     
>  <font>
> |   class="backGround"
> |   "333"
> |   <font>
> |     size="2"
> |     <span>
> |       style="background-color: rgb(0, 255, 0);"
> |       "222"
> |   <font>
> |     size="1"
> |     <span>
> |       style="background-color: rgb(0, 255, 0);"
> |       "111<#selection-caret>"

In both cases background color should be added to the font element themselves.  In general, we should avoid generating superfluous spans as much as possible.

[Artur Moryc]

Created attachment 224343 [details]
Node distribution if there is no default fontsize. 

Can you let me know if such node distribution is ok from your viewpoint?

[Artur Moryc]

Created attachment 224345 [details]
Node distribution with default fontsize.

For default fontsize node distribution changes:
- extra font tag is added 
- for default fontsize nodes are the span nodes. 

Can you let me know if such distribution is acceptable from your view point?

[Ryosuke Niwa]

Comment on attachment 223239 [details]
Updating solution for the font size and backgroundcolor changes

View in context: https://bugs.webkit.org/attachment.cgi?id=223239&action=review

> LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-expected.txt:6
> +| <font>
> +|   size="7"
> +|   <span>
> +|     style="background-color: rgb(255, 0, 0);"
> +|     "777"

We shouldn't generate nested font/span pairs like these.

[Artur Moryc]

Created attachment 228280 [details]
Node distribution for the default and small fontsize

[Artur Moryc]

Created attachment 234350 [details]
Solution update for the backgroundcolor while the fontsize varies

[Artur Moryc]

Created attachment 234406 [details]
Solution update for the backgroundcolor while the fontsize varies

[WebKit Commit Bot]

Attachment 234406 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/EditingStyle.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Artur Moryc]

Created attachment 234410 [details]
Solution update for the backgroundcolor while the fontsize varies

[Darin Adler]

Comment on attachment 234410 [details]
Solution update for the backgroundcolor while the fontsize varies

View in context: https://bugs.webkit.org/attachment.cgi?id=234410&action=review

Quite a few mistakes here, and not sure if the basic approach is right. We need test cases demonstrating why it’s OK for this to work only on span and font elements. Why not work on <div> and <p> elements too? What about <b> elements?

> Source/WebCore/editing/ApplyStyleCommand.cpp:556
> +    if (!style)
> +        return;

Seems strange to handle null here. I don’t believe the functions below handle null. I suggest that this function take EditingStyle& instead of EditingStyle&.

> Source/WebCore/editing/ApplyStyleCommand.cpp:564
> +    if (node->isTextNode() && node->parentNode())
> +        node = node->parentNode();
> +    if (node->hasTagName(spanTag) || node->hasTagName(fontTag)) {

The behavior here is bizarre. This function works only on span elements, font elements, and text elements that are just inside one of those two. How could that possibly be correct? The text node part seems particularly peculiar. If the function doesn’t work on all nodes, why would it specifically do the parent thing for text nodes?

> Source/WebCore/editing/ApplyStyleCommand.cpp:565
> +        String backgroundColorString = mutableStyle ? mutableStyle->getPropertyValue(CSSPropertyBackgroundColor) : emptyString();

No need to re-check mutableStyle for null here since you are already doing it above.

> Source/WebCore/editing/ApplyStyleCommand.cpp:568
> +            if (backgroundColor.isValid() && backgroundColor.alpha()) {

Don’t need to also check isValid if we are checking alpha for zero since all invalid colors have RGBA of 0.

> Source/WebCore/editing/ApplyStyleCommand.cpp:572
> +                if (backgroundColorString.length() && htmlElement)

It doesn’t make sense to check htmlElement for null here. It will only be null if node was null, and we have already dereferenced null.

Should be using isEmpty here rather than checking length.

> Source/WebCore/editing/ApplyStyleCommand.cpp:576
> +                }
> +            }
> +        }

These braces are indented incorrectly.

> Source/WebCore/editing/EditingStyle.cpp:392
> +    if (!colorValue.isValid() || !colorValue.alpha())
> +        return Color::transparent;

This code is repeated twice, here and in colorValueToRGBA. We don’t need it twice.

If we’re checking for alpha of zero, we don’t also have to check isValid; invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:439
> +        if (value.isValid() && value.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid; invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:1291
> +        if (value.isValid() && value.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid; invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:1647
> +        if (!ancestor || !ancestor->renderStyle())
> +            continue;

There’s no need to check !ancestor here. That’s already in the loop condition.

It doesn’t make much sense to check renderStyle for null, and then using computedStyle. The main point of computedStyle is that it can give us a color even when the render style is not available. If we require a renderStyle then we should just use it.

What guarantees that style is updated? One of this things that ComputedStyleExtractor::propertyValue does is call the appropriate layout functions, such as updateLayoutIgnorePendingStylesheets and such. If you are removing it, then the new code has to handle that issue somehow. Style isn’t guaranteed to already be computed when this function is called.

> Source/WebCore/editing/EditingStyle.cpp:1648
> +        RenderStyle* style = ancestor->computedStyle();

I don’t think we should be using computedStyle at all.

> Source/WebCore/editing/EditingStyle.cpp:1652
> +        if (color.isValid() && color.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid; invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.h:37
> +#include "Color.h"

A return value of Color does not require including the entire Color header. A forward class declaration will suffice.

[Artur Moryc]

Created attachment 238550 [details]
Solution update for the backgroundcolor while the fontsize varies

Tank you for previous comments.
Please, have a look also at the charts backgroundColorFontsizeItalic_setInlineStyleProperty.png  and backgrounColorFonssizeItalic_No_setInlineStyle.png which show the node distribution for the case pointed out in the last review i.e. whether the proposed solution is still valid if backgroundcolor and ather style like italic are set with the font size varying. Also the charts show that the styles once set are being continued if a new paragraph  is added. 
Regards,

Artur M.

[Artur Moryc]

Created attachment 238551 [details]
Node distribution for the latest patch 

This chart shows the node distribution for the latest patch

[Artur Moryc]

Created attachment 238553 [details]
Node distribution

This chart show the node distribution for the latest patch without setting setInlineStyleProperty(CSSPropertyBackgroundColor, backgroundColorString);
in the setPropertyAndInlineStylePropertyWithBackgroundColorIfNeeded method.
But then for one case in the fontsize-varying-cssBackgroundcolor-adjustment-if-continue-style.html test- as described in the change log, the bug still exists.

[Artur Moryc]

Dear reviewer can you comment?

[Michael Catanzaro]

Comment on attachment 238550 [details]
Solution update for the backgroundcolor while the fontsize varies

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[Ahmad Saleem]

Created attachment 459934 [details]
Safari 15.5 matches other browsers

Based on screenshots and reproduction of test case, I think all browser matches each other and this might have been fixed. If the attached in screenshot is not intended behavior, appreciate if someone can provide an update. Else I think this can be marked as "RESOLVED CONFIGURATION CHANGED" or "RESOLVED INVALID". Thanks!

[Alexey Proskuryakov]

I believe that the intention here is to improve on status quo. It's not obvious to me personally if the proposed behavior is better, but that's what people have been driving towards here.