Bug 106535 - (computedstyle-pseudo) getComputedStyle() doesn't report intermediate values during a transition of a pseudo element
(computedstyle-pseudo)
: getComputedStyle() doesn't report intermediate values during a transition of ...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.7
: P2 Normal
Assigned To: Elliott Sprehn
http://rodneyrehm.de/t/generated-cont...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-10 00:25 PST by Rodney Rehm
Modified: 2013-02-08 00:35 PST (History)
16 users (show)

See Also:


Attachments
Patch (18.26 KB, patch)
2013-01-11 15:01 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (26.93 KB, patch)
2013-01-15 16:42 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (16.58 KB, patch)
2013-02-06 15:27 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (16.82 KB, patch)
2013-02-07 16:41 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Rehm 2013-01-10 00:25:13 PST
The computedStyle() of a property is supposed to change according to the transition.  While computedStyle() of a regular element behaves as expected (returning intermediate values), it fails on pseudo elements.


Steps to Reproduce:

1. define a CSS transition of any property of :before
2. bind a requestAnimationFrame loop to extract the computedStyle of that property on :before and log to console
3. start the transition


Actual Result:

Output looks like "1px, 100px"


Expected Result:

Output looks like "1px, 3px, 10px, …, 100px"
Comment 1 Elliott Sprehn 2013-01-10 02:29:30 PST
This is a really old FIXME:

RenderStyle* Element::computedStyle(PseudoId pseudoElementSpecifier)
{
    // FIXME: Find and use the renderer from the pseudo element instead of the actual element so that the 'length'
    // properties, which are only known by the renderer because it did the layout, will be correct and so that the
    // values returned for the ":selection" pseudo-element will be correct.

We need to get the pseudoElement(BEFORE or AFTER) and use their renderStyle() instead.

We also need to fix CSSComputedStyleDeclaration::getPropertyCSSValue as it seems the logic for pseudo element styles is wrong since we consult the cached pseudo style from the parent element in the composited case.
Comment 2 Elliott Sprehn 2013-01-11 15:01:52 PST
Created attachment 182424 [details]
Patch

Still needs a non-manual test.
Comment 3 Early Warning System Bot 2013-01-11 15:18:14 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15831039
Comment 4 Early Warning System Bot 2013-01-11 15:19:02 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15828040
Comment 5 Build Bot 2013-01-11 15:34:37 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15830064
Comment 6 Elliott Sprehn 2013-01-11 16:15:01 PST
Woops, botched a variable rename right before uploading.
Comment 7 WebKit Review Bot 2013-01-11 16:22:34 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15813351
Comment 8 Build Bot 2013-01-11 16:26:49 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15833063
Comment 9 Peter Beverloo (cr-android ews) 2013-01-11 16:28:09 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15820098
Comment 10 EFL EWS Bot 2013-01-13 20:58:01 PST
Comment on attachment 182424 [details]
Patch

Attachment 182424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15860329
Comment 11 Elliott Sprehn 2013-01-15 16:42:28 PST
Created attachment 182873 [details]
Patch
Comment 12 Ojan Vafai 2013-01-17 17:09:40 PST
Comment on attachment 182873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182873&action=review

Fine at a high-level. Just a few nits.

Mainly R- because I'm hoping we can tighten up the tests.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1509
> +    Node* styledNode = this->styledNode();

Nit: Can we make this styledElement? You can't get computed style on a non-Element anymore with Antti's recent refactorings.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1513
> +    Document* document = styledNode->document();

Do you need to move this? pseudo elements are necessarily in the same document as their node, no?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1550
> +        style = m_node->computedStyle(m_pseudoElementSpecifier);

May as well use styledNode and avoid the need for the comment?

> LayoutTests/fast/css-generated-content/pseudo-animation.html:86
> +        shouldBeCloseTo('div.offsetWidth', 20, 1);

Why shouldBeCloseTo instead of shouldBe? Seems like we should be able to modify the test so that we can get a hard value.
Comment 13 Elliott Sprehn 2013-01-17 18:54:10 PST
(In reply to comment #12)
> (From update of attachment 182873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182873&action=review
> 
> Fine at a high-level. Just a few nits.
> 
> Mainly R- because I'm hoping we can tighten up the tests.
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1509
> > +    Node* styledNode = this->styledNode();
> 
> Nit: Can we make this styledElement? You can't get computed style on a non-Element anymore with Antti's recent refactorings.

Unfortunately EditingStyle creates these for text nodes :( I tried that at first and found out we can't do that without a bunch of refactoring. I think we may still want to do that, but I'd like to do it in a separate step.

> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1513
> > +    Document* document = styledNode->document();
> 
> Do you need to move this? pseudo elements are necessarily in the same document as their node, no?

Yeah, I was just trying to clean up the code.

> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1550
> > +        style = m_node->computedStyle(m_pseudoElementSpecifier);
> 
> May as well use styledNode and avoid the need for the comment?

The problem is that you end up doing essentially beforePseudo->computedStyle(":before"), which I think works because Element::computedStyle falls back to the element's own style if there's no pseudo style available, but seemed a bit weird. I'll change it to be consistent.

> 
> > LayoutTests/fast/css-generated-content/pseudo-animation.html:86
> > +        shouldBeCloseTo('div.offsetWidth', 20, 1);
> 
> Why shouldBeCloseTo instead of shouldBe? Seems like we should be able to modify the test so that we can get a hard value.

I had trouble with subpixel layout where my test fails in ports that don't have it turned on yet in Bug 92591, I can see about trying to pick values that snap but I'm not sure how to do that. Do you have suggestions?
Comment 14 Elliott Sprehn 2013-02-06 15:27:46 PST
Created attachment 186933 [details]
Patch
Comment 15 Elliott Sprehn 2013-02-06 15:29:32 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 182873 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182873&action=review
> > 
> ...
> 
> I had trouble with subpixel layout where my test fails in ports that don't have it turned on yet in Bug 92591, I can see about trying to pick values that snap but I'm not sure how to do that. Do you have suggestions?

I fixed the other comments, but I left the test the way it is for now. I'll fix that in a follow up patch, for now I just added a FIXME.
Comment 16 Ojan Vafai 2013-02-06 16:21:28 PST
Comment on attachment 186933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186933&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1513
> +Node* CSSComputedStyleDeclaration::styledNode() const

Can you add a FIXME to make this styledElement with a brief explanation that we create EditingStyles for TextNodes?
Comment 17 Elliott Sprehn 2013-02-07 16:41:20 PST
(In reply to comment #16)
> (From update of attachment 186933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186933&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1513
> > +Node* CSSComputedStyleDeclaration::styledNode() const
> 
> Can you add a FIXME to make this styledElement with a brief explanation that we create EditingStyles for TextNodes?

Done.
Comment 18 Elliott Sprehn 2013-02-07 16:41:24 PST
Created attachment 187197 [details]
Patch for landing
Comment 19 WebKit Review Bot 2013-02-07 17:42:06 PST
Comment on attachment 187197 [details]
Patch for landing

Clearing flags on attachment: 187197

Committed r142215: <http://trac.webkit.org/changeset/142215>
Comment 20 WebKit Review Bot 2013-02-07 17:42:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Roger Fong 2013-02-07 18:48:30 PST
This broke about 100 tests on Windows port.
http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r142215%20(32210)/results.html

Please fix or roll out
Comment 22 Elliott Sprehn 2013-02-07 19:00:32 PST
(In reply to comment #21)
> This broke about 100 tests on Windows port.
> http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r142215%20(32210)/results.html
> 
> Please fix or roll out

It seems very unlikely it was this patch. All those failures have to do with compositing and not getComputedStyle. It also doesn't make any sense that all the pills would be green and then it would fail. :)
Comment 23 Elliott Sprehn 2013-02-07 19:16:02 PST
(In reply to comment #22)
> (In reply to comment #21)
> > This broke about 100 tests on Windows port.
> > http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r142215%20(32210)/results.html
> > 
> > Please fix or roll out
> 
> It seems very unlikely it was this patch. All those failures have to do with compositing and not getComputedStyle. It also doesn't make any sense that all the pills would be green and then it would fail. :)

Also note that the failures go away in the next run:

http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r142217%20%2832211%29/results.html

http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29?numbuilds=100

This looks like a problem with the Windows bot, there's several other runs in the history that seem to fail hundreds where no patch makes sense and the next run is fine.
Comment 24 Roger Fong 2013-02-08 00:35:37 PST
Ah yes, my b, didn't really look at the commit, just which revision the tests went crazy on.
I'll have to look into that bot hiccup at some point.