[webkit-reviews] review denied: [Bug 121415] The size of the background colour is not properly adjusted when the font size is being changed : [Attachment 216387] Solution improvment for the font size change and backgroundcolor issue

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Artur Moryc <a.moryc at samsung.com>'s
request for review:
Bug 121415: The size of the background colour is not properly adjusted when the
font size is being changed
https://bugs.webkit.org/show_bug.cgi?id=121415

Attachment 216387: Solution improvment for the font size change and
backgroundcolor issue
https://bugs.webkit.org/attachment.cgi?id=216387&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.t
xt: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.t
xt: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.