Bug 77204 – Kill per-Attribute style declarations.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andreas Kling]

The way style-based-on-attributes works today is by creating a style declaration for each Attribute that affects element style and then adding these one by one in CSSStyleSelector::matchAllRules().
The design is a remnant of the Old Web(tm) where we people couldn't rely on CSS for styling to the extent it's possible today.

I'm proposing that we move away from this approach and instead let attributes affect a single style declaration that hangs on the element, much like a secondary inline "style" attribute. The change is transparent to the web, but frees us from having Attributes that point to style declarations.

This will mean that style declarations generated from attributes are no longer sharable in StyledElement.cpp's CSSMappedAttributeDeclaration table, but I believe it will be a net win in the end.

[Andreas Kling]

Created attachment 124293 [details]
Patch for EWS and brave people

Uploading my current WIP, which passes all layout tests, but lacks ChangeLog and a solution for supporting the old behavior of Attr::style() which is exposed as Obj-C API.
Immediate follow-up work after this patch would be removing the "preserveDecls" argument from attributeChanged() and killing CSSMappedAttributeDeclaration, moving the remaining logic into StyledElement's helpers.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:51:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:58:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:65:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:81:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/html/HTMLHRElement.cpp:104:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 45 files


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

[WebKit Review Bot]

Comment on attachment 124293 [details]
Patch for EWS and brave people

Attachment 124293 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11265710

New failing tests:
editing/style/make-text-writing-direction-inline.html
inspector/elements/elements-panel-styles.html
inspector/styles/styles-new-API.html
inspector/styles/styles-computed-trace.html

[Andreas Kling]

CC'ing some Inspector folks since this will break their feature for displaying applied style per attribute.

[Andreas Kling]

Created attachment 125444 [details]
Proposed patch

[Andreas Kling]

Created attachment 125445 [details]
Proposed patch

Hm, seems it didn't upload the whole patch..

[Benjamin Poulain]

Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:122
> -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)

Objection!

[Benjamin Poulain]

Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:122
> -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)

Objection!

[Andreas Kling]

(In reply to comment #8)
> (From update of attachment 125445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125445&action=review
> 
> > Source/WebCore/dom/StyledElement.cpp:122
> > -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> > +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)
> 
> Objection!

Yeah, fine. That's horrible. I'll expand into multiple removeCSSProperty() calls again.

[Ryosuke Niwa]

Comment on attachment 125445 [details]
Proposed patch

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

>>> Source/WebCore/dom/StyledElement.cpp:122
>>> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)
>> 
>> Objection!
> 
> Objection!

I think it's okay to enumerate ids like this but you may also consider using an array here.

[Antti Koivisto]

Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:1006
> +                result.isCacheable = false;

Caching is useful even when properties are not shared between elements as it is common for the same element style to get recomputed.

We should be able to share identical attribute styles between elements too.

> Source/WebCore/dom/Attr.h:62
> +    // A deprecated extension to get presentational information for attributes.
> +    // We have to keep it around because it's exposed in the Obj-C DOM API.
> +    CSSStyleDeclaration* style() { return 0; }

Hope we don't have clients for this!

> Source/WebCore/dom/CSSMappedAttributeDeclaration.h:-76
>  
>      RefPtr<StylePropertySet> m_declaration;
> -    MappedAttributeEntry m_entryType;
> -    QualifiedName m_attrName;
> -    AtomicString m_attrValue;

This class has kinda lost its reason to exist.

[Antti Koivisto]

Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:144
> +    if (id2 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id2);
> +    if (id3 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id3);
> +    if (id4 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id4);
> +    if (id5 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id5);
> +    if (id6 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id6);
> +    if (id7 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id7);
> +    if (id8 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id8);

I think you can bail out when you see the first CSSPropertyInvalid.

[Andreas Kling]

Committed r106740: <http://trac.webkit.org/changeset/106740>

[Andreas Kling]

Committed r106741: <http://trac.webkit.org/changeset/106741>

[Darin Adler]

(In reply to comment #11)
> > Source/WebCore/dom/Attr.h:62
> > +    // A deprecated extension to get presentational information for attributes.
> > +    // We have to keep it around because it's exposed in the Obj-C DOM API.
> > +    CSSStyleDeclaration* style() { return 0; }
> 
> Hope we don't have clients for this!

If nobody is using this from Objective-C we can remove it from the Objective-C DOM API.

If we want a method that just returns nil we can set that up in Mac WebKit with a category or something like that and remove it from WebCore.

On reflection, it’s a seriously bad thing that we export Attr as part of the ObjC DOM.