Bug 129154

Summary: An element having -webkit-mix-blend mode should only blend with the contents of the parent stacking context
Product: WebKit Reporter: Mihai Tica <mitica>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, esprehn+autocc, glenn, kondapallykalyan, mihnea, simon.fraser, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 95614    
Attachments:
Description Flags
Patch V1
dino: review+
Patch for landing
commit-queue: commit-queue-
Patch for landing none

Description Mihai Tica 2014-02-21 08:32:30 PST
As stated in the Blending and Compositing spec (http://dev.w3.org/fxtf/compositing-1/):
An element that has blending applied, must blend with all the underlying content of the stacking context [CSS21] that that element belongs to.

Currently, the element blends with every content drawn below. This issue implies that blending should be isolated and shouldn't blend with content outside the parent stacking context.
Comment 1 Mihai Tica 2014-02-21 09:11:10 PST
Created attachment 224877 [details]
Patch V1
Comment 2 Dean Jackson 2014-02-22 16:06:03 PST
Comment on attachment 224877 [details]
Patch V1

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

> Source/WebCore/ChangeLog:9
> +        This change isolates blending, preventing it to blend with other underlying elements besides the parent stacking context.

Typo "to" -> "from"

> Source/WebCore/rendering/RenderLayer.cpp:814
> +    if (!oldStyle && renderer().style().hasBlendMode())
> +        m_updateParentStackingContextShouldIsolateBlendingDirty = true;
> +
> +    if (oldStyle && oldStyle->hasBlendMode() != renderer().style().hasBlendMode())
> +        m_updateParentStackingContextShouldIsolateBlendingDirty = true;

Make this one test, even though it will be long.

> Source/WebCore/rendering/RenderLayer.h:785
>  #if ENABLE(CSS_COMPOSITING)
>      bool hasBlendMode() const { return renderer().hasBlendMode(); }
> +    bool isolatesBlending() const { return m_isolatesBlending; }
>  #else
>      bool hasBlendMode() const { return false; }
> +    bool isolatesBlending() const { return false; }
>  #endif

I think it would be better to write these the other way around:

bool isolatesBlending() const
{
#if ENABLE(CSS_COMPOSITING)
  return m_isolatesBlending;
#else
  return false;
#endif
}


Same for hasBlendMode
Comment 3 Mihai Tica 2014-02-24 01:03:15 PST
Created attachment 225034 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2014-02-24 01:15:23 PST
Comment on attachment 225034 [details]
Patch for landing

Rejecting attachment 225034 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 225034, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5120381560029184
Comment 5 Mihai Tica 2014-02-24 01:25:13 PST
Created attachment 225042 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2014-02-24 01:56:49 PST
Comment on attachment 225042 [details]
Patch for landing

Clearing flags on attachment: 225042

Committed r164579: <http://trac.webkit.org/changeset/164579>
Comment 7 Simon Fraser (smfr) 2014-02-24 10:57:04 PST
Comment on attachment 224877 [details]
Patch V1

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

> Source/WebCore/rendering/RenderLayer.cpp:836
> +            ancestor->m_isolatesBlending = renderer().style().hasBlendMode();

Would have been nice to call setIsolatesBlending() here.

What ensures that m_isolatesBlending is updated when layers change between being stacking context and not? I don't see how the code currently can correctly maintain the m_isolatesBlending flag.
Comment 8 Mihai Tica 2014-02-25 01:10:37 PST
(In reply to comment #7)
> (From update of attachment 224877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224877&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:836
> > +            ancestor->m_isolatesBlending = renderer().style().hasBlendMode();
> 
> Would have been nice to call setIsolatesBlending() here.
> 
> What ensures that m_isolatesBlending is updated when layers change between being stacking context and not? I don't see how the code currently can correctly maintain the m_isolatesBlending flag.

You are right, I've mistakenly omitted this case, it should be corrected in a follow up patch.
Comment 9 Mihnea Ovidenie 2014-03-03 23:54:51 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 224877 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=224877&action=review
> > 
> > > Source/WebCore/rendering/RenderLayer.cpp:836
> > > +            ancestor->m_isolatesBlending = renderer().style().hasBlendMode();
> > 
> > Would have been nice to call setIsolatesBlending() here.
> > 
> > What ensures that m_isolatesBlending is updated when layers change between being stacking context and not? I don't see how the code currently can correctly maintain the m_isolatesBlending flag.
> 
> You are right, I've mistakenly omitted this case, it should be corrected in a follow up patch.

Filled follow-up bug: https://bugs.webkit.org/show_bug.cgi?id=129307
Comment 10 Mihnea Ovidenie 2014-03-21 08:02:50 PDT
Close this one as the follow up bug landed.