Paint local attachment backgrounds into composited scrolling contents layer

third_party/WebKit/Source/core/layout/LayoutBox.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 37bc6617ca0184dea77755ea2bbc5af1699b06f7..af3a2979283bf32270c390a39f620fa1ab44d673 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -704,15 +704,36 @@ FloatQuad LayoutBox::absoluteContentQuad() const
     return localToAbsoluteQuad(FloatRect(rect));
 }
 
-LayoutRect LayoutBox::backgroundClipRect() const
-{
-    // TODO(flackr): Check for the maximum background clip rect.
-    switch (style()->backgroundClip()) {
+LayoutRect LayoutBox::backgroundRect(BackgroundRectOpacity rectOpacity) const

[trchen, 2016/08/13 00:14:08]

I'm worried about the semantic of this function being vague.

Does it return a cull rect so it is guaranteed nothing outside of this rect will
paint (i.e. allow false positive, no false negative)? However in the case of
OpaqueBackgroundRect it reports a rect that's absolutely opaque, but there can
still be opaque pixels outside of the rect (i.e. allow false negative, no false
positive).

[flackr, 2016/08/16 17:52:51]

When called with OpaqueBackgroundRect from backgroundIsOpaqueInRect we are okay
with false negatives, as this will not result in visual artifacts. If we have
false positives though it would result in creating an opaque layer and seeing
unpainted parts of the picture.

When called with TranlucentBackgroundRect from compositedLayerMapping we want
the cull / clip rect such that nothing outside this rect will paint.

Perhaps I should rephrase the values in the enum to reflect this, what do you
think of enum BackgroundRectType { BackgroundClipRect, BackgroundKnownOpaqueRect
};?

[trchen, 2016/08/17 01:22:55]

Great idea. It totally sounds clearer!

+{
+    EFillBox backgroundBox = TextFillBox;
+    // Find the largest background rect of the given opaqueness.
+    if (const FillLayer* current = &(style()->backgroundLayers())) {
+        do {
+            const FillLayer* cur = current;
+            current = current->next();
+            if (rectOpacity == OpaqueBackgroundRect) {
+                if (cur->blendMode() != WebBlendModeNormal || cur->composite() != CompositeSourceOver)
+                continue;

[chrishtr, 2016/08/12 17:03:00]

Indentation.

[flackr, 2016/08/16 17:52:51]

Done.

+                // Check if the image or color has alpha.
+                if (const StyleImage* image = cur->image()) {
+                    if (!image->knownToBeOpaque(*this))
+                        continue;
+                } else {

[trchen, 2016/08/13 00:06:42]

}
if (!current->next()) {

1. A layer with translucent image can still have opaque color below.
2. Background color is only applicable on bottom layer.

Also I recall schenney@ may have wrote a helper function to determine whether a
fill layer is opaque. We should reuse it if it's there.

[flackr, 2016/08/16 17:52:51]

Yes, that is why we continue to the next layer if we encounter a translucent
image.
Hmm, I had mistakenly assumed that !cur->image() implies the background color,
but at least "none" can be specified in a background list which has no image -
I've added a test for this..
The only related patch I can find recently is
https://codereview.chromium.org/1826013002 which only checked the background
color property. I've checked FillLayer but it doesn't seem to have any such
property, but perhaps the contents of this loop should become a helper on
FillLayer to this end.

[trchen, 2016/08/17 01:22:55]

The background color is always applicable even if the bottom layer has an image,
so this line should be 'if' instead of 'else if'. See
BoxPainter::paintFillLayer().

[flackr, 2016/08/17 18:20:55]

Thanks for the clarification. Done, but needed to rework this to be a neither
condition. If neither the image or color is opaque, then continue.

+                    Color backgroundColor = resolveColor(CSSPropertyBackgroundColor);
+                    if (backgroundColor.hasAlpha())
+                        continue;
+                }
+            }
+            EFillBox currentClip = cur->clip();
+            // Adjust clip if attachment is local.
+            if (currentClip == BorderFillBox && cur->attachment() == LocalBackgroundAttachment)
+                currentClip = PaddingFillBox;
+            backgroundBox = enclosingFillBox(backgroundBox, currentClip);

[trchen, 2016/08/13 00:06:42]

I'm worried about this line, because the unscrolled content box and the local
content box are different boxes.

http://jsbin.com/dujuzazeva/ demonstrates the difference.

[flackr, 2016/08/16 17:52:51]

Right, I think we should treat background-attachment: local content-box as the
padding box when we're asking for the "translucent" / clip / cull rect (i.e.
over-estimate is okay) but the content-box when we're asking for the opaque
background rect (i.e. under-estimate is okay). Perhaps this will be more clear
with the new enum values.

[trchen, 2016/08/17 01:22:55]

Agreed. We can do that plus a comment to explain.

[flackr, 2016/08/17 18:20:55]

Done, and added a test for this case.

+        } while (current);
+    }
+    switch (backgroundBox) {
     case BorderFillBox:
-        // A 'border-box' clip on scrollable elements local attachment is treated as 'padding-box'.
-        // https://www.w3.org/TR/css3-background/#the-background-attachment
-        if (!style()->isOverflowVisible() && style()->backgroundLayers().attachment() == LocalBackgroundAttachment)

[chrishtr, 2016/08/12 17:03:00]

Why can you remove this?

[trchen, 2016/08/13 00:06:42]

The matching logic is in line 730 on the right side.

-            return paddingBoxRect();
         return borderBoxRect();
         break;
     case PaddingFillBox:
@@ -1376,10 +1397,6 @@ bool LayoutBox::backgroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect) c
     if (isDocumentElement() || backgroundStolenForBeingBody())
         return false;
 
-    Color backgroundColor = resolveColor(CSSPropertyBackgroundColor);
-    if (backgroundColor.hasAlpha())
-        return false;
-
     // If the element has appearance, it might be painted by theme.
     // We cannot be sure if theme paints the background opaque.
     // In this case it is safe to not assume opaqueness.
@@ -1393,10 +1410,9 @@ bool LayoutBox::backgroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect) c
         return false;
     if (hasClipPath())
         return false;
-    // FIXME: The background color clip is defined by the last layer.
-    if (style()->backgroundLayers().next())
+    if (style()->hasBlendMode())
         return false;
-    return backgroundClipRect().contains(localRect);
+    return backgroundRect(OpaqueBackgroundRect).contains(localRect);
 }
 
 static bool isCandidateForOpaquenessTest(const LayoutBox& childBox)
@@ -1769,13 +1785,12 @@ void LayoutBox::mapAncestorToLocal(const LayoutBoxModelObject* ancestor, Transfo
 
     bool isFixedPos = style()->position() == FixedPosition;
 
-    if (style()->canContainFixedPositionObjects() && !isFixedPos) {
-        // If this box has a transform or contains paint, it acts as a fixed position container for fixed descendants,
-        // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
+    // If this box has a transform or contains paint, it acts as a fixed position container for fixed descendants,
+    // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
+    if (style()->canContainFixedPositionObjects() && !isFixedPos)
         mode &= ~IsFixed;
-    } else if (isFixedPos) {
+    else if (isFixedPos)
         mode |= IsFixed;
-    }
 
     LayoutBoxModelObject::mapAncestorToLocal(ancestor, transformState, mode);
 }