REGRESSION(71556,68059): queryCommandValue screws up background color at collapsed cursor

• Component: HTML Editing
• Status: RESOLVED FIXED

[Marcos Almeida]

In Chrome 8, go to the following plexode link:

http://plexode.com/eval3/#ht=%3Cdiv%20contentEditable%3Ehello%3C%2Fdiv%3E&ohh=0&ohj=0&jt=document.execCommand('backColor'%2C%20false%2C%20'%23ff0000')%3B%0Adocument.queryCommandValue('backColor')%3B&ojh=0&ojj=0&ms=100&oth=0&otj=0&cex=0

Place the cursor at the end of "hello", then click "eval JS now". The newly selected background color is output at the bottom (rgb(255, 0, 0)). Then start typing. Your text will correctly have a red background.

Now try the same in Chrome 9.0.597.83. The color output at the bottom will be rgba(0, 0, 0, 0) instead, and when you start typing, the text will not have a red background.

What seems to trigger this bug is the call to queryCommandValue. If you comment that out and try it again, the text will correctly have a red background.

[Ryosuke Niwa]

I can't believe we didn't have a test case for this!

[Ryosuke Niwa]

Ah... there's another bug with queryCommandValue("fontSize").

[Ryosuke Niwa]

Created attachment 81613 [details]
fixes the bug

[Darin Adler]

Comment on attachment 81613 [details]
fixes the bug

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:208
> +int legacyFontSizeFromCSSValue(Document* document, CSSPrimitiveValue* cssValue, bool shouldUseFixedFontDefaultSize, LegacyFontSizeMode mode)

I would just call that argument “value”

> Source/WebCore/editing/ApplyStyleCommand.cpp:210
> +    if (cssValue->primitiveType() >= CSSPrimitiveValue::CSS_PX && cssValue->primitiveType() <= CSSPrimitiveValue::CSS_PC) {

Named function for this check taking either CSSPrimitiveValue or the primitive type would be a good way to document what it is and why it makes sense to do it.

> Source/WebCore/editing/ApplyStyleCommand.cpp:219
> +            return legacyFontSize;
> +    } else if (CSSValueXSmall <= cssValue->getIdent() && cssValue->getIdent() <= CSSValueWebkitXxxLarge)
> +        return cssValue->getIdent() - CSSValueXSmall + 1;
> +    return 0;

else here is pretty awkward. I think a return 0 inside that first if would be clearer.

> Source/WebCore/editing/ApplyStyleCommand.cpp:279
> +            m_applyFontSize = String::number(legacyFontSize);

m_applyFontSize is really such a terrible name. It’s a verb phrase!

> Source/WebCore/editing/Editor.cpp:1052
> +        if (cssValue->isPrimitiveValue())
> +            value = String::number(legacyFontSizeFromCSSValue(m_frame->document(), static_cast<CSSPrimitiveValue*>(cssValue.get()),
> +                shouldUseFixedFontDefaultSize, AlwaysUseLegacyFontSize));

Need braces here.

> Source/WebCore/editing/Editor.cpp:3142
> -    RefPtr<EditingStyle> typingStyle = m_frame->selection()->typingStyle();
> -    typingStyle->removeNonEditingProperties();
> +    RefPtr<EditingStyle> typingStyle = m_frame->selection()->typingStyle()->copy();

Change log does not make it clear why it’s better to omit the call to removeNonEditingProperties. It states that it was wrong, but not why it was bad.

Need a “why” in the change log.

[Ryosuke Niwa]

Comment on attachment 81613 [details]
fixes the bug

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

>> Source/WebCore/editing/ApplyStyleCommand.cpp:210
>> +    if (cssValue->primitiveType() >= CSSPrimitiveValue::CSS_PX && cssValue->primitiveType() <= CSSPrimitiveValue::CSS_PC) {
> 
> Named function for this check taking either CSSPrimitiveValue or the primitive type would be a good way to document what it is and why it makes sense to do it.

isCSSValueLength?

>> Source/WebCore/editing/ApplyStyleCommand.cpp:219
>> +    return 0;
> 
> else here is pretty awkward. I think a return 0 inside that first if would be clearer.

Good point.  Will do.

>> Source/WebCore/editing/ApplyStyleCommand.cpp:279
>> +            m_applyFontSize = String::number(legacyFontSize);
> 
> m_applyFontSize is really such a terrible name. It’s a verb phrase!

FWIW, it's a pattern used in StyleChange class. I'm planning to do an overhaul of StyleChange class once EditingStyle is deployed in ApplyStyleCommand.

>> Source/WebCore/editing/Editor.cpp:1052
>> +                shouldUseFixedFontDefaultSize, AlwaysUseLegacyFontSize));
> 
> Need braces here.

Oops, will do.

>> Source/WebCore/editing/Editor.cpp:3142
>> +    RefPtr<EditingStyle> typingStyle = m_frame->selection()->typingStyle()->copy();
> 
> Change log does not make it clear why it’s better to omit the call to removeNonEditingProperties. It states that it was wrong, but not why it was bad.
> 
> Need a “why” in the change log.

removeNonEditingProperties was removing background-color because it's not an inheritable properties.  I guess it wasn't clear from the change log.  I'll revise the change log before landing.

[Ryosuke Niwa]

Comment on attachment 81613 [details]
fixes the bug

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

> Source/WebCore/ChangeLog:25
> +        (WebCore::Editor::selectionComputedStyle): Makes a copy before modifying typing style.
> +        No longer calls legacyFontSizeFromCSSValue on the copied typing style.

Oops, now I understand your comment about removeNonEditingProperties.  This should read "no longer calls removeNonEditingProperties".

[Ryosuke Niwa]

Committed r78011: <http://trac.webkit.org/changeset/78011>