Changeset 87319 in webkit


Ignore:
Timestamp:
May 25, 2011 2:29:32 PM (13 years ago)
Author:
commit-queue@webkit.org
Message:

2011-05-25 Kulanthaivel Palanichamy <​kulanthaivel@codeaurora.org>

Reviewed by David Hyatt.

Selector matching doesn't update when DOM changes ("[data-a=x] #x")
​https://bugs.webkit.org/show_bug.cgi?id=60752

Added test cases for all the attribute selector types (CSS2.1 & CSS3).

  • fast/css/attribute-selector-begin-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-begin-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-contain-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-contain-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-end-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-end-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-exact-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-exact-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-hyphen-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-hyphen-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-list-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-list-dynamic-no-elementstyle.html: Added.
  • fast/css/attribute-selector-set-dynamic-no-elementstyle-expected.txt: Added.
  • fast/css/attribute-selector-set-dynamic-no-elementstyle.html: Added.

2011-05-25 Kulanthaivel Palanichamy <​kulanthaivel@codeaurora.org>

Reviewed by David Hyatt.

Selector matching doesn't update when DOM changes ("[data-a=x] #x")
​https://bugs.webkit.org/show_bug.cgi?id=60752

Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
which are used in CSS attribute selectors to determine the need for style
recalculation whenever element attributes are manipulated in DOM.
In certain conditions (element with no style, element is styled and attribute
is not a mapped attribute, attribute is of type 'type' or read-only)
even when attribute selector matches for an element, the attribute is not
added to m_selectorAttrs. This results in missing style recalculations
when a DOM element attribute is changed and is not found in m_selectorAttrs.

Removing the above said conditions in
CSSStyleSelector::SelectorChecker::checkOneSelector() for registering
attributes in m_selectorAttrs will solve this issue. But this particular
function is called numerous times which triggers adding duplicate attributes
again and again.

This patch follows the approach taken for collecting ids in selectors, where
all the attributes in selectors are added to a HashSet at the time of adding
style rules to CSSStyleSelector from StyleSheets and when
CSSStyleSelector::hasSelectorForAttribute() is called, the attribute is
simply looked up in this pre-populated hash set.

Test: fast/css/attribute-selector-dynamic-no-elementstyle.html

  • css/CSSStyleSelector.cpp: (WebCore::CSSStyleSelector::SelectorChecker::checkSelector): (WebCore::CSSStyleSelector::checkSelector): (WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector): (WebCore::collectFeaturesFromSelector): (WebCore::CSSStyleSelector::applyProperty): (WebCore::CSSStyleSelector::hasSelectorForAttribute):
  • css/CSSStyleSelector.h:
