Changeset 176864 in webkit


Ignore:
Timestamp:
Dec 5, 2014 12:58:08 PM (9 years ago)
Author:
benjamin@webkit.org
Message:

Fix style sharing with the "type" and "readonly" attributes
​https://bugs.webkit.org/show_bug.cgi?id=139283

Patch by Benjamin Poulain <​bpoulain@apple.com> on 2014-12-05
Reviewed by Antti Koivisto.

Source/WebCore:

There are two bugs adressed with this patch:
1) The attributes "type" and "readonly" where only handled correctly

for input elements. For everything else, they could incorrectly
be ignored for style sharing.

2) The handling of attributes was incorrect for selector lists, leading

to various bugs (incorrect style sharing in some cases, disabling
style sharing on valid cases).

For [1], the problem was that attribute checking had been limited to
StyleResolver::canShareStyleWithControl(). That function is for handling
the special states of input element. For any other element, the attributes
were simply ignored.

For [2], there were a bunch of small problems. First, containsUncommonAttributeSelector()
was not recursive, which caused it to ignored any nested selector list. This used to be
correct but since we have advanced selectors we can no longer assumed selectors are not nested.

A second issue was that any attribute in a selector list was causing us to fall back
to the slow case. Now that we have the fast :matches(), we really don't want that.

The function containsUncommonAttributeSelector() was transformed into a recursive function
tracking where we are in the selector.

At the entry point, we start with the flag "startsOnRightmostElement" set to true. The flag is then
updated on the stack of each recursive call.

For example, "webkit > is:matches(freaking > awesome)". We evalute "is" with the flag to true, then recurse
into evaluating "freaking > awesome" with the flag still set to true. When we evalute ">", the flag
is set to false to evaluate any following selectors.
After evaluating "freaking > awesome", we go back to our previous stack frame, and the flag
is back to true and we can continue evaluating with the curren top level state.

From some logging, I discovered that the attribute handling is way too aggressive.
This is not a regression and I cannot fix that easily so I left a fixme.

Tests: fast/css/data-attribute-style-sharing-1.html

fast/css/data-attribute-style-sharing-2.html
fast/css/data-attribute-style-sharing-3.html
fast/css/data-attribute-style-sharing-4.html
fast/css/data-attribute-style-sharing-5.html
fast/css/data-attribute-style-sharing-6.html
fast/css/data-attribute-style-sharing-7.html
fast/css/readonly-attribute-style-sharing-1.html
fast/css/readonly-attribute-style-sharing-2.html
fast/css/readonly-attribute-style-sharing-3.html
fast/css/readonly-attribute-style-sharing-4.html
fast/css/readonly-attribute-style-sharing-5.html
fast/css/readonly-attribute-style-sharing-6.html
fast/css/readonly-attribute-style-sharing-7.html
fast/css/type-attribute-style-sharing-1.html
fast/css/type-attribute-style-sharing-2.html
fast/css/type-attribute-style-sharing-3.html
fast/css/type-attribute-style-sharing-4.html
fast/css/type-attribute-style-sharing-5.html
fast/css/type-attribute-style-sharing-6.html
fast/css/type-attribute-style-sharing-7.html

  • css/RuleSet.cpp:

(WebCore::containsUncommonAttributeSelector):
(WebCore::RuleData::RuleData):
(WebCore::selectorListContainsAttributeSelector): Deleted.

  • css/StyleResolver.cpp:

(WebCore::StyleResolver::canShareStyleWithControl):
(WebCore::StyleResolver::canShareStyleWithElement):

