Fix table cell background painting

third_party/WebKit/Source/core/paint/TableCellPainter.cpp

Index: third_party/WebKit/Source/core/paint/TableCellPainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/TableCellPainter.cpp b/third_party/WebKit/Source/core/paint/TableCellPainter.cpp
index d30a77340c74afe4822b12116fa57d3f56be879b..973527f990cc7a4b48ca121ab3e34827aa4eb35c 100644
--- a/third_party/WebKit/Source/core/paint/TableCellPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/TableCellPainter.cpp
@@ -5,6 +5,7 @@
 #include "core/paint/TableCellPainter.h"
 
 #include "core/layout/LayoutTableCell.h"
+#include "core/layout/LayoutTableCol.h"
 #include "core/paint/BlockPainter.h"
 #include "core/paint/BoxPainter.h"
 #include "core/paint/LayoutObjectDrawingRecorder.h"
@@ -140,8 +141,13 @@ void TableCellPainter::paintCollapsedBorders(const PaintInfo& paintInfo, const L
     }
 }
 
-void TableCellPainter::paintBackgroundsBehindCell(const PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutObject* backgroundObject, DisplayItem::Type type)
+void TableCellPainter::paintBackgroundsBehindCell(const PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutBox* backgroundObject, DisplayItem::Type type)
 {
+    if (RuntimeEnabledFeatures::newTableCellBackgroundPaintingEnabled() && backgroundObject != &m_layoutTableCell) {
+        paintParentBackgroundsBehindCell(paintInfo, paintOffset, backgroundObject, type);
+        return;
+    }
+
     if (!backgroundObject)
         return;
 
@@ -180,6 +186,68 @@ void TableCellPainter::paintBackgroundsBehindCell(const PaintInfo& paintInfo, co
     }
 }
 
+void TableCellPainter::paintParentBackgroundsBehindCell(const PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutBox* backgroundObject, DisplayItem::Type type)
+{
+    if (!backgroundObject)
+        return;
+
+    if (m_layoutTableCell.style()->visibility() != VISIBLE)
+        return;
+
+    LayoutTable* tableElt = m_layoutTableCell.table();

[Xianzhu, 2016/03/28 00:44:43]

s/tableElt/table/

[atotic1, 2016/03/29 17:05:17]

Done.

+    if (!tableElt->collapseBorders() && m_layoutTableCell.style()->emptyCells() == EmptyCellsHide && !m_layoutTableCell.firstChild())
+        return;
+
+    LayoutRect backgroundRect = paintBoundsParent(paintOffset, backgroundObject, type);
+
+    LayoutRect cellRect = m_layoutTableCell.frameRect();
+    cellRect.moveBy(paintOffset);
+
+    cellRect.setLocation(LayoutPoint(cellRect.pixelSnappedLocation()));
+    cellRect.setSize(LayoutSize(cellRect.pixelSnappedSize()));
+
+    // We paint background LTR, top->bottom.
+    // This makes code much simpler, but is not spec-conformant.
+    // The spec-conformant code would have to handle all writing-mode
+    // painting directions correctly.

[Xianzhu, 2016/03/28 00:44:42]

Convert the above to a TODO, and reference a bug. You can use
TODO(crbug.com/...): style if you are not going to fix it.

[atotic1, 2016/03/29 17:05:17]

Done.

+
+    if (backgroundRect.x() > cellRect.x())
+        backgroundRect.setX(cellRect.x());
+    if (backgroundRect.y() > cellRect.y())
+        backgroundRect.setY(cellRect.y());

[Xianzhu, 2016/03/28 00:44:43]

Why does this happen?

[atotic1, 2016/03/29 17:05:17]

This happens when table is vertical. I've reworked the code to be much simpler.
It will be incorrect, see referenced bug for more details.

+
+    Optional<LayoutObjectDrawingRecorder> recorder;

[Xianzhu, 2016/03/28 00:44:42]

This recorder doesn't need to be optional. You can declare and initialize it at
the same time at line 223. 

[atotic1, 2016/03/29 17:05:17]

Done.

+    if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(paintInfo.context, m_layoutTableCell, type)) {
+        return;
+    }
+    recorder.emplace(paintInfo.context, m_layoutTableCell, type, cellRect);
+
+    Color c = backgroundObject->resolveColor(CSSPropertyBackgroundColor);

