RESOLVED FIXED56981
CSSStyleSheet#insertRule doesn't work well with imported stylesheets 
https://bugs.webkit.org/show_bug.cgi?id=56981
Summary CSSStyleSheet#insertRule doesn't work well with imported stylesheets
Nikita Vasilyev
Reported 2011-03-23 18:00:12 PDT
How to reproduce: Open http://elv1s.ru/files/dom/insertRule-webkit-bug/ Expected: "PASS" on green background. Actual: On first load you will see: — "FAIL" on white background in latest WebKit. — "FAIL" on green background in Safari 5.0.4. After reloading the page, you will see "PASS" on green background. The reduction works well in Firefox 4 and Opera 11.01.
Attachments
Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets (10.47 KB, patch)
2011-05-18 19:58 PDT, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Updated test case - should reduce the flakiness & solve Simon's comments (10.60 KB, patch)
2011-05-26 10:08 PDT, Julien Chaffraix 		simon.fraser: review+
simon.fraser: commit-queue+
DetailsFormatted DiffDiff
Updated patch with an (hopefully) even better test case (10.64 KB, patch)
2011-05-26 17:03 PDT, Julien Chaffraix 		jchaffraix: review-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2011-03-24 10:57:42 PDT
The FAIL output is caused by the stylesheet temporarily disappearing from document.styleSheets after an @import rule is added to it. Seems clearly wrong. "FAIL" on white background in ToT WebKit looks like a regression to me, not sure what caused it.
Julien Chaffraix
Comment 2 2011-05-18 19:53:36 PDT
(In reply to comment #1) > The FAIL output is caused by the stylesheet temporarily disappearing from document.styleSheets after an @import rule is added to it. Seems clearly wrong. This is caused by Document::recalcStyleSelector() removing the style sheets while they have at least a loading rule. > "FAIL" on white background in ToT WebKit looks like a regression to me, not sure what caused it. I am not sure what caused this but the root cause is HTMLLinkElement not calling Document::removePendingSheet for dynamically loaded sheet. It is related to the new {add|remove}PendingSheet logic in HTMLLinkElement. I have a patch that solves this problem that I will attach to this bug.
Julien Chaffraix
Comment 3 2011-05-18 19:58:45 PDT
Created attachment 94035 [details] Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets
Simon Fraser (smfr)
Comment 4 2011-05-24 17:32:18 PDT
Comment on attachment 94035 [details] Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets View in context: https://bugs.webkit.org/attachment.cgi?id=94035&action=review The source changes look OK but I think the test could be improved. > LayoutTests/fast/css/import-and-insert-rule-no-update.html:1 > +<!DOCTYPE HTML> html is usually lowercase > LayoutTests/fast/css/import-and-insert-rule-no-update.html:3 > +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> Remove this line. > LayoutTests/fast/css/import-and-insert-rule-no-update.html:8 > +<div id="testArea"> </div> Remove space > LayoutTests/fast/css/import-and-insert-rule-no-update.html:33 > + document.styleSheets[0].insertRule('@import "green.css";', 1); > + // We need to wait some time to let the stylesheet load before testing. > + window.setTimeout(test, 100); This will be prone to flakiness. Is there some other way to test when the stylesheet has loaded? Maybe poll for the computed style of some other element?
Julien Chaffraix
Comment 5 2011-05-25 11:49:18 PDT
Comment on attachment 94035 [details] Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets View in context: https://bugs.webkit.org/attachment.cgi?id=94035&action=review >> LayoutTests/fast/css/import-and-insert-rule-no-update.html:1 >> +<!DOCTYPE HTML> > > html is usually lowercase Sure, thought it should not impact the document mode. >> LayoutTests/fast/css/import-and-insert-rule-no-update.html:3 >> +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> > > Remove this line. Done. >> LayoutTests/fast/css/import-and-insert-rule-no-update.html:8 >> +<div id="testArea"> </div> > > Remove space Done. >> LayoutTests/fast/css/import-and-insert-rule-no-update.html:33 >> + window.setTimeout(test, 100); > > This will be prone to flakiness. Is there some other way to test when the stylesheet has loaded? Maybe poll for the computed style of some other element? Good catch, we could do a certain number of retries to leave enough room for the stylesheet to load. The current problem is that the new stylesheet is not applied thus we could poll forever. I don't see a reliable way to get the 'stylesheet loaded' information but we can mitigate the flakiness. Let me know what you think of that.
Julien Chaffraix
Comment 6 2011-05-26 10:08:10 PDT
Created attachment 94996 [details] Updated test case - should reduce the flakiness & solve Simon's comments
Simon Fraser (smfr)
Comment 7 2011-05-26 10:19:17 PDT
Comment on attachment 94996 [details] Updated test case - should reduce the flakiness & solve Simon's comments View in context: https://bugs.webkit.org/attachment.cgi?id=94996&action=review > LayoutTests/fast/css/import-and-insert-rule-no-update.html:39 > + var testArea = document.getElementById("testArea"); > + if (getComputedStyle(testArea).backgroundColor == "rgb(0, 128, 0)") { > + testArea.innerHTML = 'PASS'; > + remainingTests = 0; > + } else { > + if (--remainingTests) > + testArea.innerHTML = 'FAIL, backgroundColor was ' + getComputedStyle(testArea).backgroundColor; > + } > + } catch (e) { > + testArea.innerHTML = 'FAIL, exception raised (' + e.message + ')'; > + remainingTests = 0; > + } > + if (!remainingTests) > + window.setTimeout(test, 25); > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > +} > + > +window.onload = function() { > + document.styleSheets[0].insertRule('@import "green.css";', 1); > + // We need to wait some time to let the stylesheet load before testing. > + window.setTimeout(test, 25); This is a confusing way to wait for the stylesheet. I'd much prefer to centralize the wait logic in one function, which should call the "doTest" function when it detects that the load is complete. Do you expect exceptions to fire here? We don't normally test for them.
Julien Chaffraix
Comment 8 2011-05-26 15:58:04 PDT
Comment on attachment 94996 [details] Updated test case - should reduce the flakiness & solve Simon's comments View in context: https://bugs.webkit.org/attachment.cgi?id=94996&action=review >> LayoutTests/fast/css/import-and-insert-rule-no-update.html:39 >> + window.setTimeout(test, 25); > > This is a confusing way to wait for the stylesheet. I'd much prefer to centralize the wait logic in one function, which should call the "doTest" function when it detects that the load is complete. > > Do you expect exceptions to fire here? We don't normally test for them. I don't expect any exceptions here. Let me remove that and rework the logic. Thanks!
Julien Chaffraix
Comment 9 2011-05-26 17:03:06 PDT
Created attachment 95081 [details] Updated patch with an (hopefully) even better test case
Julien Chaffraix
Comment 10 2011-06-01 17:53:25 PDT
Committed r87867: <http://trac.webkit.org/changeset/87867>
Julien Chaffraix
Comment 11 2011-06-01 17:54:22 PDT
Comment on attachment 95081 [details] Updated patch with an (hopefully) even better test case r- this patch as I landed the other one.
Note You need to log in before you can comment on or make changes to this bug.