[webkit-reviews] review denied: [Bug 24021] pseudo-element styles not accessible / retrievable via DOM methods : [Attachment 45754] A tentative idea for a fix that will expose the standard pseudo-elements through getComputedStyle

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Jessie Berlin <jberlin at webkit.org>'s request for
review:
Bug 24021: pseudo-element styles not accessible / retrievable via DOM methods
https://bugs.webkit.org/show_bug.cgi?id=24021

Attachment 45754: A tentative idea for a fix that will expose the standard
pseudo-elements through getComputedStyle
https://bugs.webkit.org/attachment.cgi?id=45754&action=review

------- Additional Comments from mitz at webkit.org
>      if (renderer && hasCompositedLayer(renderer) &&
AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSProp
ertyID>(propertyID)))
>	   style =
renderer->animation()->getAnimatedStyleForRenderer(renderer);
> -    else
> -	  style = node->computedStyle();
> +    else {
> +	   style = node->computedStyle();
> +	   if (m_pseudoStyle != NOPSEUDO)
> +	       style = node->renderer()->getCachedPseudoStyle(m_pseudoStyle);

Note that this code accessing node->renderer() is in the else clause of “if
(renderer && …”. One common way that the renderer can be null is if the node
has (or descends from an element that has) 'display: none;'. It looks like in
the pseudo-element case, you are calling Node::computedStyle() only to throw
away the result. Is there a side effect to calling computedStyle() that you are
trying to achieve?

For several properties, the code that follows in this method gets the computed
value from the renderer, if there is one (e.g. CSSPropertyHeight). In order for
this code to return the correct result, you should set 'renderer' to the
renderer of the pseudo-element in question. For example, consider

<style>
    div { height: 100px; }
    div:before { display: block; height: 20px; content: "Before"; }
</style>
<div id="target"></div>

You want getComputedStyle(document.getElementById("target"), ":before").height
to be "20px", not "100px".

> +    DEFINE_STATIC_LOCAL(AtomicString, after, (":after"));
…
> +    DEFINE_STATIC_LOCAL(AtomicString, sliderThumb,
(":-webkit-slider-thumb"));
> +
> +    if (value == after)
> +	   return AFTER;
…
> +    if (value == sliderThumb)
> +	   return SLIDER_THUMB;

Maybe there is a way to leverage existing code to do this, but if there isn’t,
perhaps a HashMap<AtomicStringImpl*, PseudoId> would be better?