Bug 8170 - SVG CSS property values with extra items do not get treated as invalid (they should)
Summary: SVG CSS property values with extra items do not get treated as invalid (they ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: NeedsReduction
Depends on:
Blocks:
 
Reported: 2006-04-03 22:29 PDT by Alexey Proskuryakov
Modified: 2006-04-23 15:53 PDT (History)
1 user (show)

See Also:


Attachments
Proposed fix (1.97 KB, patch)
2006-04-20 06:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Better yet (2.26 KB, patch)
2006-04-20 13:02 PDT, Rob Buis
hyatt: review-
Details | Formatted Diff | Diff
Complete patch (6.68 KB, patch)
2006-04-21 12:47 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 Alexey Proskuryakov 2006-04-03 22:29:00 PDT
SVG CSS properties that have too many elements aren't invalidated, like it used to be the case with HTML properties, see bug 7118. 

The fix should be very similar, but there is no test case to verify it yet. Actually, SVGCSSParser already does the invalidation when handling the valid_primitive case, but not in some other cases.
Comment 1 Rob Buis 2006-04-20 06:54:13 PDT
Created attachment 7849 [details]
Proposed fix

This is my first attempt. From the few small tests I did it handles the parsedValue
cases ok (for stroke and clip-path). In khtml/css/cssparser.cpp the code checks
shorthands, but for svg there only seems to be the marker property, which gets
handled in parseShorthand.
Cheers,

Rob.
Comment 2 Rob Buis 2006-04-20 13:02:14 PDT
Created attachment 7854 [details]
Better yet

After feedback from ap, this one is much better.
It removes my faulty cleanup. Also it now handles initial and inherit
cases, and shorthands (marker) works too. IMHO all that is lacking
is a testcase, which I am working on, but will take some time.
Cheers,

Rob.
Comment 3 Eric Seidel (no email) 2006-04-20 15:10:22 PDT
Comment on attachment 7854 [details]
Better yet

Hyatt should review this.
Comment 4 Dave Hyatt 2006-04-20 18:10:35 PDT
Comment on attachment 7854 [details]
Better yet

You're missing the inShorthand check when you check to see if value->current() is null.  If you're inside an SVG shorthand there could still be properties that will be parsed afterwards.
Comment 5 Rob Buis 2006-04-21 11:59:08 PDT
(In reply to comment #4)
> (From update of attachment 7854 [details] [edit])
> You're missing the inShorthand check when you check to see if value->current()
> is null.  If you're inside an SVG shorthand there could still be properties
> that will be parsed afterwards.
> 

Hi,

I think there is such a call in the last patch.
If you still think it lacks, please let me know, I
am working on wrapping this patch up including
testcase :)
Cheers,

Rob.
Comment 6 Rob Buis 2006-04-21 12:47:09 PDT
Created attachment 7883 [details]
Complete patch

This time the patch + testcase.
Cheers,

Rob.
Comment 7 Dave Hyatt 2006-04-21 22:02:20 PDT
Comment on attachment 7883 [details]
Complete patch

r=me
Comment 8 Eric Seidel (no email) 2006-04-23 15:53:42 PDT
Thanks rob!