Bug 36282 – CSS3 "Property is declared twice in rule" test fails

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

This test fails:
http://samples.msdn.microsoft.com/ietestcenter/domstyle/duplicateRule.htm

[Rob Buis]

Created attachment 57994 [details]
Patch

[WebKit Review Bot]

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.

[Rob Buis]

(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.

[Rob Buis]

Created attachment 59022 [details]
Patch

[Rob Buis]

(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.

[Dave Hyatt]

Comment on attachment 59022 [details]
Patch

This looks good.  You did the right thing letting variable-dependent values have duplicates too. :)

[James Robinson]

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.

[Dimitri Glazkov (Google)]

Reverted r61340 for reason:

Broke several editing tests.

Committed r61345: <http://trac.webkit.org/changeset/61345>

[James Robinson]

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).

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Rob Buis]

This was ultimately fixed with this change:

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

Sorry for keeping this unresolved for so long!
Cheers,

Rob.