Summary: | InsertRule can not handle @import statements | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jan Van Boghout <misc> | ||||||||||||
Component: | CSS | Assignee: | 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
Jan Van Boghout
2006-09-16 17:42:44 PDT
Created attachment 10596 [details]
test case
Could someone please fix this? :) 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 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.
Created attachment 10822 [details]
Improved testcases
The testcase should now be more like Eric described it.
Cheers,
Rob.
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!
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 on attachment 10832 [details]
Improved patch
r=me
Comment on attachment 10832 [details]
Improved patch
r=me
This seems to cause assert failures on the layout tests. Reverted for now. Not sure why it fails. Comment on attachment 10832 [details]
Improved patch
Cleared review flag after bug reopeend.
Comment on attachment 10832 [details]
Improved patch
Really clear review flag.
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... 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 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).
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 on attachment 11174 [details]
Improved patch
r=me
rwlbuis committed r17730. |