[Xianzhu, 2016/03/28 00:44:42]

s/c/color/

[atotic1, 2016/03/29 17:05:17]

Done.

+    const FillLayer& bgLayer = backgroundObject->style()->backgroundLayers();

[Xianzhu, 2016/03/28 00:44:43]

s/bgLayer/backgroundLayer/

[atotic1, 2016/03/29 17:05:17]

Done.

+    FillLayer bgAdjustedLayer(bgLayer);

[Xianzhu, 2016/03/28 00:44:42]

Not used?

[atotic1, 2016/03/29 17:05:17]

Done.

+
+    if (bgLayer.hasImage() || c.alpha()) {
+
+        GraphicsContextStateSaver stateSaver(paintInfo.context, true);
+        LayoutRect clipRect(m_layoutTableCell.location(), m_layoutTableCell.size());
+        clipRect.moveBy(paintOffset);
+        paintInfo.context.clip(pixelSnappedIntRect(clipRect));
+
+        // If cell is larger than background, we might have to paint multiple times.
+        LayoutUnit colExtraPixels = std::max(LayoutUnit(0), cellRect.maxX() - backgroundRect.maxX());
+        unsigned colRepeat = 1 + (colExtraPixels / backgroundRect.width()) + (colExtraPixels % backgroundRect.width() != 0);
+        LayoutUnit rowExtraPixels = std::max(LayoutUnit(0), cellRect.maxY() - backgroundRect.maxY());
+        unsigned rowRepeat = 1 + (rowExtraPixels / backgroundRect.height()) + (rowExtraPixels % backgroundRect.height() != 0);
+        for (unsigned row = 0; row < rowRepeat; row++) {
+            for (unsigned col = 0; col < colRepeat; col++) {
+                LayoutRect paintRect = backgroundRect;
+                paintRect.move(col * paintRect.width(), row * paintRect.height());
+                BoxPainter(*backgroundObject).paintFillLayers(paintInfo, c, bgLayer, paintRect, BackgroundBleedNone, SkXfermode::kSrcOver_Mode, backgroundObject);
+            }
+        }

[Xianzhu, 2016/03/28 00:44:43]

The following case is broken with the code:

<!DOCTYPE html>
<style>
td {
  width: 100px;
  height: 100px;
  border: 2px solid red;
}
</style>
<table>
  <tr style="background:
url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)">
    <td rowspan="2">11</td>
    <td>12</td>
  </tr>
  <tr>
    <td>22</td>
  </tr>
</table>


The '11' cell should have continuous background spanning 2 rows. It displays
wrong with the new code, but correct with the original code. (Your code does fix
the background of the second cell.)

I think you don't need the above loop, and nor the size part of the rect
returned from  'positionByCellSpan()' methods. You just need to adjust the
offset of the background. In this way, you don't need positionByCellSpan()
methods, but just the offset of the cell from the backgroundObject, and the CL
can be greatly simplified. 

[atotic1, 2016/03/28 20:45:33]

You are correct in that we fail this test case. My messy code was trying to
approximate
background image tiling, which is what BoxPainter also does. Initially, I tried
using BoxPainter for tiling, and it did not work.

The proper fix is to fix BoxPainter API so it can handle this use case.

BoxPainter is undocumented. This is my understanding of the API, 
and I'd love to learn  that I am wrong, and there is a call I 
can use without modification.

I'll use <tr> to illustrate the problem:
- tr image background has to be painted inside the entire cell. 
  It needs to support all the usual background-image properties. 
  Example 1: 
    background-image-size: 100% 100%; 
      should paint an image that is 100% of the size of the row
- if the cell size extends beyond row size, background should
  fill the extended area.

This cannot be done with BoxPainter API today because:

BoxPainter::paintFillLayers(const PaintInfo& paintInfo, 
const Color& c, 
const FillLayer& fillLayer, 
const LayoutRect& paintRect, 
BackgroundBleedAvoidance bleedAvoidance, 
SkXfermode::Mode op, 
const LayoutObject* backgroundObject)

uses paintRect for:
1) Image scaling: paintRect is a containment rectangle that determines image
size
2) Paint destination: paintRect is the area to be painted

