[webkit-reviews] review denied: [Bug 121415] The size of the background colour is not properly adjusted when the font size is being changed : [Attachment 214466] Propesed solution 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 214466: Propesed solution for the font size change and
backgroundcolor issue	
https://bugs.webkit.org/attachment.cgi?id=214466&action=review

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