Bug 143922 – Setting inline style to the same value it already has triggers a style recalc

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

PropertySetCSSStyleDeclaration::setPropertyInternal() does:

    bool changed = m_propertySet->setProperty(propertyID, value, important, contextStyleSheet());

but as far as I can see, setProperty() returns true simply when parsing succeeded, not necessarily when the property changed.

This causes lots of extra style recalcs when scrolling Facebook.

[Radar WebKit Bug Importer]

<rdar://problem/20603831>

[Simon Fraser (smfr)]

Created attachment 251102 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 251104 [details]
Patch

[Antti Koivisto]

Comment on attachment 251104 [details]
Patch

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

Ok though I would prefer a factoring that does not introduce a new bool& return value.

> Source/WebCore/css/CSSParser.cpp:1323
> +bool CSSParser::parseValue(MutableStyleProperties* declaration, CSSPropertyID propertyID, const String& string, bool important, StyleSheetContents* contextStyleSheet, bool& changed)

It would be nicer if these function returned an enum instead of two bools:

enum class ParseResult { Changed, Unchanged, Error }

or similar.

> Source/WebCore/css/CSSProperty.h:95
> +        if (!m_value && !other.m_value)
> +            return true;

You should do

if (m_value == other.m_value)
   return true;

instead. Values may be shared.

> Source/WebCore/css/StyleProperties.cpp:683
>      // When replacing an existing property value, this moves the property to the end of the list.
>      // Firefox preserves the position, and MSIE moves the property to the beginning.

Is this comment true anymore? The setProperty code looks like it replaces properties in-place.

[Simon Fraser (smfr)]

https://trac.webkit.org/r183017

[Simon Fraser (smfr)]

> > Source/WebCore/css/StyleProperties.cpp:683
> >      // When replacing an existing property value, this moves the property to the end of the list.
> >      // Firefox preserves the position, and MSIE moves the property to the beginning.
> 
> Is this comment true anymore? The setProperty code looks like it replaces
> properties in-place.

Didn't change this because I didn't have time to investigate.