Bug 88786

Summary: TextureMapperLayer: invisible layers do not let their children to be painted
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, mrobinson, noam, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch noam: review+

Description Sergio Villar Senin 2012-06-11 09:27:12 PDT
[GTK] TextureMapperLayer: invisible layers do not let their children to be painted
Comment 1 Sergio Villar Senin 2012-06-11 09:29:46 PDT
Created attachment 146866 [details]
Patch
Comment 2 Sergio Villar Senin 2012-06-11 09:32:21 PDT
In the particular case of the bug mentioned in the ChangeLog there are 3 boxes, the first one (the bigger) has the second inside, and this one has another one inside (the smaller). The second box is rotated 180º and it becomes invisible because the backface is marked as invisible. Then the third box is rotated another 180º so it becomes visible but it was not painted because its father was declared as invisible and thus it was not rendering their children.
Comment 3 Martin Robinson 2012-06-11 09:46:49 PDT
A link to a test case would be pretty useful here, I think.
Comment 4 Noam Rosenthal 2012-06-11 10:10:08 PDT
Comment on attachment 146866 [details]
Patch

I understand the bug, but the fix is not right. We still want to exit early if the opacity is 0, the size is empty etc. We should add specific tests of culling, and only paint the children if we're preserving 3D.
Comment 5 Sergio Villar Senin 2012-06-11 10:11:17 PDT
(In reply to comment #3)
> A link to a test case would be pretty useful here, I think.

This is the one I used for testing:

http://svn.webkit.org/repository/webkit/trunk/LayoutTests/compositing/backface-visibility/backface-visibility-3d.html
Comment 6 Sergio Villar Senin 2012-06-13 01:14:18 PDT
Created attachment 147258 [details]
Patch
Comment 7 Noam Rosenthal 2012-06-13 06:09:34 PDT
Comment on attachment 147258 [details]
Patch

This is not the right logic. It should be:
- opacity 0, empty size, or (backfacing and backface-visibility is false and we're not preserving 3D)? return early at paintRecursive
- (backfacing and backface-visibility is false) ? return early and paintSelf
Comment 8 Sergio Villar Senin 2012-06-13 09:38:09 PDT
(In reply to comment #7)
> (From update of attachment 147258 [details])
> This is not the right logic. It should be:
> - opacity 0, empty size, or (backfacing and backface-visibility is false and we're not preserving 3D)? return early at paintRecursive



> - (backfacing and backface-visibility is false) ? return early and paintSelf

(In reply to comment #7)
> (From update of attachment 147258 [details])
> This is not the right logic. It should be:
> - opacity 0, empty size, or (backfacing and backface-visibility is false and we're not preserving 3D)? return early at paintRecursive

That preserves3D is interesting because I don't understand the expected results of the third test case of http://svn.webkit.org/repository/webkit/trunk/LayoutTests/compositing/backface-visibility/backface-visibility-non3d.html. Shouldn't the lime div be not visible?
Comment 9 Sergio Villar Senin 2012-06-14 10:23:01 PDT
(In reply to comment #8)
> (In reply to comment #7)

> > (From update of attachment 147258 [details] [details])
> > This is not the right logic. It should be:
> > - opacity 0, empty size, or (backfacing and backface-visibility is false and we're not preserving 3D)? return early at paintRecursive
> 
> That preserves3D is interesting because I don't understand the expected results of the third test case of http://svn.webkit.org/repository/webkit/trunk/LayoutTests/compositing/backface-visibility/backface-visibility-non3d.html. Shouldn't the lime div be not visible?

Well that's is really not that important now so ignore it.

So please correct me if I'm wrong, but this is how I understand the issue:
- we cannot skip a whole hierarchy in paintRecursive() just because we're backfacing and not preserving 3D because even in that case we could have a visible child (including childs backfacing with backface-visibility: visible). See for example test #3 in LayoutTests/compositing/backface-visibility/backface-visibility-non3d.html. Not preserving 3d as I understand it, does not invalidate the CSS painting order.
- in paintSelf() we should basically early return if (!m_state.visible) (which already takes into account the backface and its visiblity)

Following this rationale I cooked a 3rd patch that makes both LayoutTests/compositing/backface-visibility/backface-visibility-non3d.html and LayoutTests/compositing/backface-visibility/backface-visibility-3d.html work fine. But before uploading it I'd like to know your opinion about what I said above.
Comment 10 Sergio Villar Senin 2012-06-15 00:03:31 PDT
Created attachment 147748 [details]
Patch

Noam, I was talking about something like this
Comment 11 Sergio Villar Senin 2012-06-18 01:13:08 PDT
Committed r120577: <http://trac.webkit.org/changeset/120577>