Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

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

Created:
3 years ago by dsinclair
Modified:
2 years, 12 months 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 : #

Messages

Total messages: 20 (4 generated)
dsinclair
PTAL.
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 years ago (2014-12-16 16:05:53 UTC) #7
Julien - ping for review
+the right Morten :)
3 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 ...
3 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 ...
3 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 ...
2 years, 12 months 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 ...
2 years, 12 months 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 > ...
2 years, 12 months 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 ...
2 years, 12 months 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 ...
2 years, 12 months ago (2014-12-17 20:44:08 UTC) #16
mstensho (USE GERRIT)
lgtm
2 years, 12 months 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
2 years, 12 months ago (2014-12-19 04:29:24 UTC) #19
commit-bot: I haz the power
2 years, 12 months 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 0eb02b776