Bug 129480 - Child elements aren't fully repainted when setting opacity from script
Summary: Child elements aren't fully repainted when setting opacity from script
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-28 05:35 PST by Mihai Tica
Modified: 2016-01-02 09:26 PST (History)
16 users (show)

See Also:


Attachments
Sample reproducing the issue (665 bytes, text/html)
2014-02-28 05:36 PST, Mihai Tica
no flags Details
Patch (10.75 KB, patch)
2014-03-28 06:36 PDT, Enrique Ocaña
mcatanzaro: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (456.54 KB, application/zip)
2014-03-28 08:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (487.48 KB, application/zip)
2014-03-28 10:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (548.69 KB, application/zip)
2014-03-28 10:53 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Tica 2014-02-28 05:35:42 PST
Consider the following document:

<div id="parent">
   <div id="child" class="overflowsParent createsStackingContext">
   </div>
</div>

Setting the parent's opacity from script doesn't trigger a full repaint of the child. Instead, only the fragments that intersect the parent are painted.

See attached test case. After the opacity is changed (about 1s), observe the rendering defect. Resizing the window to see the desired result.

This is probably caused by the repaintRect not including the children elements. When detecting such a change, we should hook into RenderLayer::computeRepaintRectsIncludingDescendants.
Note that this problem is only present when the child creates a stacking context.

This problem should be fixed for all properties that cause transparency: opacity, isolation, blend modes. In other words, we should repaint the descendants when detecting a change in the layer transparency.
Comment 1 Mihai Tica 2014-02-28 05:36:19 PST
Created attachment 225454 [details]
Sample reproducing the issue
Comment 2 Enrique Ocaña 2014-03-28 06:36:43 PDT
Created attachment 228046 [details]
Patch
Comment 3 Build Bot 2014-03-28 08:33:13 PDT
Comment on attachment 228046 [details]
Patch

Attachment 228046 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5131992635015168

New failing tests:
fast/repaint/blend-mode-isolate-stacking-context.html
Comment 4 Build Bot 2014-03-28 08:33:19 PDT
Created attachment 228052 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-03-28 10:06:56 PDT
Comment on attachment 228046 [details]
Patch

Attachment 228046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5405030148472832

New failing tests:
fast/repaint/blend-mode-isolate-stacking-context.html
Comment 6 Build Bot 2014-03-28 10:07:05 PDT
Created attachment 228059 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Simon Fraser (smfr) 2014-03-28 10:20:53 PDT
Comment on attachment 228046 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1143
> +        style.setEffectiveOpacity(style.opacity() * parentStyle.opacity());

Is "parentStyle" the correct one to be getting opacity from in all cases?
Comment 8 Build Bot 2014-03-28 10:53:12 PDT
Comment on attachment 228046 [details]
Patch

Attachment 228046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4701668587339776

New failing tests:
fast/repaint/blend-mode-isolate-stacking-context.html
Comment 9 Build Bot 2014-03-28 10:53:19 PDT
Created attachment 228069 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Enrique Ocaña 2014-03-28 14:07:12 PDT
(In reply to comment #7)

> (From update of attachment 228046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228046&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1143
> > +        style.setEffectiveOpacity(style.opacity() * parentStyle.opacity());
> 
> Is "parentStyle" the correct one to be getting opacity from in all cases?

Thanks for pointing this out, Simon. It shouldn't be parentStyle.opacity(), but parentStyle.effectiveOpacity(), which is the field that "accumulates" opacity and represents the combined opacity() of all the ancestors.
Comment 11 Simon Fraser (smfr) 2014-03-28 14:23:45 PDT
(In reply to comment #10)
> (In reply to comment #7)
> 
> > (From update of attachment 228046 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=228046&action=review
> > 
> > > Source/WebCore/css/StyleResolver.cpp:1143
> > > +        style.setEffectiveOpacity(style.opacity() * parentStyle.opacity());
> > 
> > Is "parentStyle" the correct one to be getting opacity from in all cases?
> 
> Thanks for pointing this out, Simon. It shouldn't be parentStyle.opacity(), but parentStyle.effectiveOpacity(), which is the field that "accumulates" opacity and represents the combined opacity() of all the ancestors.

Not just that, but I'm not convinced that the parent element is always the one you multiply style from (assuming that parentStyle is really the parent element's style). E.g. in some configurations of positioning and opacity.
Comment 12 Michael Catanzaro 2016-01-02 09:26:16 PST
Comment on attachment 228046 [details]
Patch

Seems from the comments that Simon has found a bug in this patch, so I'm removing this from request queue.