Bug 131153 - [CSS Blending] Add compositing reason for isolation in RenderLayerCompositor.
Summary: [CSS Blending] Add compositing reason for isolation in RenderLayerCompositor.
Status: RESOLVED FIXED
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: 95614
  Show dependency treegraph
 
Reported: 2014-04-02 23:35 PDT by Ion Rosca
Modified: 2014-04-04 13:34 PDT (History)
11 users (show)

See Also:


Attachments
adding compositing reason for isolation (3.50 KB, patch)
2014-04-02 23:48 PDT, Ion Rosca
no flags Details | Formatted Diff | Diff
nit (3.51 KB, patch)
2014-04-03 00:03 PDT, Ion Rosca
no flags Details | Formatted Diff | Diff
adding inspector code & test (14.75 KB, patch)
2014-04-03 12:50 PDT, Ion Rosca
no flags Details | Formatted Diff | Diff
fixing user facing string & test error check (14.56 KB, patch)
2014-04-04 06:50 PDT, Ion Rosca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ion Rosca 2014-04-02 23:35:57 PDT
There are 2 reasons for a layer to be composited involving blend modes:
1) When a layer has blend mode and has composited descendants, it should be composited as well in order to apply blending to the whole group at once. CompositingReasonBlendingWithCompositedDescendants is the existing reason for this case.
2) When a layer has to isolate composited blending descendants (it is the enclosing stacking context for those blending descendants), that layer should be composited as well. This will restrict blending layers to use the backdrop information only from the current stacking context. This blending reason should be added.

