| Summary: | Selector matching doesn't update when DOM changes ("[data-a=x] #x") | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ian 'Hixie' Hickson <ian> | ||||||||||
| Component: | CSS | Assignee: | 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
Ian 'Hixie' Hickson
2011-05-12 23:37:06 PDT
*** Bug 60861 has been marked as a duplicate of this bug. *** Created attachment 93983 [details]
Proposed patch.
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 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
Created attachment 94026 [details]
Proposed patch.
Fixed the layout test failure.
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 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 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 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 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. 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 on attachment 94106 [details]
Proposed patch.
Looks good.
r=me
Comment on attachment 94106 [details] Proposed patch. Clearing flags on attachment: 94106 Committed r87319: <http://trac.webkit.org/changeset/87319> All reviewed patches have been landed. Closing bug. *** Bug 12519 has been marked as a duplicate of this bug. *** |