Don't update column position in StyleDidChange.

Don't update column position in StyleDidChange.

StyleDidChange set the first column position to the horizontal border
spacing value regardless of whether this value changed or not.

I am not familiar with how table layout works in Blink and haven't
debugged this extensively, but when we re-layout a table cell because
it has out-of-flow content which needs layout, the first effective
column position is set wrongly. It gets its initial value from the line
removed in this CL by a style recalc on the table element prior to the
re-layout, which is the h_spacing_, but the spacing is subtracted once
more in http://bit.ly/2pBB7x1

R=eae@chromium.org,mstensho@opera.com
BUG=716006
Review-Url: https://codereview.chromium.org/2855853002
Cr-Commit-Position: refs/heads/master@{#468920}
Committed: https://chromium.googlesource.com/chromium/src/+/d3c625b0b03346472e45dc1fda33d35b1d40efd3

[rune, 2017-05-02 13:53:21]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 13:53:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rune, 2017-05-02 14:36:39]

ptal

[commit-bot: I haz the power, 2017-05-02 15:43:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 15:43:49]

Dry run: This issue passed the CQ dry run.

[eae, 2017-05-02 15:58:32]

eae@chromium.org changed reviewers:
+ dgrogan@chromium.org

[eae, 2017-05-02 15:58:32]

This seems reasonable to me but I'd like dgrogan to take a look.

Thanks!

[dgrogan, 2017-05-03 03:07:59]

lgtm

I think the code change is right but the description is a little off: the
spacing is _subtracted_ twice in the code highlighted in the bit.ly, causing the
cell to be assigned incorrect width -98 instead of correct width 2. Though I
didn't fully trace through to see how that caused the affected 'X' to end up to
the right of its correct location.

The erroneous line was present in webkit's svn r1.

[rune, 2017-05-03 08:50:12]

Description was changed from

==========
Don't update column position in StyleDidChange.

StyleDidChange set the first column position to the horizontal border
spacing value regardless of whether this value changed or not.

I am not familiar with how table layout works in Blink and haven't
debugged this extensively, but when we re-layout a table cell because
it has out-of-flow content which needs layout, the first effective
column position is set wrongly. It gets its initial value from the line
removed in this CL by a style recalc on the table element prior to the
re-layout, which is the h_spacing_, but the spacing is added once more
in http://bit.ly/2pBB7x1

R=eae@chromium.org,mstensho@opera.com
BUG=716006
==========

to

==========
Don't update column position in StyleDidChange.

StyleDidChange set the first column position to the horizontal border
spacing value regardless of whether this value changed or not.

I am not familiar with how table layout works in Blink and haven't
debugged this extensively, but when we re-layout a table cell because
it has out-of-flow content which needs layout, the first effective
column position is set wrongly. It gets its initial value from the line
removed in this CL by a style recalc on the table element prior to the
re-layout, which is the h_spacing_, but the spacing is subtracted once
more in http://bit.ly/2pBB7x1

R=eae@chromium.org,mstensho@opera.com
BUG=716006
==========

[rune, 2017-05-03 08:50:27]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-05-03 08:50:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 08:54:03]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493801427559900, "parent_rev":
"1c49164e024e59ded9ce1bcb84f376d4b47bdd93", "commit_rev":
"d3c625b0b03346472e45dc1fda33d35b1d40efd3"}

[commit-bot: I haz the power, 2017-05-03 08:54:14]

Description was changed from

==========
Don't update column position in StyleDidChange.

StyleDidChange set the first column position to the horizontal border
spacing value regardless of whether this value changed or not.

I am not familiar with how table layout works in Blink and haven't
debugged this extensively, but when we re-layout a table cell because
it has out-of-flow content which needs layout, the first effective
column position is set wrongly. It gets its initial value from the line
removed in this CL by a style recalc on the table element prior to the
re-layout, which is the h_spacing_, but the spacing is subtracted once
more in http://bit.ly/2pBB7x1

R=eae@chromium.org,mstensho@opera.com
BUG=716006
==========

to

==========
Don't update column position in StyleDidChange.

StyleDidChange set the first column position to the horizontal border
spacing value regardless of whether this value changed or not.

I am not familiar with how table layout works in Blink and haven't
debugged this extensively, but when we re-layout a table cell because
it has out-of-flow content which needs layout, the first effective
column position is set wrongly. It gets its initial value from the line
removed in this CL by a style recalc on the table element prior to the
re-layout, which is the h_spacing_, but the spacing is subtracted once
more in http://bit.ly/2pBB7x1

R=eae@chromium.org,mstensho@opera.com
BUG=716006

Review-Url: https://codereview.chromium.org/2855853002
Cr-Commit-Position: refs/heads/master@{#468920}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3c625b0b03346472e45dc1fda33...
==========

[commit-bot: I haz the power, 2017-05-03 08:54:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/d3c625b0b03346472e45dc1fda33...

[mstensho (USE GERRIT), 2017-05-04 13:37:52]

lgtm

https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (left):

https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:84: h_spacing_ =
ShouldCollapseBorders() ? 0 : Style()->HorizontalBorderSpacing();
Furthermore, why are we updating h_spacing_ and v_spacing_ at this point? Can't
it wait until UpdateLayout()?

[rune, 2017-05-04 20:32:03]

https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (left):

https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:84: h_spacing_ =
ShouldCollapseBorders() ? 0 : Style()->HorizontalBorderSpacing();
On 2017/05/04 13:37:52, mstensho wrote:
> Furthermore, why are we updating h_spacing_ and v_spacing_ at this point?
Can't
> it wait until UpdateLayout()?

That sounds like a good idea, assuming LayoutTable::UpdateLayout is always
called when the new h/v_border_spacing_ needs to be updated.

[rune, 2017-05-05 08:19:07]

On 2017/05/04 20:32:03, rune wrote:
>
https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutTable.cpp (left):
> 
>
https://codereview.chromium.org/2855853002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutTable.cpp:84: h_spacing_ =
> ShouldCollapseBorders() ? 0 : Style()->HorizontalBorderSpacing();
> On 2017/05/04 13:37:52, mstensho wrote:
> > Furthermore, why are we updating h_spacing_ and v_spacing_ at this point?
> Can't
> > it wait until UpdateLayout()?
> 
> That sounds like a good idea, assuming LayoutTable::UpdateLayout is always
> called when the new h/v_border_spacing_ needs to be updated.

We looked at this off-line, and for nested tables we use these local variables
to calculate intrinsic widths before UpdateLayout is called on the inner table.
So this cannot easily be changed.

[dgrogan, 2017-05-05 15:32:00]

Thanks for circling back.