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.
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.
(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.
Created attachment 94035 [details] Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets
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?
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.
Created attachment 94996 [details] Updated test case - should reduce the flakiness & solve Simon's comments
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.
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!
Created attachment 95081 [details] Updated patch with an (hopefully) even better test case
Committed r87867: <http://trac.webkit.org/changeset/87867>
Comment on attachment 95081 [details] Updated patch with an (hopefully) even better test case r- this patch as I landed the other one.