Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 804383002: Layout captions in simplifiedNormalFlowLayout for tables. (Closed)

Created:
6 years ago by dsinclair
Modified:
6 years ago
CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1
Target Ref:
refs/remotes/origin/master
Project:
blink
Visibility:
Public.

Description

Layout captions in simplifiedNormalFlowLayout for tables. It is currently possible to get into simplifiedNormalFlowLayout for a table while the captions need layout. Currently the simplified code will just iterate over all of the table sections and lay them out if needed. This CL adds iteration over the table captions as well and makes sure they're also laid out during simplifedNormalFlowLayout. BUG=414109 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187509

Patch Set 1 #

Total comments: 13

Patch Set 2 : Review feedback #

Total comments: 8

Patch Set 3 : #

Patch Set 4 : #

Total comments: 1

Patch Set 5 : #

Total comments: 4

Patch Set 6 : #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, --1 lines) Patch
A LayoutTests/fast/table/caption-position.html View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/caption-position-expected.html View 1 2 3 4 5 6 1 chunk +12 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/display-table-caption-crash.html View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A + LayoutTests/fast/table/display-table-caption-crash-expected.txt View 1 0 chunks +-1 lines, --1 lines 0 comments Download
M Source/core/rendering/RenderTable.cpp View 1 3 4 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (4 generated)
dsinclair
PTAL.
6 years ago (2014-12-16 15:00:23 UTC) #2
mstensho (USE GERRIT)
Drive-by comments. Not adding myself as reviewer. I'll leave reviewing to Julien, once he enters ...
6 years ago (2014-12-16 15:27:01 UTC) #3
dsinclair
https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/TestExpectations#newcode1100 LayoutTests/TestExpectations:1100: crbug.com/414109 fast/css/display-table-caption.html [ NeedsRebaseline ] On 2014/12/16 15:27:01, mstensho ...
6 years ago (2014-12-16 15:40:33 UTC) #4
mstensho (USE GERRIT)
https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display-table-caption.html File LayoutTests/fast/css/display-table-caption.html (right): https://codereview.chromium.org/804383002/diff/1/LayoutTests/fast/css/display-table-caption.html#newcode14 LayoutTests/fast/css/display-table-caption.html:14: <option> On 2014/12/16 15:40:32, dsinclair wrote: > On 2014/12/16 ...
6 years ago (2014-12-16 15:43:32 UTC) #5
Julien - ping for review
Morten, you should feel free to review patches. We all started with informal reviews and ...
6 years ago (2014-12-16 16:05:53 UTC) #7
Julien - ping for review
+the right Morten :)
6 years ago (2014-12-16 16:06:19 UTC) #9
dsinclair
https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/RenderTable.cpp File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/RenderTable.cpp#newcode390 Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 16:05:53, Julien ...
6 years ago (2014-12-16 17:23:19 UTC) #10
mstensho (USE GERRIT)
https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/RenderTable.cpp File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/1/Source/core/rendering/RenderTable.cpp#newcode390 Source/core/rendering/RenderTable.cpp:390: for (auto& caption : m_captions) On 2014/12/16 16:05:53, Julien ...
6 years ago (2014-12-16 20:37:07 UTC) #11
dsinclair
I took a look at turning this into a check-layout.js test but ran into a ...
6 years ago (2014-12-17 16:46:07 UTC) #12
mstensho (USE GERRIT)
https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/RenderTable.cpp File Source/core/rendering/RenderTable.cpp (right): https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/RenderTable.cpp#newcode393 Source/core/rendering/RenderTable.cpp:393: layoutCaption(*caption); This would become the only place in the ...
6 years ago (2014-12-17 19:03:07 UTC) #13
dsinclair
On 2014/12/17 19:03:07, mstensho wrote: > https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/RenderTable.cpp > File Source/core/rendering/RenderTable.cpp (right): > > https://codereview.chromium.org/804383002/diff/60001/Source/core/rendering/RenderTable.cpp#newcode393 > ...
6 years ago (2014-12-17 20:12:44 UTC) #14
mstensho (USE GERRIT)
https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/caption-position-expected.html File LayoutTests/fast/table/caption-position-expected.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/caption-position-expected.html#newcode1 LayoutTests/fast/table/caption-position-expected.html:1: Test passes is above is on top. Should have ...
6 years ago (2014-12-17 20:36:02 UTC) #15
dsinclair
https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/caption-position-expected.html File LayoutTests/fast/table/caption-position-expected.html (right): https://codereview.chromium.org/804383002/diff/80001/LayoutTests/fast/table/caption-position-expected.html#newcode1 LayoutTests/fast/table/caption-position-expected.html:1: Test passes is above is on top. On 2014/12/17 ...
6 years ago (2014-12-17 20:44:08 UTC) #16
mstensho (USE GERRIT)
lgtm
6 years ago (2014-12-17 20:47:35 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/804383002/120001
6 years ago (2014-12-19 04:29:24 UTC) #19
commit-bot: I haz the power
6 years ago (2014-12-19 05:39:28 UTC) #20
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187509

Powered by Google App Engine
This is Rietveld 408576698