Bug 131153

Summary: [CSS Blending] Add compositing reason for isolation in RenderLayerCompositor.
Product: WebKit Reporter: Ion Rosca <rosca>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, graouts, joepeck, kondapallykalyan, krit, mitica, simon.fraser, timothy, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 95614    
Attachments:
Description Flags
adding compositing reason for isolation
none
nit
none
adding inspector code & test
none
fixing user facing string & test error check none

Ion Rosca
Reported 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.
Attachments
adding compositing reason for isolation (3.50 KB, patch)
2014-04-02 23:48 PDT, Ion Rosca 		no flags
DetailsFormatted DiffDiff
nit (3.51 KB, patch)
2014-04-03 00:03 PDT, Ion Rosca 		no flags
DetailsFormatted DiffDiff
adding inspector code & test (14.75 KB, patch)
2014-04-03 12:50 PDT, Ion Rosca 		no flags
DetailsFormatted DiffDiff
fixing user facing string & test error check (14.56 KB, patch)
2014-04-04 06:50 PDT, Ion Rosca 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Ion Rosca
Comment 1 2014-04-02 23:48:13 PDT
Created attachment 228473 [details] adding compositing reason for isolation
Ion Rosca
Comment 2 2014-04-03 00:03:38 PDT
Created attachment 228475 [details] nit
Dirk Schulze
Comment 3 2014-04-03 04:17:44 PDT
Is there no way to test it? Does DRT expose these information?
Mihai Tica
Comment 4 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
Ion Rosca
Comment 5 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)
Dirk Schulze
Comment 6 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.
Ion Rosca
Comment 7 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.
Ion Rosca
Comment 8 2014-04-03 12:50:18 PDT
Created attachment 228532 [details] adding inspector code & test
Dean Jackson
Comment 9 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.
Joseph Pecoraro
Comment 10 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.
Ion Rosca
Comment 11 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"?.
Joseph Pecoraro
Comment 12 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
Ion Rosca
Comment 13 2014-04-04 06:50:50 PDT
Created attachment 228589 [details] fixing user facing string & test error check
Joseph Pecoraro
Comment 14 2014-04-04 10:51:54 PDT
Comment on attachment 228589 [details] fixing user facing string & test error check r=me
WebKit Commit Bot
Comment 15 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>
WebKit Commit Bot
Comment 16 2014-04-04 13:34:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.