Bug 20995 - Rewrite keyframe resolution to be like styleForElement()
Summary: Rewrite keyframe resolution to be like styleForElement()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Chris Marrin
URL:
Keywords:
: 20148 (view as bug list)
Depends on:
Blocks: 21043
  Show dependency tree / graph
 
Reported: 2008-09-22 09:45 PDT by Chris Marrin
Modified: 2008-10-09 16:27 PDT (History)
3 users (show)

See Also:

Attachments
Patch, including LayoutTest file (36.31 KB, patch)
2008-09-25 14:15 PDT, Chris Marrin 		hyatt: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2008-09-22 09:45:31 PDT 
Today, I resolve style when the style sheet is read. This has several problems, most notably that it can't resolve physical units, like ems. So it needs to change.

Hyatt suggests:

1) In addKeyframeStyle, just store the CSSStyleDeclarations for each keyframe
2) Create a new method (keyframeStyleForElement?) which resolves keyframes given an element and animation name
3) When starting an animation, use this method to get the list of RenderStyles for each keyframe, caching them for the duration of the animation, and using them as today to run the animation.
Comment 1 Chris Marrin 2008-09-22 09:47:53 PDT 
That's actually keyframeStylesForElement() since it collects style for all keyframes at once
Comment 2 mitz 2008-09-24 13:08:33 PDT 
Raising to P1 because of dependent bug 21043.
Comment 3 Chris Marrin 2008-09-25 14:15:57 PDT 
Created attachment 23821 [details]
Patch, including LayoutTest file
Comment 4 Chris Marrin 2008-09-25 14:22:25 PDT 
The fix is as outlines above. I ended up duplicating the element's RenderStyle into each keyframe RenderStyle, so they had full information for doing things like lineheight and other font metrics (and possibly others). This makes the RenderStyles a bit more heavyweight in some cases, but I think the model is right.

The submitted testcase tests the line-height property, which was borked before this change.
Comment 5 Dave Hyatt 2008-09-29 12:38:56 PDT 
Comment on attachment 23821 [details]
Patch, including LayoutTest file

r=me
Comment 6 Simon Fraser (smfr) 2008-09-29 14:31:37 PDT 
Committed r37077
	M	WebCore/rendering/style/KeyframeList.cpp
	M	WebCore/rendering/style/StyleRareNonInheritedData.h
	M	WebCore/rendering/style/StyleRareNonInheritedData.cpp
	M	WebCore/rendering/style/Animation.h
	M	WebCore/rendering/style/Animation.cpp
	M	WebCore/rendering/style/KeyframeList.h
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSStyleSelector.h
	M	WebCore/page/animation/CompositeAnimation.cpp
	M	WebCore/page/animation/KeyframeAnimation.cpp
	M	WebCore/page/animation/KeyframeAnimation.h
	M	LayoutTests/ChangeLog
	A	LayoutTests/animations/lineheight-animation.html
	A	LayoutTests/animations/lineheight-animation-expected.txt
r37077 = e979de02d997b41646bbf7c704ca6bc0c6c3bfdb (trunk)
Comment 7 Chris Marrin 2008-10-09 16:27:36 PDT 
*** Bug 20148 has been marked as a duplicate of this bug. ***