Simply put, there is no way to use <tr> frame for image scaling, and cell frame 
for image painting in current API.

For this to be fixed, BoxPainter::paintFillLayer needs to be modified to allow
separate rectangles: one for image scaling, another as paint destination.

This will be a slightly larger change, there are 7 calls to paintFillLayer
in our code.

I see no other way to paint tiled backgrounds correctly. The code that handles
all permutations of background-image properties is very complex, and I do 
not want to reinvent it.
To paint the background of every cell correctly, I need two things:
- background size
- background origin with respect to cell

How can I paint the background correctly without knowing the its size? 
The tiling properties need to know element's size.

[Xianzhu, 2016/03/28 21:26:04]

Because the background bug has been there for a long time, many developers have
learned to avoid it. If a fix to a bug is to introduce new bugs, we'd rather not
to fix the bug. Unsized background may be more common than sized background, so
we should not break it.

Please modify paintFillLayer to accept separate rectangles if you would like to
continue the work to fix the bug.
You are right. Sorry I overlooked the percentage-sized background case.

+    }
+}
+
 void TableCellPainter::paintBoxDecorationBackground(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     LayoutTable* table = m_layoutTableCell.table();
@@ -238,5 +306,41 @@ LayoutRect TableCellPainter::paintBounds(const LayoutPoint& paintOffset, PaintBo
     return LayoutRect(adjustedPaintOffset, LayoutSize(m_layoutTableCell.pixelSnappedSize()));
 }
 
+LayoutRect TableCellPainter::paintBoundsParent(const LayoutPoint& paintOffset, const LayoutBox* backgroundObject, DisplayItem::Type type)
+{
+    LayoutRect position;
+    switch (type) {
+    case DisplayItem::TableCellBackgroundFromColumnGroup:
+    case DisplayItem::TableCellBackgroundFromColumn:
+        {

[Xianzhu, 2016/03/28 00:44:43]

Put '{' at the end of the previous line.

[atotic1, 2016/03/29 17:05:17]

Done.

+        position = static_cast<const LayoutTableCol*>(backgroundObject)->positionByCellSpan(
+            m_layoutTableCell.absoluteColumnIndex());
+        // position is relative to table, must correct for section location
+        LayoutPoint sectionLocation = m_layoutTableCell.section()->location();
+        position.moveBy(-sectionLocation);
+        position.moveBy(paintOffset);
+        }

[Xianzhu, 2016/03/28 00:44:43]

Put break inside of '}' and align '}' with 'case'.

[atotic1, 2016/03/29 17:05:17]

Done.

+        break;
+    case DisplayItem::TableCellBackgroundFromSection:

[Xianzhu, 2016/03/28 00:44:42]

We know TableCellSection's paint bounds in TableCellPainter. You can just pass
it from the callsite into TableCellPaint::paintBackgroundsBehindCell(). In this
way, you don't need LayoutTableSection::positionByCellSpan().

[atotic1, 2016/03/29 17:05:17]

How do we know TableSection bounds inside TableCellPainter?

+        position = static_cast<const LayoutTableSection*>(backgroundObject)->positionByCellSpan();
+        position.moveBy(paintOffset);
+        break;
+    case DisplayItem::TableCellBackgroundFromRow:
+        position = static_cast<const LayoutTableRow*>(backgroundObject)->positionByCellSpan();
+        position.moveBy(paintOffset);
+        break;

[Xianzhu, 2016/03/28 00:44:42]

Ditto.

+    case DisplayItem::BoxDecorationBackground: {
+        LayoutPoint adjustedPaintOffset = paintOffset;
+        adjustedPaintOffset.moveBy(m_layoutTableCell.location());
+        position =  LayoutRect(adjustedPaintOffset, LayoutSize(m_layoutTableCell.pixelSnappedSize()));
+        }
+        break;
+    default:
+        ASSERT(false);

[Xianzhu, 2016/03/28 00:44:42]

ASSERT_NOT_REACHED();

[atotic1, 2016/03/29 17:05:17]

Done.

+        break;
+    }
+    return LayoutRect(position.pixelSnappedLocation(), position.pixelSnappedSize());
+}
+
 } // namespace blink