Bug 60752

Summary: Selector matching doesn't update when DOM changes ("[data-a=x] #x")
Product: WebKit Reporter: Ian 'Hixie' Hickson <ian>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, eric, hyatt, kulanthaivel, lea, mjs, noel.gordon, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://damowmow.com/playground/bugs/selector-update/001.html
Attachments:
Description Flags
Proposed patch.
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
Proposed patch.
none
Proposed patch. none

Description Ian 'Hixie' Hickson 2011-05-12 23:37:06 PDT
TESTCASE
http://damowmow.com/playground/bugs/selector-update/001.html

Works in Opera and Firefox.
Comment 1 Dominic Cooney 2011-05-15 22:43:01 PDT
*** Bug 60861 has been marked as a duplicate of this bug. ***
Comment 2 Kulanthaivel Palanichamy 2011-05-18 14:28:31 PDT
Created attachment 93983 [details]
Proposed patch.
Comment 3 WebKit Review Bot 2011-05-18 15:50:20 PDT
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
Comment 4 WebKit Review Bot 2011-05-18 15:50:25 PDT
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
Comment 5 Kulanthaivel Palanichamy 2011-05-18 19:04:59 PDT
Created attachment 94026 [details]
Proposed patch.

Fixed the layout test failure.
Comment 6 Dominic Cooney 2011-05-18 22:25:34 PDT
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?
Comment 7 Kulanthaivel Palanichamy 2011-05-19 10:13:36 PDT
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.
Comment 8 Kulanthaivel Palanichamy 2011-05-19 10:21:03 PDT
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.
Comment 9 Kulanthaivel Palanichamy 2011-05-19 10:37:37 PDT
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.
Comment 10 Kulanthaivel Palanichamy 2011-05-19 10:41:52 PDT
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.
Comment 11 Kulanthaivel Palanichamy 2011-05-19 13:26:46 PDT
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 :( .
Comment 12 Dave Hyatt 2011-05-25 11:25:10 PDT
Comment on attachment 94106 [details]
Proposed patch.

Looks good.

r=me
Comment 13 WebKit Commit Bot 2011-05-25 14:29:45 PDT
Comment on attachment 94106 [details]
Proposed patch.

Clearing flags on attachment: 94106

Committed r87319: <http://trac.webkit.org/changeset/87319>
Comment 14 WebKit Commit Bot 2011-05-25 14:29:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Dominic Cooney 2011-06-05 23:15:40 PDT
*** Bug 12519 has been marked as a duplicate of this bug. ***