Bug 60752 – Selector matching doesn't update when DOM changes ("[data-a=x] #x")

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

[Ian 'Hixie' Hickson]

TESTCASE
http://damowmow.com/playground/bugs/selector-update/001.html

Works in Opera and Firefox.

[Dominic Cooney]

*** Bug 60861 has been marked as a duplicate of this bug. ***

[Kulanthaivel Palanichamy]

Created attachment 93983 [details]
Proposed patch.

[WebKit Review Bot]

Comment on attachment 93983 [details]
Proposed patch.

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

New failing tests:
fast/css-generated-content/016.html

[WebKit Review Bot]

Created attachment 93994 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kulanthaivel Palanichamy]

Created attachment 94026 [details]
Proposed patch.

Fixed the layout test failure.

[Dominic Cooney]

Comment on attachment 94026 [details]
Proposed patch.

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

> LayoutTests/fast/css/attribute-selector-dynamic-no-elementstyle-expected.txt:3
> +This test checks whether CSS attribute selectors are re-evaluated after attribute changes in html elements which are having no style associated with them.

html elements -> elements (this isn’t HTML-specific, right?)

are having -> have

> Source/WebCore/ChangeLog:8
> +        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)

Is it worth beefing up the test coverage for these specific cases?

> Source/WebCore/ChangeLog:27
> +        simply looked up in this pre-papulated hash set.

populated

> Source/WebCore/css/CSSStyleSelector.cpp:2379
>          // FIXME: Handle the case were elementStyle is 0.

Is this FIXME still valid?

[Kulanthaivel Palanichamy]

Comment on attachment 94026 [details]
Proposed patch.

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

>> LayoutTests/fast/css/attribute-selector-dynamic-no-elementstyle-expected.txt:3
>> +This test checks whether CSS attribute selectors are re-evaluated after attribute changes in html elements which are having no style associated with them.
> 
> html elements -> elements (this isn’t HTML-specific, right?)
> 
> are having -> have

you are correct, this isn't HTML specific

>> Source/WebCore/ChangeLog:8
>> +        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
> 
> Is it worth beefing up the test coverage for these specific cases?

Well, all of the attribute selectors cannot be covered in one test as if any one of the attribute change triggers style recalculation, then the previously failed attribute test results will not propagate till the end of the test.
How about writing separate test cases for each one of the 7 attribute selector types?

>> Source/WebCore/ChangeLog:27
>> +        simply looked up in this pre-papulated hash set.
> 
> populated

Sorry for the spelling and grammar mistakes. I'll correct them all.

>> Source/WebCore/css/CSSStyleSelector.cpp:2379
>>          // FIXME: Handle the case were elementStyle is 0.
> 
> Is this FIXME still valid?

Yes, it is still valid. 
Consider the scenario given in the test case

span { display: none }
*[test="0"] #sp { color: green; display: block} 

<div test="1">
    <span id="sp">PASSED</span>
    </div>
- Here the <div> element has no elementStyle, but it's attribute selector triggers style change on <span> so <span>'s style shouldn't be sharable. But when <span>'s style recalculation is done, its attribute match type is 'id' and it will never hit this condition thus makes its style still sharable.

[Kulanthaivel Palanichamy]

Comment on attachment 94026 [details]
Proposed patch.

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

>> LayoutTests/fast/css/attribute-selector-dynamic-no-elementstyle-expected.txt:3
>> +This test checks whether CSS attribute selectors are re-evaluated after attribute changes in html elements which are having no style associated with them.
> 
> html elements -> elements (this isn’t HTML-specific, right?)
> 
> are having -> have

you are correct, this isn't HTML specific

>> Source/WebCore/ChangeLog:8
>> +        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
> 
> Is it worth beefing up the test coverage for these specific cases?

Well, all of the attribute selectors cannot be covered in one test as if any one of the attribute change triggers style recalculation, then the previously failed attribute test results will not propagate till the end of the test.
How about writing separate test cases for each one of the 7 attribute selector types?

>> Source/WebCore/ChangeLog:27
>> +        simply looked up in this pre-papulated hash set.
> 
> populated

Sorry for the spelling and grammar mistakes. I'll correct them all.

>> Source/WebCore/css/CSSStyleSelector.cpp:2379
>>          // FIXME: Handle the case were elementStyle is 0.
> 
> Is this FIXME still valid?

Yes, it is still valid. 
Consider the scenario given in the test case

span { display: none }
*[test="0"] #sp { color: green; display: block} 

<div test="1">
    <span id="sp">PASSED</span>
    </div>
- Here the <div> element has no elementStyle, but it's attribute selector triggers style change on <span> so <span>'s style shouldn't be sharable. But when <span>'s style recalculation is done, its attribute match type is 'id' and it will never hit this condition thus makes its style still sharable.

[Kulanthaivel Palanichamy]

Comment on attachment 94026 [details]
Proposed patch.

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

>> LayoutTests/fast/css/attribute-selector-dynamic-no-elementstyle-expected.txt:3
>> +This test checks whether CSS attribute selectors are re-evaluated after attribute changes in html elements which are having no style associated with them.
> 
> html elements -> elements (this isn’t HTML-specific, right?)
> 
> are having -> have

you are correct, this isn't HTML specific

>> Source/WebCore/ChangeLog:8
>> +        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
> 
> Is it worth beefing up the test coverage for these specific cases?

Well, all of the attribute selectors cannot be covered in one test as if any one of the attribute change triggers style recalculation, then the previously failed attribute test results will not propagate till the end of the test.
How about writing separate test cases for each one of the 7 attribute selector types?

>> Source/WebCore/ChangeLog:27
>> +        simply looked up in this pre-papulated hash set.
> 
> populated

Sorry for the spelling and grammar mistakes. I'll correct them all.

>> Source/WebCore/css/CSSStyleSelector.cpp:2379
>>          // FIXME: Handle the case were elementStyle is 0.
> 
> Is this FIXME still valid?

Yes, it is still valid. 
Consider the scenario given in the test case

span { display: none }
*[test="0"] #sp { color: green; display: block} 

<div test="1">
    <span id="sp">PASSED</span>
    </div>
- Here the <div> element has no elementStyle, but it's attribute selector triggers style change on <span> so <span>'s style shouldn't be sharable. But when <span>'s style recalculation is done, its attribute match type is 'id' and it will never hit this condition thus makes its style still sharable.

[Kulanthaivel Palanichamy]

Comment on attachment 94026 [details]
Proposed patch.

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

>> LayoutTests/fast/css/attribute-selector-dynamic-no-elementstyle-expected.txt:3
>> +This test checks whether CSS attribute selectors are re-evaluated after attribute changes in html elements which are having no style associated with them.
> 
> html elements -> elements (this isn’t HTML-specific, right?)
> 
> are having -> have

you are correct, this isn't HTML specific

>> Source/WebCore/ChangeLog:8
>> +        Currently CSSStyleSelector maintains a HashSet of attributes (m_selectorAttrs)
> 
> Is it worth beefing up the test coverage for these specific cases?

Well, all of the attribute selectors cannot be covered in one test as if any one of the attribute change triggers style recalculation, then the previously failed attribute test results will not propagate till the end of the test.
How about writing separate test cases for each one of the 7 attribute selector types?

>> Source/WebCore/ChangeLog:27
>> +        simply looked up in this pre-papulated hash set.
> 
> populated

Sorry for the spelling and grammar mistakes. I'll correct them all.

>> Source/WebCore/css/CSSStyleSelector.cpp:2379
>>          // FIXME: Handle the case were elementStyle is 0.
> 
> Is this FIXME still valid?

Yes, it is still valid. 
Consider the scenario given in the test case

span { display: none }
*[test="0"] #sp { color: green; display: block} 

<div test="1">
    <span id="sp">PASSED</span>
    </div>
- Here the <div> element has no elementStyle, but it's attribute selector triggers style change on <span> so <span>'s style shouldn't be sharable. But when <span>'s style recalculation is done, its attribute match type is 'id' and it will never hit this condition thus makes its style still sharable.

[Kulanthaivel Palanichamy]

Created attachment 94106 [details]
Proposed patch.

Don't mind the above duplicate comments. It looks like Bugzilla re-submits the form whenever the page is refreshed :( .

[Dave Hyatt]

Comment on attachment 94106 [details]
Proposed patch.

Looks good.

r=me

[WebKit Commit Bot]

Comment on attachment 94106 [details]
Proposed patch.

Clearing flags on attachment: 94106

Committed r87319: <http://trac.webkit.org/changeset/87319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dominic Cooney]

*** Bug 12519 has been marked as a duplicate of this bug. ***