LayoutTests:

  • fast/css/data-attribute-style-sharing-1-expected.html: Added.
  • fast/css/data-attribute-style-sharing-1.html: Added.
  • fast/css/data-attribute-style-sharing-2-expected.html: Added.
  • fast/css/data-attribute-style-sharing-2.html: Added.
  • fast/css/data-attribute-style-sharing-3-expected.html: Added.
  • fast/css/data-attribute-style-sharing-3.html: Added.
  • fast/css/data-attribute-style-sharing-4-expected.html: Added.
  • fast/css/data-attribute-style-sharing-4.html: Added.
  • fast/css/data-attribute-style-sharing-5-expected.html: Added.
  • fast/css/data-attribute-style-sharing-5.html: Added.
  • fast/css/data-attribute-style-sharing-6-expected.html: Added.
  • fast/css/data-attribute-style-sharing-6.html: Added.
  • fast/css/data-attribute-style-sharing-7-expected.html: Added.
  • fast/css/data-attribute-style-sharing-7.html: Added.
  • fast/css/readonly-attribute-style-sharing-1-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-1.html: Added.
  • fast/css/readonly-attribute-style-sharing-2-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-2.html: Added.
  • fast/css/readonly-attribute-style-sharing-3-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-3.html: Added.
  • fast/css/readonly-attribute-style-sharing-4-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-4.html: Added.
  • fast/css/readonly-attribute-style-sharing-5-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-5.html: Added.
  • fast/css/readonly-attribute-style-sharing-6-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-6.html: Added.
  • fast/css/readonly-attribute-style-sharing-7-expected.html: Added.
  • fast/css/readonly-attribute-style-sharing-7.html: Added.
  • fast/css/type-attribute-style-sharing-1-expected.html: Added.
  • fast/css/type-attribute-style-sharing-1.html: Added.
  • fast/css/type-attribute-style-sharing-2-expected.html: Added.
  • fast/css/type-attribute-style-sharing-2.html: Added.
  • fast/css/type-attribute-style-sharing-3-expected.html: Added.
  • fast/css/type-attribute-style-sharing-3.html: Added.
  • fast/css/type-attribute-style-sharing-4-expected.html: Added.
  • fast/css/type-attribute-style-sharing-4.html: Added.
  • fast/css/type-attribute-style-sharing-5-expected.html: Added.
  • fast/css/type-attribute-style-sharing-5.html: Added.
  • fast/css/type-attribute-style-sharing-6-expected.html: Added.
  • fast/css/type-attribute-style-sharing-6.html: Added.
  • fast/css/type-attribute-style-sharing-7-expected.html: Added.
  • fast/css/type-attribute-style-sharing-7.html: Added.
