Changing border color and size simultaneously fails to repaint

[Philip Rogers]

Created attachment 279536 [details]
Dynamic table border style repaint bug

We're working on a table border sizing bug in https://crbug.com/613728 where all browsers do something a little weird wrt updating table borders. I noticed a repaint bug in Safari 9.1.1/dev while investigating this.

In the attached testcase, there should be no red visible after 500ms. In Safari, red is visible after 500ms and pressing ctrl+a causes a repaint which makes the red disappear.

[zalan]

Created attachment 279543 [details]
Test case

I assume if we fix the layout issue (so that we relayout all the boxes affected by the collapsed border change) the repaint bug will go away.

[zalan]

Marking the affected cells dirty (including the prefWidth bit) fixes the layout issue (and it fixes the missing repaint too)

[zalan]

I wonder how common this collapse border related style change is as now with this fix, we do a lot more work.

diff --git a/Source/WebCore/rendering/RenderTable.cpp b/Source/WebCore/rendering/RenderTable.cpp
index d8a8d6e..f6289b6 100644
--- a/Source/WebCore/rendering/RenderTable.cpp
+++ b/Source/WebCore/rendering/RenderTable.cpp
@@ -594,6 +594,14 @@ void RenderTable::layout()
     clearNeedsLayout();
 }
 
+static inline void markCellDirtyWhenCollapsedBorderChanges(RenderTableCell* cell)
+{
+    if (!cell)
+        return;
+    cell->invalidateHasEmptyCollapsedBorders();
+    cell->setNeedsLayoutAndPrefWidthsRecalc();
+}
+
 void RenderTable::invalidateCollapsedBorders(RenderTableCell* cellWithStyleChange)
 {
     m_collapsedBordersValid = false;
@@ -608,14 +616,10 @@ void RenderTable::invalidateCollapsedBorders(RenderTableCell* cellWithStyleChang
     if (cellWithStyleChange) {
         // It is enough to invalidate just the surrounding cells when cell border style changes.
         cellWithStyleChange->invalidateHasEmptyCollapsedBorders();
-        if (auto* below = cellBelow(cellWithStyleChange))
-            below->invalidateHasEmptyCollapsedBorders();
-        if (auto* above = cellAbove(cellWithStyleChange))
-            above->invalidateHasEmptyCollapsedBorders();
-        if (auto* before = cellBefore(cellWithStyleChange))
-            before->invalidateHasEmptyCollapsedBorders();
-        if (auto* after = cellAfter(cellWithStyleChange))
-            after->invalidateHasEmptyCollapsedBorders();
+        markCellDirtyWhenCollapsedBorderChanges(cellBelow(cellWithStyleChange));
+        markCellDirtyWhenCollapsedBorderChanges(cellAbove(cellWithStyleChange));
+        markCellDirtyWhenCollapsedBorderChanges(cellBefore(cellWithStyleChange));
+        markCellDirtyWhenCollapsedBorderChanges(cellAfter(cellWithStyleChange));
         return;
     }

[zalan]

rdar://problem/26423918

[zalan]

Created attachment 279578 [details]
Patch

[Dave Hyatt]

Comment on attachment 279578 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 279578 [details]
Patch

Clearing flags on attachment: 279578

Committed r201296: <http://trac.webkit.org/changeset/201296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158002

[zalan]

Created attachment 279603 [details]
Patch

[zalan]

Added document.body.offsetWidth to force layout.

[WebKit Commit Bot]

Comment on attachment 279603 [details]
Patch

Clearing flags on attachment: 279603

Committed r201312: <http://trac.webkit.org/changeset/201312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[zalan]

see https://bugs.webkit.org/show_bug.cgi?id=158009

[zalan]

http://trac.webkit.org/changeset/201346