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

Ian 'Hixie' Hickson
Reported 2011-05-12 23:37:06 PDT
Attachments
Proposed patch. (17.48 KB, patch)
2011-05-18 14:28 PDT, Kulanthaivel Palanichamy
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 (1.17 MB, application/zip)
2011-05-18 15:50 PDT, WebKit Review Bot
no flags
Proposed patch. (17.47 KB, patch)
2011-05-18 19:04 PDT, Kulanthaivel Palanichamy
no flags
Proposed patch. (31.49 KB, patch)
2011-05-19 13:26 PDT, Kulanthaivel Palanichamy
no flags
Dominic Cooney
Comment 1 2011-05-15 22:43:01 PDT
*** Bug 60861 has been marked as a duplicate of this bug. ***
Kulanthaivel Palanichamy
Comment 2 2011-05-18 14:28:31 PDT
Created attachment 93983 [details] Proposed patch.
WebKit Review Bot
Comment 3 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
WebKit Review Bot
Comment 4 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
Kulanthaivel Palanichamy
Comment 5 2011-05-18 19:04:59 PDT
Created attachment 94026 [details] Proposed patch. Fixed the layout test failure.
Dominic Cooney
Comment 6 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?
Kulanthaivel Palanichamy
Comment 7 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.
Kulanthaivel Palanichamy
Comment 8 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.
Kulanthaivel Palanichamy
Comment 9 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.
Kulanthaivel Palanichamy
Comment 10 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.
Kulanthaivel Palanichamy
Comment 11 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 :( .
Dave Hyatt
Comment 12 2011-05-25 11:25:10 PDT
Comment on attachment 94106 [details] Proposed patch. Looks good. r=me
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2011-05-25 14:29:53 PDT
All reviewed patches have been landed. Closing bug.
Dominic Cooney
Comment 15 2011-06-05 23:15:40 PDT
*** Bug 12519 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.