Bug 92591

Summary: Transitions and animations do not apply to CSS ::before and ::after pseudo-elements
Product: WebKit Reporter: bugmenot
Component: CSSAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: 7raivis, cdumez, dbates, dglazkov, dino, dtrebbien, eric, esprehn, jussi.kukkonen, paulirish, peter, samuel.vogel, simon.fraser, syoichi, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 104462, 106015    
Bug Blocks: 23209    
Attachments:
Description Flags
Testcase of transitioning opacity and transform scale of ::after content url() on mouse hover/active tap
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description bugmenot 2012-07-29 04:13:40 PDT
Created attachment 155165 [details]
Testcase of transitioning opacity and transform scale of ::after content url() on mouse hover/active tap

Bug 23209 has become a meta bug so this one is just for ::before and ::after pseudo-elements, marked as blocking that bug.
Comment 1 Elliott Sprehn 2012-10-04 20:25:00 PDT
This is resolved by the patch on WKBug 95117
Comment 2 Simon Fraser (smfr) 2012-10-05 09:01:48 PDT
If you had written bug 95117 bugzilla would have linkified it.
Comment 3 Elliott Sprehn 2012-12-13 23:09:26 PST
Now that Bug 104462 has been landed we can support these by removing the guard logic in AnimationController.cpp
Comment 4 Elliott Sprehn 2012-12-17 12:46:35 PST
Created attachment 179781 [details]
Patch
Comment 5 WebKit Review Bot 2012-12-17 13:40:13 PST
Comment on attachment 179781 [details]
Patch

Attachment 179781 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15377832

New failing tests:
fast/css-generated-content/pseudo-animation.html
Comment 6 Elliott Sprehn 2012-12-17 13:45:29 PST
(In reply to comment #5)
> (From update of attachment 179781 [details])
> Attachment 179781 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15377832
> 
> New failing tests:
> fast/css-generated-content/pseudo-animation.html

Hmm, that's weird. I wonder why the test would timeout since it does setTimeout with notifyDone() inside it.
Comment 7 Elliott Sprehn 2012-12-17 13:51:45 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 179781 [details] [details])
> > Attachment 179781 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/15377832
> > 
> > New failing tests:
> > fast/css-generated-content/pseudo-animation.html
> 
> Hmm, that's weird. I wonder why the test would timeout since it does setTimeout with notifyDone() inside it.

Err, woops. I broke the test right before I uploaded. Lets try this again...
Comment 8 Elliott Sprehn 2012-12-17 14:08:36 PST
Created attachment 179799 [details]
Patch
Comment 9 Eric Seidel 2012-12-17 14:17:53 PST
Comment on attachment 179799 [details]
Patch

Great!
Comment 10 WebKit Review Bot 2012-12-17 16:13:22 PST
Comment on attachment 179799 [details]
Patch

Attachment 179799 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15361940

New failing tests:
fast/css-generated-content/pseudo-animation.html
Comment 11 Elliott Sprehn 2012-12-17 16:14:25 PST
Created attachment 179824 [details]
Patch
Comment 12 Build Bot 2012-12-17 18:15:12 PST
Comment on attachment 179824 [details]
Patch

Attachment 179824 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15360985

New failing tests:
fast/css-generated-content/pseudo-transition.html
fast/css-generated-content/pseudo-animation.html
Comment 13 Elliott Sprehn 2012-12-17 18:28:44 PST
(In reply to comment #12)
> (From update of attachment 179824 [details])
> Attachment 179824 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15360985
> 
> New failing tests:
> fast/css-generated-content/pseudo-transition.html
> fast/css-generated-content/pseudo-animation.html

Hmm, it seems this approach to testing the feature isn't going to work because it's too racy in DRT. I think we're going to need to expose the PseudoElement's through Internals and write regular animation style tests.
Comment 14 Elliott Sprehn 2012-12-20 18:40:41 PST
Created attachment 180456 [details]
Patch
Comment 15 Elliott Sprehn 2012-12-20 20:13:33 PST
Created attachment 180466 [details]
Patch
Comment 16 Elliott Sprehn 2012-12-20 20:38:47 PST
Created attachment 180471 [details]
Patch
Comment 17 Eric Seidel 2012-12-21 13:35:48 PST
Comment on attachment 180471 [details]
Patch

I'm not sure why an animation and a transition are different, but OK. :)  You might want to ping dino, but LGTM.
Comment 18 Elliott Sprehn 2012-12-21 13:57:33 PST
(In reply to comment #17)
> (From update of attachment 180471 [details])
> I'm not sure why an animation and a transition are different, but OK. :)  You might want to ping dino, but LGTM.

They're effectively the same thing, but internally they are represented as different things: ImplicitAnimation vs KeyframeAnimation. It doesn't seem there's any reason one should be different than the other for pseudo elements, but it's nice to have tests just in case someone refactors the animation code to try walking around in the DOM or something in the future where PseudoElement's missing nextSibling and previousSibling might matter.
Comment 19 Elliott Sprehn 2013-01-02 11:54:53 PST
Created attachment 181041 [details]
Patch
Comment 20 WebKit Review Bot 2013-01-02 12:33:02 PST
Comment on attachment 181041 [details]
Patch

Clearing flags on attachment: 181041

Committed r138632: <http://trac.webkit.org/changeset/138632>
Comment 21 WebKit Review Bot 2013-01-02 12:33:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Jussi Kukkonen (jku) 2013-01-03 04:47:08 PST
The tests are failing on EFL, Qt and GTK.

At least on EFL it's because the mid-way (1.0 sec) value is just slightly off: offsetWidth jumps from 20px to 19px at t=0.996 already. This is 100% reproducable.

So the questions are:
 * is the current test correct? I don't have a bezier calculator at hand to
   check what the ease value should be at t=1.0, nor do I know the exact rules of
   rounding...
 * are these just rounding issues we shouldn't care about _in this test_? Can we
   pick a time value that gives the same result for everyone, or use platform
   expected results?

For reference, webkit-efl thinks 20px is the correct value for t=[0.948-0.995]
Comment 23 Simon Fraser (smfr) 2013-01-03 10:10:04 PST
Are the tests not using the pause API that was added?
Comment 24 Elliott Sprehn 2013-01-03 10:12:38 PST
(In reply to comment #23)
> Are the tests not using the pause API that was added?

They are, but I check the value for exact equality. On closer inspection of the other animation tests they all use a tolerance. See runAnimationTest or runTransitionTest usage. I think we just need to change the tests to use shouldBeCloseTo and provide a tolerance of 1.
Comment 25 Simon Fraser (smfr) 2013-01-03 10:26:40 PST
Hm, with the pause API there should be no need for fuzzy equality checking.
Comment 26 Jussi Kukkonen (jku) 2013-01-07 02:13:38 PST
(In reply to comment #23)
> Are the tests not using the pause API that was added?

Just to make it clear: the results are not flaky. I get exactly the same px value  when testing multiple times with the same time value (even if the results is right at the edge of almost getting rounded to the next pixel).
Comment 27 Elliott Sprehn 2013-01-08 14:41:25 PST
(In reply to comment #26)
> (In reply to comment #23)
> > Are the tests not using the pause API that was added?
> 
> Just to make it clear: the results are not flaky. I get exactly the same px value  when testing multiple times with the same time value (even if the results is right at the edge of almost getting rounded to the next pixel).

Yeah, this seems like it's subpixel. Perhaps someone who can build EFL/GTK/QT can pick different stop points and change the test so it doesn't land on a subpixel boundary?