Location:
trunk
Files:
42 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r176821 r176864  
     12014-12-05  Benjamin Poulain  <bpoulain@apple.com>
     2
     3        Fix style sharing with the "type" and "readonly" attributes
     4        https://bugs.webkit.org/show_bug.cgi?id=139283
     5
     6        Reviewed by Antti Koivisto.
     7
     8        * fast/css/data-attribute-style-sharing-1-expected.html: Added.
     9        * fast/css/data-attribute-style-sharing-1.html: Added.
     10        * fast/css/data-attribute-style-sharing-2-expected.html: Added.
     11        * fast/css/data-attribute-style-sharing-2.html: Added.
     12        * fast/css/data-attribute-style-sharing-3-expected.html: Added.
     13        * fast/css/data-attribute-style-sharing-3.html: Added.
     14        * fast/css/data-attribute-style-sharing-4-expected.html: Added.
     15        * fast/css/data-attribute-style-sharing-4.html: Added.
     16        * fast/css/data-attribute-style-sharing-5-expected.html: Added.
     17        * fast/css/data-attribute-style-sharing-5.html: Added.
     18        * fast/css/data-attribute-style-sharing-6-expected.html: Added.
     19        * fast/css/data-attribute-style-sharing-6.html: Added.
     20        * fast/css/data-attribute-style-sharing-7-expected.html: Added.
     21        * fast/css/data-attribute-style-sharing-7.html: Added.
     22        * fast/css/readonly-attribute-style-sharing-1-expected.html: Added.
     23        * fast/css/readonly-attribute-style-sharing-1.html: Added.
     24        * fast/css/readonly-attribute-style-sharing-2-expected.html: Added.
     25        * fast/css/readonly-attribute-style-sharing-2.html: Added.
     26        * fast/css/readonly-attribute-style-sharing-3-expected.html: Added.
     27        * fast/css/readonly-attribute-style-sharing-3.html: Added.
     28        * fast/css/readonly-attribute-style-sharing-4-expected.html: Added.
     29        * fast/css/readonly-attribute-style-sharing-4.html: Added.
     30        * fast/css/readonly-attribute-style-sharing-5-expected.html: Added.
     31        * fast/css/readonly-attribute-style-sharing-5.html: Added.
     32        * fast/css/readonly-attribute-style-sharing-6-expected.html: Added.
     33        * fast/css/readonly-attribute-style-sharing-6.html: Added.
     34        * fast/css/readonly-attribute-style-sharing-7-expected.html: Added.
     35        * fast/css/readonly-attribute-style-sharing-7.html: Added.
     36        * fast/css/type-attribute-style-sharing-1-expected.html: Added.
     37        * fast/css/type-attribute-style-sharing-1.html: Added.
     38        * fast/css/type-attribute-style-sharing-2-expected.html: Added.
     39        * fast/css/type-attribute-style-sharing-2.html: Added.
     40        * fast/css/type-attribute-style-sharing-3-expected.html: Added.
     41        * fast/css/type-attribute-style-sharing-3.html: Added.
     42        * fast/css/type-attribute-style-sharing-4-expected.html: Added.
     43        * fast/css/type-attribute-style-sharing-4.html: Added.
     44        * fast/css/type-attribute-style-sharing-5-expected.html: Added.
     45        * fast/css/type-attribute-style-sharing-5.html: Added.
     46        * fast/css/type-attribute-style-sharing-6-expected.html: Added.
     47        * fast/css/type-attribute-style-sharing-6.html: Added.
     48        * fast/css/type-attribute-style-sharing-7-expected.html: Added.
     49        * fast/css/type-attribute-style-sharing-7.html: Added.
     50
    1512014-12-04  Dean Jackson  <dino@apple.com>
    252

  • trunk/Source/WebCore/ChangeLog

    r176863 r176864  
     12014-12-05  Benjamin Poulain  <bpoulain@apple.com>
     2
     3        Fix style sharing with the "type" and "readonly" attributes
     4        https://bugs.webkit.org/show_bug.cgi?id=139283
     5
     6        Reviewed by Antti Koivisto.
     7
     8        There are two bugs adressed with this patch:
     9        1) The attributes "type" and "readonly" where only handled correctly
     10           for input elements. For everything else, they could incorrectly
     11           be ignored for style sharing.
     12        2) The handling of attributes was incorrect for selector lists, leading
     13           to various bugs (incorrect style sharing in some cases, disabling
     14           style sharing on valid cases).
     15
     16        For [1], the problem was that attribute checking had been limited to
     17        StyleResolver::canShareStyleWithControl(). That function is for handling
     18        the special states of input element. For any other element, the attributes
     19        were simply ignored.
     20
     21        For [2], there were a bunch of small problems. First, containsUncommonAttributeSelector()
     22        was not recursive, which caused it to ignored any nested selector list. This used to be
     23        correct but since we have advanced selectors we can no longer assumed selectors are not nested.
     24
     25        A second issue was that any attribute in a selector list was causing us to fall back
     26        to the slow case. Now that we have the fast :matches(), we really don't want that.
     27
     28        The function containsUncommonAttributeSelector() was transformed into a recursive function
     29        tracking where we are in the selector.
     30
     31        At the entry point, we start with the flag "startsOnRightmostElement" set to true. The flag is then
     32        updated on the stack of each recursive call.
     33
     34        For example, "webkit > is:matches(freaking > awesome)". We evalute "is" with the flag to true, then recurse
     35        into evaluating "freaking > awesome" with the flag still set to true. When we evalute ">", the flag
     36        is set to false to evaluate any following selectors.
     37        After evaluating "freaking > awesome", we go back to our previous stack frame, and the flag
     38        is back to true and we can continue evaluating with the curren top level state.
     39
     40        From some logging, I discovered that the attribute handling is way too aggressive.
     41        This is not a regression and I cannot fix that easily so I left a fixme.
     42
     43        Tests: fast/css/data-attribute-style-sharing-1.html
     44               fast/css/data-attribute-style-sharing-2.html
     45               fast/css/data-attribute-style-sharing-3.html
     46               fast/css/data-attribute-style-sharing-4.html
     47               fast/css/data-attribute-style-sharing-5.html
     48               fast/css/data-attribute-style-sharing-6.html
     49               fast/css/data-attribute-style-sharing-7.html
     50               fast/css/readonly-attribute-style-sharing-1.html
     51               fast/css/readonly-attribute-style-sharing-2.html
     52               fast/css/readonly-attribute-style-sharing-3.html
     53               fast/css/readonly-attribute-style-sharing-4.html
     54               fast/css/readonly-attribute-style-sharing-5.html
     55               fast/css/readonly-attribute-style-sharing-6.html
     56               fast/css/readonly-attribute-style-sharing-7.html
     57               fast/css/type-attribute-style-sharing-1.html
     58               fast/css/type-attribute-style-sharing-2.html
     59               fast/css/type-attribute-style-sharing-3.html
     60               fast/css/type-attribute-style-sharing-4.html
     61               fast/css/type-attribute-style-sharing-5.html
     62               fast/css/type-attribute-style-sharing-6.html
     63               fast/css/type-attribute-style-sharing-7.html
     64
     65        * css/RuleSet.cpp:
     66        (WebCore::containsUncommonAttributeSelector):
     67        (WebCore::RuleData::RuleData):
     68        (WebCore::selectorListContainsAttributeSelector): Deleted.
     69        * css/StyleResolver.cpp:
     70        (WebCore::StyleResolver::canShareStyleWithControl):
     71        (WebCore::StyleResolver::canShareStyleWithElement):
     72
    1732014-12-05  Jer Noble  <jer.noble@apple.com>
    274

  • trunk/Source/WebCore/css/RuleSet.cpp

    r176157 r176864  
    9797}
    9898
    99 static inline bool selectorListContainsAttributeSelector(const CSSSelector* selector)
    100 {
    101     const CSSSelectorList* selectorList = selector->selectorList();
    102     if (!selectorList)
    103         return false;
    104     for (const CSSSelector* selector = selectorList->first(); selector; selector = CSSSelectorList::next(selector)) {
    105         for (const CSSSelector* component = selector; component; component = component->tagHistory()) {
    106             if (component->isAttributeSelector())
    107                 return true;
    108         }
    109     }
    110     return false;
    111 }
    112 
    11399static inline bool isCommonAttributeSelectorAttribute(const QualifiedName& attribute)
    114100{
    … …  
    117103}
    118104
    119 static inline bool containsUncommonAttributeSelector(const CSSSelector* selector)
    120 {
    121     for (; selector; selector = selector->tagHistory()) {
    122         // Allow certain common attributes (used in the default style) in the selectors that match the current element.
    123         if (selector->isAttributeSelector() && !isCommonAttributeSelectorAttribute(selector->attribute()))
    124             return true;
    125         if (selectorListContainsAttributeSelector(selector))
    126             return true;
    127         if (selector->relation() != CSSSelector::SubSelector) {
    128             selector = selector->tagHistory();
    129             break;
    130         }
    131     }
    132 
    133     for (; selector; selector = selector->tagHistory()) {
    134         if (selector->isAttributeSelector())
    135             return true;
    136         if (selectorListContainsAttributeSelector(selector))
    137             return true;
    138     }
     105static bool containsUncommonAttributeSelector(const CSSSelector& rootSelector, bool matchesRightmostElement)
     106{
     107    const CSSSelector* selector = &rootSelector;
     108    do {
     109        if (selector->isAttributeSelector()) {
     110            // FIXME: considering non-rightmost simple selectors is necessary because of the style sharing of cousins.
     111            // It is a primitive solution which disable a lot of style sharing on pages that rely on attributes for styling.
     112            // We should investigate better ways of doing this.
     113            if (!isCommonAttributeSelectorAttribute(selector->attribute()) || !matchesRightmostElement)
     114                return true;
     115        }
     116
     117        if (const CSSSelectorList* selectorList = selector->selectorList()) {
     118            for (const CSSSelector* subSelector = selectorList->first(); subSelector; subSelector = CSSSelectorList::next(subSelector)) {
     119                if (containsUncommonAttributeSelector(*subSelector, matchesRightmostElement))
     120                    return true;
     121            }
     122        }
     123
     124        if (selector->relation() != CSSSelector::SubSelector)
     125            matchesRightmostElement = false;
     126
     127        selector = selector->tagHistory();
     128    } while (selector);
    139129    return false;
     130}
     131
     132static inline bool containsUncommonAttributeSelector(const CSSSelector& rootSelector)
     133{
     134    return containsUncommonAttributeSelector(rootSelector, true);
    140135}
    141136
    … …  
    162157    , m_matchBasedOnRuleHash(static_cast<unsigned>(computeMatchBasedOnRuleHash(*selector())))
    163158    , m_canMatchPseudoElement(selectorCanMatchPseudoElement(*selector()))
    164     , m_containsUncommonAttributeSelector(WebCore::containsUncommonAttributeSelector(selector()))
     159    , m_containsUncommonAttributeSelector(WebCore::containsUncommonAttributeSelector(*selector()))
    165160    , m_linkMatchType(SelectorChecker::determineLinkMatchType(selector()))
    166161    , m_propertyWhitelistType(determinePropertyWhitelistType(addRuleFlags, selector()))

  • trunk/Source/WebCore/css/StyleResolver.cpp

    r176798 r176864  
    506506        return false;
    507507
    508     if (thisInputElement->elementData() != otherInputElement->elementData()) {
    509         if (thisInputElement->fastGetAttribute(typeAttr) != otherInputElement->fastGetAttribute(typeAttr))
    510             return false;
    511         if (thisInputElement->fastGetAttribute(readonlyAttr) != otherInputElement->fastGetAttribute(readonlyAttr))
    512             return false;
    513     }
    514 
    515508    if (thisInputElement->isAutofilled() != otherInputElement->isAutofilled())
    516509        return false;
    … …  
    647640    if (element->isLink() && state.elementLinkState() != style->insideLink())
    648641        return false;
     642
     643    if (element->elementData() != state.element()->elementData()) {
     644        if (element->fastGetAttribute(readonlyAttr) != state.element()->fastGetAttribute(readonlyAttr))
     645            return false;
     646        if (element->isSVGElement()) {
     647            if (element->getAttribute(typeAttr) != state.element()->getAttribute(typeAttr))
     648                return false;
     649        } else {
     650            if (element->fastGetAttribute(typeAttr) != state.element()->fastGetAttribute(typeAttr))
     651                return false;
     652        }
     653    }
    649654
    650655#if ENABLE(VIDEO_TRACK)
Note: See TracChangeset for help on using the changeset viewer.