Location:
trunk
Files:
14 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r87318 r87319  
     12011-05-25  Kulanthaivel Palanichamy  <kulanthaivel@codeaurora.org>
     2
     3        Reviewed by David Hyatt.
     4
     5        Selector matching doesn't update when DOM changes ("[data-a=x] #x")
     6        https://bugs.webkit.org/show_bug.cgi?id=60752
     7       
     8        Added test cases for all the attribute selector types (CSS2.1 & CSS3).
     9
     10        * fast/css/attribute-selector-begin-dynamic-no-elementstyle-expected.txt: Added.
     11        * fast/css/attribute-selector-begin-dynamic-no-elementstyle.html: Added.
     12        * fast/css/attribute-selector-contain-dynamic-no-elementstyle-expected.txt: Added.
     13        * fast/css/attribute-selector-contain-dynamic-no-elementstyle.html: Added.
     14        * fast/css/attribute-selector-end-dynamic-no-elementstyle-expected.txt: Added.
     15        * fast/css/attribute-selector-end-dynamic-no-elementstyle.html: Added.
     16        * fast/css/attribute-selector-exact-dynamic-no-elementstyle-expected.txt: Added.
     17        * fast/css/attribute-selector-exact-dynamic-no-elementstyle.html: Added.
     18        * fast/css/attribute-selector-hyphen-dynamic-no-elementstyle-expected.txt: Added.
     19        * fast/css/attribute-selector-hyphen-dynamic-no-elementstyle.html: Added.
     20        * fast/css/attribute-selector-list-dynamic-no-elementstyle-expected.txt: Added.
     21        * fast/css/attribute-selector-list-dynamic-no-elementstyle.html: Added.
     22        * fast/css/attribute-selector-set-dynamic-no-elementstyle-expected.txt: Added.
     23        * fast/css/attribute-selector-set-dynamic-no-elementstyle.html: Added.
     24
    1252011-05-25  Adam Klein  <adamk@chromium.org>
    226

  • trunk/Source/WebCore/ChangeLog

    r87317 r87319  
     12011-05-25  Kulanthaivel Palanichamy  <kulanthaivel@codeaurora.org>
     2
     3        Reviewed by David Hyatt.
     4
     5        Selector matching doesn't update when DOM changes ("[data-a=x] #x")
     6        https://bugs.webkit.org/show_bug.cgi?id=60752
     7
     8        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
     9        which are used in CSS attribute selectors to determine the need for style
     10        recalculation whenever element attributes are manipulated in DOM.
     11        In certain conditions (element with no style, element is styled and attribute
     12        is not a mapped attribute, attribute is of type 'type' or read-only)
     13        even when attribute selector matches for an element, the attribute is not
     14        added to m_selectorAttrs. This results in missing style recalculations
     15        when a DOM element attribute is changed and is not found in m_selectorAttrs.
     16
     17        Removing the above said conditions in
     18        CSSStyleSelector::SelectorChecker::checkOneSelector() for registering
     19        attributes in m_selectorAttrs will solve this issue. But this particular
     20        function is called numerous times which triggers adding duplicate attributes
     21        again and again.
     22
     23        This patch follows the approach taken for collecting ids in selectors, where
     24        all the attributes in selectors are added to a HashSet at the time of adding
     25        style rules to CSSStyleSelector from StyleSheets and when
     26        CSSStyleSelector::hasSelectorForAttribute() is called, the attribute is
     27        simply looked up in this pre-populated hash set.
     28
     29        Test: fast/css/attribute-selector-dynamic-no-elementstyle.html
     30
     31        * css/CSSStyleSelector.cpp:
     32        (WebCore::CSSStyleSelector::SelectorChecker::checkSelector):
     33        (WebCore::CSSStyleSelector::checkSelector):
     34        (WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector):
     35        (WebCore::collectFeaturesFromSelector):
     36        (WebCore::CSSStyleSelector::applyProperty):
     37        (WebCore::CSSStyleSelector::hasSelectorForAttribute):
     38        * css/CSSStyleSelector.h:
     39
    1402011-05-25  Ryosuke Niwa  <rniwa@webkit.org>
    241

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

    r87317 r87319  
    962962{
    963963    PseudoId dynamicPseudo = NOPSEUDO;
    964     return checkSelector(sel, element, 0, dynamicPseudo, false, false) == SelectorMatches;
     964    return checkSelector(sel, element, dynamicPseudo, false, false) == SelectorMatches;
    965965}
    966966
    … …  
    20522052
    20532053    // Slow path.
    2054     SelectorMatch match = m_checker.checkSelector(ruleData.selector(), m_element, &m_selectorAttrs, m_dynamicPseudo, false, false, style(), m_parentNode ? m_parentNode->renderStyle() : 0);
     2054    SelectorMatch match = m_checker.checkSelector(ruleData.selector(), m_element, m_dynamicPseudo, false, false, style(), m_parentNode ? m_parentNode->renderStyle() : 0);
    20552055    if (match != SelectorMatches)
    20562056        return false;
    … …  
    21812181// * SelectorFailsLocally    - the selector fails for the element e
    21822182// * SelectorFailsCompletely - the selector fails for e and any sibling or ancestor of e
    2183 CSSStyleSelector::SelectorMatch CSSStyleSelector::SelectorChecker::checkSelector(CSSSelector* sel, Element* e, HashSet<AtomicStringImpl*>* selectorAttrs, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* elementStyle, RenderStyle* elementParentStyle) const
     2183CSSStyleSelector::SelectorMatch CSSStyleSelector::SelectorChecker::checkSelector(CSSSelector* sel, Element* e, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* elementStyle, RenderStyle* elementParentStyle) const
    21842184{
    21852185#if ENABLE(SVG)
    … …  
    21912191
    21922192    // first selector has to match
    2193     if (!checkOneSelector(sel, e, selectorAttrs, dynamicPseudo, isSubSelector, encounteredLink, elementStyle, elementParentStyle))
     2193    if (!checkOneSelector(sel, e, dynamicPseudo, isSubSelector, encounteredLink, elementStyle, elementParentStyle))
    21942194        return SelectorFailsLocally;
    21952195
    … …  
    22252225                    return SelectorFailsCompletely;
    22262226                e = static_cast<Element*>(n);
    2227                 SelectorMatch match = checkSelector(sel, e, selectorAttrs, dynamicPseudo, false, encounteredLink);
     2227                SelectorMatch match = checkSelector(sel, e, dynamicPseudo, false, encounteredLink);
    22282228                if (match != SelectorFailsLocally)
    22292229                    return match;
    … …  
    22362236                return SelectorFailsCompletely;
    22372237            e = static_cast<Element*>(n);
    2238             return checkSelector(sel, e, selectorAttrs, dynamicPseudo, false, encounteredLink);
     2238            return checkSelector(sel, e, dynamicPseudo, false, encounteredLink);
    22392239        }
    22402240        case CSSSelector::DirectAdjacent:
    … …  
    22522252            e = static_cast<Element*>(n);
    22532253            m_matchVisitedPseudoClass = false;
    2254             return checkSelector(sel, e, selectorAttrs, dynamicPseudo, false, encounteredLink);
     2254            return checkSelector(sel, e, dynamicPseudo, false, encounteredLink);
    22552255        }
    22562256        case CSSSelector::IndirectAdjacent:
    … …  
    22682268                e = static_cast<Element*>(n);
    22692269                m_matchVisitedPseudoClass = false;
    2270                 SelectorMatch match = checkSelector(sel, e, selectorAttrs, dynamicPseudo, false, encounteredLink);
     2270                SelectorMatch match = checkSelector(sel, e, dynamicPseudo, false, encounteredLink);
    22712271                if (match != SelectorFailsLocally)
    22722272                    return match;
    … …  
    22802280                !((RenderScrollbar::scrollbarForStyleResolve() || dynamicPseudo == SCROLLBAR_CORNER || dynamicPseudo == RESIZER) && sel->m_match == CSSSelector::PseudoClass))
    22812281                return SelectorFailsCompletely;
    2282             return checkSelector(sel, e, selectorAttrs, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle);
     2282            return checkSelector(sel, e, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle);
    22832283        case CSSSelector::ShadowDescendant:
    22842284        {
    … …  
    22872287                return SelectorFailsCompletely;
    22882288            e = static_cast<Element*>(shadowHostNode);
    2289             return checkSelector(sel, e, selectorAttrs, dynamicPseudo, false, encounteredLink);
     2289            return checkSelector(sel, e, dynamicPseudo, false, encounteredLink);
    22902290        }
    22912291    }
    … …  
    23612361}
    23622362
    2363 bool CSSStyleSelector::SelectorChecker::checkOneSelector(CSSSelector* sel, Element* e, HashSet<AtomicStringImpl*>* selectorAttrs, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* elementStyle, RenderStyle* elementParentStyle) const
     2363bool CSSStyleSelector::SelectorChecker::checkOneSelector(CSSSelector* sel, Element* e, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* elementStyle, RenderStyle* elementParentStyle) const
    23642364{
    23652365    ASSERT(e);
    … …  
    23822382        if (elementStyle && (!e->isStyledElement() || (!static_cast<StyledElement*>(e)->isMappedAttribute(attr) && attr != typeAttr && attr != readonlyAttr))) {
    23832383            elementStyle->setAffectedByAttributeSelectors(); // Special-case the "type" and "readonly" attributes so input form controls can share style.
    2384             if (selectorAttrs)
    2385                 selectorAttrs->add(attr.localName().impl());
    23862384        }
    23872385
    … …  
    24572455                ASSERT(subSel->pseudoType() != CSSSelector::PseudoNot);
    24582456
    2459                 if (!checkOneSelector(subSel, e, selectorAttrs, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle))
     2457                if (!checkOneSelector(subSel, e, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle))
    24602458                    return true;
    24612459            }
    … …  
    27662764            case CSSSelector::PseudoAny:
    27672765                for (CSSSelector* selector = sel->selectorList()->first(); selector; selector = CSSSelectorList::next(selector)) {
    2768                     if (checkSelector(selector, e, selectorAttrs, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle) == SelectorMatches)
     2766                    if (checkSelector(selector, e, dynamicPseudo, true, encounteredLink, elementStyle, elementParentStyle) == SelectorMatches)
    27692767                        return true;
    27702768                }
    … …  
    32353233    if (selector->m_match == CSSSelector::Id && !selector->value().isEmpty())
    32363234        features.idsInRules.add(selector->value().impl());
     3235    if (selector->hasAttribute()) {
     3236        switch (selector->m_match) {
     3237        case CSSSelector::Exact:
     3238        case CSSSelector::Set:
     3239        case CSSSelector::List:
     3240        case CSSSelector::Hyphen:
     3241        case CSSSelector::Contain:
     3242        case CSSSelector::Begin:
     3243        case CSSSelector::End:
     3244            features.attrsInRules.add(selector->attribute().localName().impl());
     3245            break;
     3246        default:
     3247            break;
     3248        }
     3249    }
    32373250    switch (selector->pseudoType()) {
    32383251    case CSSSelector::PseudoFirstLine:
    … …  
    42234236                didSet = true;
    42244237                // register the fact that the attribute value affects the style
    4225                 m_selectorAttrs.add(attr.localName().impl());
     4238                m_features.attrsInRules.add(attr.localName().impl());
    42264239                break;
    42274240            }
    … …  
    65226535bool CSSStyleSelector::hasSelectorForAttribute(const AtomicString &attrname) const
    65236536{
    6524     return m_selectorAttrs.contains(attrname.impl());
     6537    return m_features.attrsInRules.contains(attrname.impl());
    65256538}
    65266539

  • trunk/Source/WebCore/css/CSSStyleSelector.h

    r87317 r87319  
    192192            ~Features();
    193193            HashSet<AtomicStringImpl*> idsInRules;
     194            HashSet<AtomicStringImpl*> attrsInRules;
    194195            OwnPtr<RuleSet> siblingRules;
    195196            bool usesFirstLineRules;
    … …  
    262263
    263264            bool checkSelector(CSSSelector*, Element*) const;
    264             SelectorMatch checkSelector(CSSSelector*, Element*, HashSet<AtomicStringImpl*>* selectorAttrs, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* = 0, RenderStyle* elementParentStyle = 0) const;
    265             bool checkOneSelector(CSSSelector*, Element*, HashSet<AtomicStringImpl*>* selectorAttrs, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle*, RenderStyle* elementParentStyle) const;
     265            SelectorMatch checkSelector(CSSSelector*, Element*, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle* = 0, RenderStyle* elementParentStyle = 0) const;
     266            bool checkOneSelector(CSSSelector*, Element*, PseudoId& dynamicPseudo, bool isSubSelector, bool encounteredLink, RenderStyle*, RenderStyle* elementParentStyle) const;
    266267            bool checkScrollbarPseudoClass(CSSSelector*, PseudoId& dynamicPseudo) const;
    267268            static bool fastCheckSelector(const CSSSelector*, const Element*);
    … …  
    360361       
    361362        RefPtr<CSSFontSelector> m_fontSelector;
    362         HashSet<AtomicStringImpl*> m_selectorAttrs;
    363363        Vector<CSSMutableStyleDeclaration*> m_additionalAttributeStyleDecls;
    364364        Vector<MediaQueryResult*> m_viewportDependentMediaQueryResults;
Note: See TracChangeset for help on using the changeset viewer.