I will add in a follow up patch the inspector side of this issue.
Comment 1 Ion Rosca 2014-04-02 23:48:13 PDT
Created attachment 228473 [details]
adding compositing reason for isolation
Comment 2 Ion Rosca 2014-04-03 00:03:38 PDT
Created attachment 228475 [details]
nit
Comment 3 Dirk Schulze 2014-04-03 04:17:44 PDT
Is there no way to test it? Does DRT expose these information?
Comment 4 Mihai Tica 2014-04-03 04:42:59 PDT
(In reply to comment #3)
> Is there no way to test it? Does DRT expose these information?

You should be able to dump the layer hierarchy into a text file.
https://bugs.webkit.org/attachment.cgi?id=161391&action=prettypatch is an example
Comment 5 Ion Rosca 2014-04-03 05:07:34 PDT
(In reply to comment #3)
> Is there no way to test it? Does DRT expose these information?

No, DRT does not expose this information. In WebCore reasons are used only for logging. This is the patch where other reasons were added: https://bugs.webkit.org/show_bug.cgi?id=110505

I'm going to add another patch with the implementation in WebInspector and I will use the WebInspector protocol to test this flag (like in this bug: https://bugs.webkit.org/show_bug.cgi?id=111703)
Comment 6 Dirk Schulze 2014-04-03 06:34:17 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > Is there no way to test it? Does DRT expose these information?
> 
> No, DRT does not expose this information. In WebCore reasons are used only for logging. This is the patch where other reasons were added: https://bugs.webkit.org/show_bug.cgi?id=110505
> 
> I'm going to add another patch with the implementation in WebInspector and I will use the WebInspector protocol to test this flag (like in this bug: https://bugs.webkit.org/show_bug.cgi?id=111703)

How much bigger is the inspector patch? Maybe you can combine them or the reviewer  can review them together.
Comment 7 Ion Rosca 2014-04-03 06:50:22 PDT
It's not big. I was thinking that there might be different people to review them. I can combine them.
Comment 8 Ion Rosca 2014-04-03 12:50:18 PDT
Created attachment 228532 [details]
adding inspector code & test
Comment 9 Dean Jackson 2014-04-03 14:04:10 PDT
Comment on attachment 228532 [details]
adding inspector code & test

This looks good to me, but I'd like to see an Inspector person review it too.
Comment 10 Joseph Pecoraro 2014-04-03 14:15:42 PDT
Comment on attachment 228532 [details]
adding inspector code & test

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

r=me

> Source/WebInspectorUI/UserInterface/Views/LayerTreeSidebarPanel.js:436
> +        if (compositingReasons.isolatesCompositedBlendingDescendants)
> +            addReason(WebInspector.UIString("Element isolates composited descendants having CSS blending applied"));

Very interesting! The patch looks good, As a user facing feature, I'd want to get a little more information on what isolation is so that maybe we can improve the UI String. Right now it sounds fine.

Is "-webkit-isolation" with children having blending the only way to get this state? Is just -webkit-isolation enough?

Note some of the other reasons include the CSS property, so it might be useful here to include "-webkit-isolation".

> LayoutTests/inspector-protocol/layers/layers-blending-compositing-reasons.html:114
> +            if (messageObject.hasOwnProperty("error")) {

Nit: This can be shorted to:

    InspectorTest.checkForError(messageObject);

Which will print a fail message, complete the test, and short circuit by throwing an exception.
Comment 11 Ion Rosca 2014-04-03 14:46:04 PDT
Comment on attachment 228532 [details]
adding inspector code & test

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

>> Source/WebInspectorUI/UserInterface/Views/LayerTreeSidebarPanel.js:436
>> +            addReason(WebInspector.UIString("Element isolates composited descendants having CSS blending applied"));
> 
> Very interesting! The patch looks good, As a user facing feature, I'd want to get a little more information on what isolation is so that maybe we can improve the UI String. Right now it sounds fine.
> 
> Is "-webkit-isolation" with children having blending the only way to get this state? Is just -webkit-isolation enough?
> 
> Note some of the other reasons include the CSS property, so it might be useful here to include "-webkit-isolation".

> I'd want to get a little more information on what isolation
We say that a layer isolates, or create an isolated group as defined here: http://dev.w3.org/fxtf/compositing-1/#isolatedgroups, if descendant layers are composited over a fully transparent black backdrop, so that the layers with CSS blending will be able to access only the elements from inside the isolated group.

> Is "-webkit-isolation" with children having blending the only way to get this state? Is just -webkit-isolation enough?
A layer isolates if it is a stacking context and has blending descendants: http://dev.w3.org/fxtf/compositing-1/#csscompositingrules_CSS . -webkit-isolation just turns the element into a stacking context: http://dev.w3.org/fxtf/compositing-1/#isolation, so a positioned layer with z-index would have the same effect.

I think we should not say "-webkit-isolation", maybe "Element is a stacking context and has composited descendants with CSS blending applied"?.
Comment 12 Joseph Pecoraro 2014-04-03 15:14:22 PDT
(In reply to comment #11)
> (From update of attachment 228532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228532&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/LayerTreeSidebarPanel.js:436
> >> +            addReason(WebInspector.UIString("Element isolates composited descendants having CSS blending applied"));
> > 
> > Very interesting! The patch looks good, As a user facing feature, I'd want to get a little more information on what isolation is so that maybe we can improve the UI String. Right now it sounds fine.
> > 
> > Is "-webkit-isolation" with children having blending the only way to get this state? Is just -webkit-isolation enough?
> > 
> > Note some of the other reasons include the CSS property, so it might be useful here to include "-webkit-isolation".
> 
> > I'd want to get a little more information on what isolation
> We say that a layer isolates, or create an isolated group as defined here: http://dev.w3.org/fxtf/compositing-1/#isolatedgroups, if descendant layers are composited over a fully transparent black backdrop, so that the layers with CSS blending will be able to access only the elements from inside the isolated group.
> 
> > Is "-webkit-isolation" with children having blending the only way to get this state? Is just -webkit-isolation enough?
> A layer isolates if it is a stacking context and has blending descendants: http://dev.w3.org/fxtf/compositing-1/#csscompositingrules_CSS . -webkit-isolation just turns the element into a stacking context: http://dev.w3.org/fxtf/compositing-1/#isolation, so a positioned layer with z-index would have the same effect.

Excellent, thanks for the links.

> I think we should not say "-webkit-isolation", maybe "Element is a stacking context and has composited descendants with CSS blending applied"?.

I'm fine with both "Element isolates" or "Element is a stacking context". The 2nd will be more familiar to web developers, and if that really is all isolation is in this context, than it might be the more user friendly string. It seems that "isolates" is a more technical term. For example, I had no idea what it meant =).

Thanks
Comment 13 Ion Rosca 2014-04-04 06:50:50 PDT
Created attachment 228589 [details]
fixing user facing string & test error check
Comment 14 Joseph Pecoraro 2014-04-04 10:51:54 PDT
Comment on attachment 228589 [details]
fixing user facing string & test error check

r=me
Comment 15 WebKit Commit Bot 2014-04-04 13:34:49 PDT
Comment on attachment 228589 [details]
fixing user facing string & test error check

Clearing flags on attachment: 228589

Committed r166800: <http://trac.webkit.org/changeset/166800>
Comment 16 WebKit Commit Bot 2014-04-04 13:34:55 PDT
All reviewed patches have been landed.  Closing bug.