Bug 10893

Summary: InsertRule can not handle @import statements
Product: WebKit Reporter: Jan Van Boghout <misc>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ap, rwlbuis
Priority: P2 Keywords: HasReduction
Version: 420+   
Hardware: Mac (Intel)   
OS: OS X 10.4   
Attachments:
Description Flags
test case
none
First attempt
eric: review-
Improved testcases
darin: review-
Improved patch
none
Improved patch hyatt: review+

Description Jan Van Boghout 2006-09-16 17:42:44 PDT
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.
Comment 1 Alexey Proskuryakov 2006-09-17 00:28:27 PDT
Created attachment 10596 [details]
test case
Comment 2 Joost de Valk (AlthA) 2006-09-19 04:40:21 PDT
Could someone please fix this? :)
Comment 3 Rob Buis 2006-09-27 12:52:46 PDT
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.
Comment 4 Eric Seidel (no email) 2006-09-27 13:44:30 PDT
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.
Comment 5 Rob Buis 2006-09-28 04:37:36 PDT
Created attachment 10822 [details]
Improved testcases

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

Rob.
Comment 6 Darin Adler 2006-09-28 09:08:26 PDT
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!
Comment 7 Rob Buis 2006-09-29 01:18:34 PDT
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.
Comment 8 Maciej Stachowiak 2006-09-29 01:25:20 PDT
Comment on attachment 10832 [details]
Improved patch

r=me
Comment 9 Maciej Stachowiak 2006-09-29 01:25:21 PDT
Comment on attachment 10832 [details]
Improved patch

r=me
Comment 10 Alexey Proskuryakov 2006-09-30 08:26:51 PDT
This was committed in r16633 and r16637.
Comment 11 Maciej Stachowiak 2006-10-01 22:19:19 PDT
This seems to cause assert failures on the layout tests. Reverted for now. Not sure why it fails.
Comment 12 David Kilzer (:ddkilzer) 2006-10-02 03:25:46 PDT
Comment on attachment 10832 [details]
Improved patch

Cleared review flag after bug reopeend.
Comment 13 David Kilzer (:ddkilzer) 2006-10-02 03:26:13 PDT
Comment on attachment 10832 [details]
Improved patch

Really clear review flag.
Comment 14 Jan Van Boghout 2006-10-20 13:57:56 PDT
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...
Comment 15 Rob Buis 2006-10-21 06:25:44 PDT
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.
Comment 16 Dave Hyatt 2006-10-21 14:30:40 PDT
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).
Comment 17 Rob Buis 2006-10-22 13:16:45 PDT
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).
> 

Comment 18 Dave Hyatt 2006-11-11 00:36:49 PST
Comment on attachment 11174 [details]
Improved patch

r=me
Comment 19 David Kilzer (:ddkilzer) 2006-11-11 13:32:06 PST
rwlbuis committed r17730.