Bug 56981 - CSSStyleSheet#insertRule doesn't work well with imported stylesheets
: CSSStyleSheet#insertRule doesn't work well with imported stylesheets
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To:
: http://elv1s.ru/files/dom/insertRule-...
: HasReduction
:
:
  Show dependency treegraph
 
Reported: 2011-03-23 18:00 PST by
Modified: 2011-06-01 17:54 PST (History)


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 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Updated test case - should reduce the flakiness & solve Simon's comments (10.60 KB, patch)
2011-05-26 10:08 PST, Julien Chaffraix
simon.fraser: review+
simon.fraser: commit‑queue+
Review Patch | Details | Formatted Diff | Diff
Updated patch with an (hopefully) even better test case (10.64 KB, patch)
2011-05-26 17:03 PST, Julien Chaffraix
jchaffraix: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-23 18:00:12 PST
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.
------- Comment #1 From 2011-03-24 10:57:42 PST -------
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.
------- Comment #2 From 2011-05-18 19:53:36 PST -------
(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.
------- Comment #3 From 2011-05-18 19:58:45 PST -------
Created an attachment (id=94035) [details]
Proposed fix: Make <link> aware of the dynamic stylesheet so that it properly removes them from the pending sheets
------- Comment #4 From 2011-05-24 17:32:18 PST -------
(From update of attachment 94035 [details])
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 #5 From 2011-05-25 11:49:18 PST -------
(From update of attachment 94035 [details])
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.
------- Comment #6 From 2011-05-26 10:08:10 PST -------
Created an attachment (id=94996) [details]
Updated test case - should reduce the flakiness & solve Simon's comments
------- Comment #7 From 2011-05-26 10:19:17 PST -------
(From update of attachment 94996 [details])
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 #8 From 2011-05-26 15:58:04 PST -------
(From update of attachment 94996 [details])
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!
------- Comment #9 From 2011-05-26 17:03:06 PST -------
Created an attachment (id=95081) [details]
Updated patch with an (hopefully) even better test case
------- Comment #10 From 2011-06-01 17:53:25 PST -------
Committed r87867: <http://trac.webkit.org/changeset/87867>
------- Comment #11 From 2011-06-01 17:54:22 PST -------
(From update of attachment 95081 [details])
r- this patch as I landed the other one.