Summary: | CSS3 "Property is declared twice in rule" test fails | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, hyatt, jamesr, kling, rwlbuis, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | OS X 10.5 | ||||||||
URL: | http://samples.msdn.microsoft.com/ietestcenter/domstyle/duplicateRule.htm | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 53470 | ||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2010-03-17 20:33:17 PDT
Created attachment 57994 [details]
Patch
Attachment 57994 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSMutableStyleDeclaration.cpp:90: Missing space before ( in if( [whitespace/parens] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #1) > Created an attachment (id=57994) [details] > Patch This is just a first stab and at least has one style problem. I uploaded it anyway to get some early feedback. If I understand it correctly, earlier !important properties should not be removed, hence the check. However additionaly there seems a problem with how to treat padding. One of the failing cases was padding: 5em; padding: -1em. Before my patch -1em was discarded so 5em "won" hence the test passing. With my patch 5em is removed and later on -1em is discarded. When I inspect this rule in Opera, it doesn't even mention the -1em, so I am guessing there it is stripped out in the css parsing phase. Since I would not like special casing CSSPaddingProperty (and there may be even more exceptions) I am not sure how to fix that. Should we try to discard negative padding earlier? Will that cause problems with other testcases? Cheers, Rob. Created attachment 59022 [details]
Patch
(In reply to comment #4) > Created an attachment (id=59022) [details] > Patch Some clarification, I had to change the padding handling to discard negative padding earlier, i.e. when parsing instead of only at style selection time. The reason is that the duplicate property filtering should not have the knowledge to discard negative padding. The filtering could maybe be put in a private method that can be reused, I'll have to look in the similar FIXME spot and another place in CSSMutableStyleDeclration.cpp that seems to do filtering. I didnt try to make the filtering as fast as possible yet, it seems to me it will always be a bit slower than the current code, otoh I assume duplicate properties are rare. What I assume may need to be done is adding tests, I have some ideas for that but suggestions are welcome. The patch does not cause new regressions. Cheers, Rob. Comment on attachment 59022 [details]
Patch
This looks good. You did the right thing letting variable-dependent values have duplicates too. :)
Looks like the patch broke these tests: editing/pasteboard/5478250.html editing/selection/extend-by-word-002.html editing/style/smoosh-styles-003.html on the OS X, Qt, and chromium layout test bots. Reverted r61340 for reason: Broke several editing tests. Committed r61345: <http://trac.webkit.org/changeset/61345> We chatted on IRC, looks like the problem is that this patch discards negative text indents as well as negative padding, which is wrong. Rob said he'd fix it tomorrow and put the patch back up (it looks pretty straightforward). http://trac.webkit.org/changeset/61345 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/61344 http://trac.webkit.org/changeset/61345 http://trac.webkit.org/changeset/61340 http://trac.webkit.org/changeset/61340 might have broken Tiger Intel Release, Leopard Intel Release (Tests), and Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/61344 http://trac.webkit.org/changeset/61345 http://trac.webkit.org/changeset/61340 This was ultimately fixed with this change: http://trac.webkit.org/changeset/61383 Sorry for keeping this unresolved for so long! Cheers, Rob. |