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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by dsinclair
Modified:
2 years, 10 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.
2 years, 10 months 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 ...
2 years, 10 months 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 ...
2 years, 10 months 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 ...
2 years, 10 months 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 ...
2 years, 10 months ago (2014-12-16 16:05:53 UTC) #7
Julien - ping for review
+the right Morten :)
2 years, 10 months 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 ...
2 years, 10 months 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 ...
2 years, 10 months 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, 10 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, 10 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, 10 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, 10 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, 10 months ago (2014-12-17 20:44:08 UTC) #16
mstensho (USE GERRIT)
lgtm
2 years, 10 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, 10 months ago (2014-12-19 04:29:24 UTC) #19
commit-bot: I haz the power
2 years, 10 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa