Bug 59301 – Opacity transition doesn't work while content property is set.

• Component: CSS
• Status: RESOLVED FIXED

The opacity transition doesn't work while content property is set in CSS.

Procedure to Reproduce:
1. open the attachment in safari or chrome.
2. wait and see.

Created attachment 90884 [details]
opacity transition bug

[Simon Fraser (smfr)]

We don't support transitions on generated content, but this should still work.

[Simon Fraser (smfr)]

Weird. RenderStyle::setOpacity() is blowing away the m_content of rareNonInheritedData.

[Simon Fraser (smfr)]

StyleRareNonInheritedData's copy ctor totally fails to set up m_content

[Simon Fraser (smfr)]

Created attachment 96203 [details]
WIP patch

Starter patch, but it would be nice to fix the mess that is ContentData (bug 62185)

[Simon Fraser (smfr)]

Created attachment 96306 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 96312 [details]
Patch

[Darin Adler]

Comment on attachment 96312 [details]
Patch

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

r=me as is, but I see some opportunity to improve

> Source/WebCore/rendering/style/ContentData.cpp:53
> +    switch (type()) {

Since we already have subclasses for each type of ContentData, it would be better style to use virtual function rather than a switch on type() for the type-specific part of this function. That would eliminate all the calls to static_cast.

> Source/WebCore/rendering/style/ContentData.cpp:57
> +        RefPtr<StyleImage> image = const_cast<StyleImage*>(static_cast<const ImageContentData*>(this)->image());

There must be some way to avoid this const_cast. Ugh.

> Source/WebCore/rendering/style/ContentData.cpp:75
> +    if (result && m_next)
> +        result->setNext(m_next->clone());

It should be straightforward to write this in a loop instead of making it recursive. It’s slightly better to not use stack space proportional to the complexity of the content.

> Source/WebCore/rendering/style/CounterDirectives.cpp:45
> +    CounterDirectiveMap::const_iterator end = counterDirectives.end();
> +    for (CounterDirectiveMap::const_iterator it = counterDirectives.begin(); it != end; ++it)
> +        result->add(it->first, it->second);

You should just be able to say *result = counterDirectives here. Did you try that?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:162
> +    if ((!m_counterDirectives && o.m_counterDirectives) || (m_counterDirectives && !o.m_counterDirectives))
> +        return false;
> +    if (m_counterDirectives && o.m_counterDirectives && (*m_counterDirectives != *o.m_counterDirectives))
> +        return false;
> +    return true;
> +

Your version is OK. My version is the same speed or faster and maybe a little more obvious:

    if (m_counterDirectives == o.m_counterDirectives)
        return true;
    if (m_counterDirectives && o.m_counterDirectives && *m_counterDirectives == *o.m_counterDirectives)
        return false;
    return false;

[Simon Fraser (smfr)]

(In reply to comment #8)

> Your version is OK. My version is the same speed or faster and maybe a little more obvious:
> 
>     if (m_counterDirectives == o.m_counterDirectives)
>         return true;

I had to write this as m_counterDirectives.get() == o.m_counterDirectives.get()

>     if (m_counterDirectives && o.m_counterDirectives && *m_counterDirectives == *o.m_counterDirectives)
>         return false;

return true there.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/88308