Margins on children of display:table-cell elements get stuck at highest value.

Margins on children of display:table-cell elements get stuck at highest value.

Row's beforeMargin was not resetting back to 0 when margin of the
cell's child is changed from 40px tp 0.

Calculating the row's baseline excluding intrinsic padding because
row's baseline will come into it's right position when row's baseline
would be calculated for the cell which is introducing intrinsic padding
for other cells.

R=ojan@chromium.org
BUG=241331
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155864

[ojan, 2013-08-07 23:03:12]

I'd prefer Julien to review the C++ side of this if he doesn't mind. He's much
more familiar with table layout code than I am.

https://codereview.chromium.org/22589002/diff/1/LayoutTests/fast/table/table-...
File LayoutTests/fast/table/table-toggle-paragraph-padding.html (right):

https://codereview.chromium.org/22589002/diff/1/LayoutTests/fast/table/table-...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:11:
setTimeout(funRemoveClass, 0);
I think you can do this without a setTimeout. See the test case in
https://code.google.com/p/chromium/issues/detail?id=241331#c14. In fact, it
would be better if you just used that test case since it's a lot more reduced
than this.

Also, new tests should avoid being pixel tests if at all possible. In this case,
this can be a ref test, where is the second div in my test case and the
reference is the first.

https://codereview.chromium.org/22589002/diff/1/LayoutTests/fast/table/table-...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:13: /*function
buttonClick()
Please remove dead code.

[a.suchit, 2013-08-08 10:19:29]

https://codereview.chromium.org/22589002/diff/1/LayoutTests/fast/table/table-...
File LayoutTests/fast/table/table-toggle-paragraph-padding.html (right):

https://codereview.chromium.org/22589002/diff/1/LayoutTests/fast/table/table-...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:11:
setTimeout(funRemoveClass, 0);
On 2013/08/07 23:03:12, ojan wrote:
> I think you can do this without a setTimeout. See the test case in
> https://code.google.com/p/chromium/issues/detail?id=241331#c14. In fact, it
> would be better if you just used that test case since it's a lot more reduced
> than this.
> 
> Also, new tests should avoid being pixel tests if at all possible. In this
case,
> this can be a ref test, where is the second div in my test case and the
> reference is the first.

Change into Ref Test.

[Julien - ping for review, 2013-08-08 22:15:51]

The C++ code change is fine. The test needs a bit more love but it's looking
good.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-toggle-paragraph-padding-expected.html
(right):

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding-expected.html:21: <div
class="cell"><div class="thinger toggle" style="margin:0px">b</div></div>
The inline style declaration is unneeded, the initial value for all margin's
longhands is already 0.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-toggle-paragraph-padding.html (right):

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:4: .cell .thinger {
background-color: salmon; }
This can be simplified to .thinger { ... }

Also the class name could be improved by just saying what it does:
backgroundSalmon.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:10: <h4>Row's before
margin was not reseting back to 0 when margin of the cell's child is changed
from 40px to 0.</h4>
s/40px/100px/

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:14: <div>
Do we really need to use an anonymous table by using CSS instead of explicit
<table> / <td> tags?

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:26:
document.querySelector('.toggle').style.margin = 0;
This really look like it could be a checkLayout() test instead of a much slower
ref-test.

https://codereview.chromium.org/22589002/diff/7001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/22589002/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:476: // Calculating the row's
baseline excluding intrinsic padding because row's baseline will come into it's
right
s/it's/its/ :(

https://codereview.chromium.org/22589002/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:477: // position when row's
baseline would be calculated for the cell which introduced intrinsic padding for
other cells.
Let's try to make comment good English sentence. That means that words have an
article in front of them (usually 'the').

// Ignoring the intrinsic padding as it depends on knowing the row's baseline,
which won't be accurate
// until the end of this function.

[ojan, 2013-08-08 22:19:29]

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-toggle-paragraph-padding.html (right):

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:14: <div>
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> Do we really need to use an anonymous table by using CSS instead of explicit
> <table> / <td> tags?

I tried that actually and couldn't get it to reproduce. I didn't dig into why.

[suchit.agrawal, 2013-08-09 11:00:42]

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-toggle-paragraph-padding.html (right):

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:4: .cell .thinger {
background-color: salmon; }
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> This can be simplified to .thinger { ... }
> 
> Also the class name could be improved by just saying what it does:
> backgroundSalmon.

Done.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:10: <h4>Row's before
margin was not reseting back to 0 when margin of the cell's child is changed
from 40px to 0.</h4>
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> s/40px/100px/

Done.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:14: <div>
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> Do we really need to use an anonymous table by using CSS instead of explicit
> <table> / <td> tags?

I also tried it with table and td tags. But I am not able to reproduce it.

https://codereview.chromium.org/22589002/diff/7001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-toggle-paragraph-padding.html:26:
document.querySelector('.toggle').style.margin = 0;
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> This really look like it could be a checkLayout() test instead of a much
slower
> ref-test.

Done.

https://codereview.chromium.org/22589002/diff/7001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/22589002/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:477: // position when row's
baseline would be calculated for the cell which introduced intrinsic padding for
other cells.
On 2013/08/08 22:15:51, Julien Chaffraix wrote:
> Let's try to make comment good English sentence. That means that words have an
> article in front of them (usually 'the').
> 
> // Ignoring the intrinsic padding as it depends on knowing the row's baseline,
> which won't be accurate
> // until the end of this function.

Done.

[Julien - ping for review, 2013-08-09 15:59:51]

lgtm

[commit-bot: I haz the power, 2013-08-09 18:06:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/22589002/15001

[commit-bot: I haz the power, 2013-08-09 20:33:00]

Change committed as 155864