Bug 20088 - parse CSS Animations
Summary: parse CSS Animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 19028
  Show dependency treegraph
 
Reported: 2008-07-17 16:24 PDT by Dean Jackson
Modified: 2008-08-05 16:02 PDT (History)
1 user (show)

See Also:


Attachments
patch to provide parsing for css animation properties (61.70 KB, patch)
2008-07-17 16:25 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
updated patch for css parsing (195.50 KB, patch)
2008-07-20 12:27 PDT, Dean Jackson
hyatt: review-
Details | Formatted Diff | Diff
Updated patch that adds keyframes correctly (65.29 KB, patch)
2008-08-04 17:19 PDT, Dean Jackson
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2008-07-17 16:24:20 PDT
This patch updates the CSS parser to accept the CSS Animation properties, including the keyframe syntax.
Comment 1 Dean Jackson 2008-07-17 16:25:30 PDT
Created attachment 22354 [details]
patch to provide parsing for css animation properties
Comment 2 Dean Jackson 2008-07-20 12:27:49 PDT
Created attachment 22395 [details]
updated patch for css parsing

Removed an old keyframes @rule from the grammar. I'd also left out the keyword in the tokenizer so keyframes were not being noticed.
Comment 3 Dave Hyatt 2008-07-23 15:32:09 PDT
Comment on attachment 22395 [details]
updated patch for css parsing

CSSComputedStyleDeclaration should not be part of this patch.

Need a followup bug that keyframes should support a comma-separated list of percentages and not just a single percentage.

The way keyframe rules are added is wrong.  You need to add keyframe styles as part of the normal addRulesFromSheet process.  You can't add them in a separate walk or you'll miss @import and @media behavior.

Make a test with a <style> element that @imports a sheet with keyframes and you'll see that it doesn't work.  Similarly try putting a @keyframes rule inside @media print { } and watch it incorrectly apply to screen.

You already have the hashtable from name -> keyframes rule held by the CSSStyleSelector.  Just build it up during the normal addRulesFromSheet process instead.

The code that aggressively constructs RenderStyles at keyframe-buildup time in the style selector is wrong, but that's the part I'm looking the other way on.

This patch duplicates stuff from 19938.... the Node/Document/Element changes.  Pick one patch to put them in. :)
Comment 4 Dean Jackson 2008-08-04 17:19:37 PDT
Created attachment 22646 [details]
Updated patch that adds keyframes correctly

This (hopefully) addresses the previous review comments about adding Keyframes at the wrong time during parsing. They should now be picked up within @media rules, for example.
Comment 5 Dave Hyatt 2008-08-05 15:01:41 PDT
Comment on attachment 22646 [details]
Updated patch that adds keyframes correctly

This looks good.  

I think we need tests of error recovery from messed up keyframes, since I'm a bit worried that malformed keyframes rules aren't going to error recover properly, but I think that could be a followup bug.
Comment 6 Dean Jackson 2008-08-05 16:02:17 PDT
r35579 = 684ed79bb9f8a782bb5f4d789adef25c161ec0e7 (trunk)
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/css/CSSValueKeywords.in
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSStyleSelector.h
	M	WebCore/css/CSSRule.h
	A	WebCore/css/WebKitCSSKeyframesRule.cpp
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSGrammar.y
	A	WebCore/css/WebKitCSSKeyframeRule.cpp
	M	WebCore/css/tokenizer.flex
	M	WebCore/css/StyleBase.h
	M	WebCore/css/CSSPropertyNames.in
	A	WebCore/css/WebKitCSSKeyframeRule.h
	A	WebCore/css/WebKitCSSKeyframesRule.h
	M	WebCore/css/CSSParser.h
	M	WebCore/WebCoreSources.bkl