Bug 36282 - CSS3 "Property is declared twice in rule" test fails
Summary: CSS3 "Property is declared twice in rule" test fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://samples.msdn.microsoft.com/iet...
Keywords:
Depends on:
Blocks: 53470
  Show dependency treegraph
 
Reported: 2010-03-17 20:33 PDT by Simon Fraser (smfr)
Modified: 2011-01-31 19:45 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.03 KB, patch)
2010-06-06 23:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2010-06-17 11:31 PDT, Rob Buis
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-03-17 20:33:17 PDT
This test fails:
http://samples.msdn.microsoft.com/ietestcenter/domstyle/duplicateRule.htm
Comment 1 Rob Buis 2010-06-06 23:36:21 PDT
Created attachment 57994 [details]
Patch
Comment 2 WebKit Review Bot 2010-06-06 23:37:44 PDT
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.
Comment 3 Rob Buis 2010-06-06 23:43:16 PDT
(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.
Comment 4 Rob Buis 2010-06-17 11:31:52 PDT
Created attachment 59022 [details]
Patch
Comment 5 Rob Buis 2010-06-17 11:40:17 PDT
(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 6 Dave Hyatt 2010-06-17 12:02:32 PDT
Comment on attachment 59022 [details]
Patch

This looks good.  You did the right thing letting variable-dependent values have duplicates too. :)
Comment 7 James Robinson 2010-06-17 13:17:52 PDT
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.
Comment 8 Dimitri Glazkov (Google) 2010-06-17 13:28:11 PDT
Reverted r61340 for reason:

Broke several editing tests.

Committed r61345: <http://trac.webkit.org/changeset/61345>
Comment 9 James Robinson 2010-06-17 14:03:14 PDT
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).
Comment 10 WebKit Review Bot 2010-06-17 14:13:07 PDT
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
Comment 11 WebKit Review Bot 2010-06-17 14:13:14 PDT
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
Comment 12 Rob Buis 2010-08-23 01:31:52 PDT
This was ultimately fixed with this change:

http://trac.webkit.org/changeset/61383

Sorry for keeping this unresolved for so long!
Cheers,

Rob.