Bug 10893 – InsertRule can not handle @import statements

• Component: CSS
• Status: RESOLVED FIXED

[Jan Van Boghout]

The following code:

DOMStyleSheetList *styleSheets = [[[webView mainFrame] DOMDocument] styleSheets];
DOMCSSStyleSheet *styleSheet = [styleSheets item:0];
[styleSheet insertRule:@"@import url(\"bla.css\");" :0];

throws a #12 exception, which is a syntax error. I presume it's hardcoded to accept regular style rules, which does work. I've tried several variations of the @import, using different quotes, removing the semicolon... None of them works.

[Alexey Proskuryakov]

Created attachment 10596 [details]
test case

[Joost de Valk (AlthA)]

Could someone please fix this? :)

[Rob Buis]

Created attachment 10803 [details]
First attempt

This patch allows @import as part of a rule. Let me know whether there is any better way
to include it in CSSGrammar.y.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10803 [details]
First attempt

That test does not look sufficient.

I would suggest having a <div> which is colored red, when you insert the new @import rule, it should link to a stylesheet with a more specific definition for that div which turns it green.

Otherwise the patch looks fine.

[Rob Buis]

Created attachment 10822 [details]
Improved testcases

The testcase should now be more like Eric described it.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 10822 [details]
Improved testcases

The bug fix looks great.

But I don't understand why css-insert-import-rule.html ignores exceptions. Either it should expect an exception, and fail if the expected one doesn't happen, or expect no exception, and fail if an exception occurs.

Hiding whether an exception happened or not is not a good choice for a test.

Another way to do the grammar is to have a non-terminal that's either a ruleset or an import, then we would not have to repeat the assignment statement.

I'm going to review- just for the exception issue in the test, but this change otherwise looks great!

[Rob Buis]

Created attachment 10832 [details]
Improved patch

This patch should address Darin's issues. I tested that it is in fact possible to insert the
@import at positions > 0. Since FF also allows it, I do not think it is a big deal. Let me know
whether there should be a check added if it is a big deal though.
Thanks to Maciej for helping with CSSGrammar.y.
Cheers,

Rob.

[Maciej Stachowiak]

Comment on attachment 10832 [details]
Improved patch

r=me

[Maciej Stachowiak]

Comment on attachment 10832 [details]
Improved patch

r=me

[Alexey Proskuryakov]

This was committed in r16633 and r16637.

[Maciej Stachowiak]

This seems to cause assert failures on the layout tests. Reverted for now. Not sure why it fails.

[David Kilzer (:ddkilzer)]

Comment on attachment 10832 [details]
Improved patch

Cleared review flag after bug reopeend.

[David Kilzer (:ddkilzer)]

Comment on attachment 10832 [details]
Improved patch

Really clear review flag.

[Jan Van Boghout]

Can I do something to make sure a fix for this makes it into Leopard 1.0 at least? This is absolutely crucial to me, but how WebKit's syntax definitions and automated tests all work and intertwine is pretty much a mystery to me...

[Rob Buis]

Created attachment 11174 [details]
Improved patch

This patch should eliminate the assertion failure problems. It is actually quite tricky to get this right it seems, but I didn't find any better solution than this. I don't think there is an equivalent test that would allow us to get rid of the extra bool in CSSStyleSheet, but I'd love to be proven wrong there. Also note that the testcase is slightly cleaned up.
Cheers,

Rob.

[Dave Hyatt]

Comment on attachment 11174 [details]
Improved patch

I'm not following why the extra boolean is needed.  We already have methods for figuring out if a sheet is loading (including any child imports).

[Rob Buis]

Hi,

(In reply to comment #16)
> (From update of attachment 11174 [details] [edit])
> I'm not following why the extra boolean is needed.  We already have methods for
> figuring out if a sheet is loading (including any child imports).

I think the test is different, it is not really about whether the sheet has loaded, but whether it
was processed and whether the sylesheet is signalled to the doc as loaded. So maybe the name
was wrong. Again I'd like to get rid of the bool too, but I don't see any other way. I am open
for ideas though? :)
Cheers,

Rob.
(In reply to comment #16)
> (From update of attachment 11174 [details] [edit])
> I'm not following why the extra boolean is needed.  We already have methods for
> figuring out if a sheet is loading (including any child imports).
> 

[Dave Hyatt]

Comment on attachment 11174 [details]
Improved patch

r=me

[David Kilzer (:ddkilzer)]

rwlbuis committed r17730.