New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 613728 link

Starred by 5 users
Status: Fixed
Owner:
dgro...@chromium.org
Closed: Oct 2016
Cc:
pdr@chromium.org
esprehn@chromium.org
█ tabatkins@chromium.org
chrishtr@chromium.org
dgro...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

border collapsing -- table width is wrong when width of collapsed borders changes

Project Member Reported by wangxianzhu@chromium.org, May 20 2016 
This bug extends  bug 609674  to all collapsed borders.  Bug 609674  fixed the case of row border width change. We also need to update table width when other border widths change.

Discovered this when I converted collapsed border repaint tests into ref tests (https://codereview.chromium.org/2005553002). Table widths are different in tests that dynamically change collapsed border width and refs that defines collapsed borders statically.

dgrogan@ could you work on this?

Comment 1 by dgro...@chromium.org, May 21 2016 

I noticed the tbody case ( issue 613220 ) but I see that your try jobs failed tests that changed cell, col and table border widths too. I didn't test col or table. But I did test changing cell widths (https://jsfiddle.net/dgrogan/sLhgk2sk/1/) and that seems to work already.

Should I create new checkLayout tests for these (like I did with rows) or just reuse your repaint tests, converting them to ref tests as I go?

Comment 2 by dgro...@chromium.org, May 21 2016 

fast/table/border-collapsing/cached-cell-remove.html is interesting. Removing an interior cell obviously shouldn't affect the table width but it does, decreasing the width by 1 pixel.

Comment 3 by wangxianzhu@chromium.org, May 21 2016 

Thanks for working on this bug.

I can mark the tests fail in my CL and land it first, then you fix this bug and remove the Fail entries.

In fast/table/border-collapsing/cached-cell-remove.html, a cell with border-left-width: 6px is removed. Before the cell is removed, because the border is shared between the removed cell and the cell left to the cell, the border also contributes 3px to the left cell. When the middle cell is removed, the left cell will use its own border (width 4px) as its right border which contributes 2px to the cell, so the width of the left cell should reduce by 1px. That said, border-width change of a cell may also affect layout of the adjacent cells.

Comment 4 by wangxianzhu@chromium.org, May 21 2016

Cc: chrishtr@chromium.org pdr@chromium.org
Just found that Firefox's behavior is the same (wrong) as Chrome: no relayout when width of collapsed-border changes.

For http://output.jsbin.com/jicuji, only IE's behavior is correct. I wonder if we should fix this bug which would make us different from FireFox and Safari.

Add API owners for more opinions.

Comment 5 by pdr@chromium.org, May 21 2016

Cc: esprehn@chromium.org tabatkins@chromium.org
Interesting find! Here's another version of the bug: http://jsbin.com/wixide

It looks like we just fail to properly re-layout the table since zooming and unzooming fixes the issue. I wonder if both Blink and Gecko/Firefox are relying on the same bad table optimization? I think this is just a rare case where both Gecko and Blink share a similar bug. I think we could fix this without affecting web compatibility.

+cc Elliott and TabAtkins, can you think of any reason this isn't a bug in Blink? I don't see anything in the spec about this.


Safari9.1.1/dev share our layout bugs and has a repaint bug too. I'll file that separately against WebKit.

Comment 6 by pdr@chromium.org, May 21 2016 

There's already some discussion on the layout issue on the WebKit tracker: https://bugs.webkit.org/show_bug.cgi?id=157967

Comment 7 by chrishtr@chromium.org, May 23 2016 

I also think it sounds like we can just fix the bug.

Comment 8 by pdr@chromium.org, May 23 2016 

Just FYI, WebKit has an approach up for review and may land it:
https://bugs.webkit.org/attachment.cgi?id=279578&action=prettypatch

Comment 9 by dgro...@chromium.org, May 24 2016 

 Issue 613220  has been merged into this issue.

Comment 10 Deleted

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 13 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/596c28c4ab1759719fe00b8319571fba398f48e1

commit 596c28c4ab1759719fe00b8319571fba398f48e1
Author: dgrogan <dgrogan@chromium.org>
Date: Wed Jul 13 04:00:00 2016

[css-tables] Set table and cell widths dirty when section border changes

When borders collapse, section borders affect both the table width and
cell widths. Previously, tables and cells would use the old width when a
section's border changed.

BUG= 613728 

Review-Url: https://codereview.chromium.org/2013693002
Cr-Commit-Position: refs/heads/master@{#404994}

[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/border-collapsing/cached-change-tbody-border-width-expected.txt
[add] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/change-tbody-border-width-expected.txt
[add] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/change-tbody-border-width.html
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableSection.h
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/TableLayoutAlgorithmFixed.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/style/BorderData.h
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 13 2016

Labels: merge-merged-2795
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/596c28c4ab1759719fe00b8319571fba398f48e1

commit 596c28c4ab1759719fe00b8319571fba398f48e1
Author: dgrogan <dgrogan@chromium.org>
Date: Wed Jul 13 04:00:00 2016

[css-tables] Set table and cell widths dirty when section border changes

When borders collapse, section borders affect both the table width and
cell widths. Previously, tables and cells would use the old width when a
section's border changed.

BUG= 613728 

Review-Url: https://codereview.chromium.org/2013693002
Cr-Commit-Position: refs/heads/master@{#404994}

[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/border-collapsing/cached-change-tbody-border-width-expected.txt
[add] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/change-tbody-border-width-expected.txt
[add] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/LayoutTests/fast/table/change-tbody-border-width.html
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/LayoutTableSection.h
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/layout/TableLayoutAlgorithmFixed.cpp
[modify] https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1/third_party/WebKit/Source/core/style/BorderData.h
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37d40efe1477a9ca978115a69aa369b156659931

commit 37d40efe1477a9ca978115a69aa369b156659931
Author: dgrogan <dgrogan@chromium.org>
Date: Fri Jul 15 19:25:46 2016

[css-tables] Set needsLayout on cells when column border width changes

When borders collapse, col/colgroup borders affect cell widths but we
weren't marking cells properly. As a result cells and tables were using
their old widths.

BUG= 613728 

Review-Url: https://codereview.chromium.org/2149953004
Cr-Commit-Position: refs/heads/master@{#405818}

[modify] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/LayoutTests/fast/table/border-collapsing/cached-change-col-border-width-expected.txt
[modify] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/LayoutTests/fast/table/border-collapsing/cached-change-colgroup-border-width-expected.txt
[add] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/LayoutTests/fast/table/change-col-border-width-expected.txt
[add] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/LayoutTests/fast/table/change-col-border-width.html
[modify] https://crrev.com/37d40efe1477a9ca978115a69aa369b156659931/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6

commit 4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6
Author: dgrogan <dgrogan@chromium.org>
Date: Sat Jul 23 03:36:51 2016

[css-tables] Mark sibling cells dirty when a cell is removed

Cells's border boxes were not getting resized when their collapsed
border changed because a sibling was removed.

BUG= 613728 

Review-Url: https://codereview.chromium.org/2173163003
Cr-Commit-Position: refs/heads/master@{#407339}

[modify] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/fast/table/remove-cell-with-border-box-expected.html
[add] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/fast/table/remove-cell-with-border-box.html
[add] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/fast/table/remove-cell-with-large-border-width-expected.txt
[add] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/fast/table/remove-cell-with-large-border-width.html
[modify] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/table-collapsed-border-expected.txt
[modify] https://crrev.com/4f82f2cf4077cb507ed9c8ab7a577073bf9d8eb6/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31dd85fea5e602d90f9b5372e0d5daaf05e2342e

commit 31dd85fea5e602d90f9b5372e0d5daaf05e2342e
Author: dgrogan <dgrogan@chromium.org>
Date: Wed Sep 21 23:17:41 2016

[css-tables] Set needsLayout on cells when table border width changes

When borders collapse, table borders affect cell widths but we weren't
marking cells properly. As a result cells and tables were using their
old widths.

BUG= 613728 

Review-Url: https://codereview.chromium.org/2358663003
Cr-Commit-Position: refs/heads/master@{#420193}

[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/LayoutTests/fast/table/change-table-border-width-expected.txt
[add] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/LayoutTests/fast/table/change-table-border-width.html
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/LayoutTests/paint/invalidation/table/cached-change-table-border-width-expected.txt
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/LayoutTableSection.h
[modify] https://crrev.com/31dd85fea5e602d90f9b5372e0d5daaf05e2342e/third_party/WebKit/Source/core/layout/TableLayoutAlgorithmFixed.cpp

Comment 22 by dgro...@chromium.org, Oct 4 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment