Bug 56981

Summary:

CSSStyleSheet#insertRule doesn't work well with imported stylesheets

Product:

WebKit

Reporter:

Nikita Vasilyev <me>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, hyatt, jchaffraix, koivisto, simon.fraser

Priority:

Keywords:

HasReduction

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

URL:

http://elv1s.ru/files/dom/insertRule-webkit-bug/

Attachments:

Description	Flags
Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets	none
Updated test case - should reduce the flakiness & solve Simon's comments	simon.fraser: review+, simon.fraser: commit-queue+
Updated patch with an (hopefully) even better test case	jchaffraix: review-

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	Details Formatted Diff Diff
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+	Details Formatted Diff Diff
Updated patch with an (hopefully) even better test case (10.64 KB, patch) 2011-05-26 17:03 PDT, Julien Chaffraix	jchaffraix: review-	Details Formatted Diff Diff
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

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.