[table] Stretching tables when needed due to self-alignment properties

[table] Stretching tables when needed due to self-alignment properties

The Self-Alignment properties, align-self and justify-self, apply to
tables when they are flex or grid items.

This patch addresses only the issue of applying the stretch value, which
effectively affects the size of the table in those cases.

The justify-self applies also to tables when they are block-level
boxes, but the alignment feature is still not implemented for that
case, so for now the related test cases expect no stretching.

Other values of alignment aren't implemented yet for tables, but those
will be part of future CLs, once somebody files the appropriated bug.

BUG=667785
Review-Url: https://codereview.chromium.org/2528253003
Cr-Commit-Position: refs/heads/master@{#454743}
Committed: https://chromium.googlesource.com/chromium/src/+/a90dba1f2dd094e51229dd2e5e059c6b53cee93a

[jfernandez, 2016-11-28 12:13:09]

Description was changed from

==========
[table] Stretching tables when needed due to self-alignment properties

The Self-Alignment properties, align-self and justify-self, apply to
tables when they are flex or grid items.

This patch addresses only the issue of applying the stretch value, which
effectively affects the size of the table in those cases.

The justify-self applies also to tables when they are block-level
boxes, but the alignment feature is still not implemented for that
case, so for now the related test cases expect no stretching.

Other values of alignment aren't implemented yet for tables, but those
will be part of future CLs, once somebody files the appropriated bug.

BUG=667785
==========

to

==========
[table] Stretching tables when needed due to self-alignment properties

The Self-Alignment properties, align-self and justify-self, apply to
tables when they are flex or grid items.

This patch addresses only the issue of applying the stretch value, which
effectively affects the size of the table in those cases.

The justify-self applies also to tables when they are block-level
boxes, but the alignment feature is still not implemented for that
case, so for now the related test cases expect no stretching.

Other values of alignment aren't implemented yet for tables, but those
will be part of future CLs, once somebody files the appropriated bug.

BUG=667785
==========

[jfernandez, 2016-11-28 12:13:09]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, dgrogan@chromium.org, rego@igalia.com,
svillar@igalia.com

[jfernandez, 2016-11-28 12:15:12]

Sent for review.

[mstensho (USE GERRIT), 2016-11-28 12:54:30]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-11-28 12:54:31]

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html
(right):

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:26:
border-spacing: 0px;
border-spacing doesn't apply to table cells.

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

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:302: ?
availableContentLogicalWidth
Skipping maxPreferredLogicalWidth() here seems wrong. The comment right below
says that it needs to be called prior to calling
scaledWidthFromPercentColumns().

I'm pretty sure stretching needs to take place as a separate step, rather than
in the middle of max preferred width handling.

[jfernandez, 2016-11-29 16:03:33]

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html
(right):

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:26:
border-spacing: 0px;
On 2016/11/28 12:54:31, mstensho wrote:
> border-spacing doesn't apply to table cells.

Done.

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

https://codereview.chromium.org/2528253003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:302: ?
availableContentLogicalWidth
On 2016/11/28 12:54:31, mstensho wrote:
> Skipping maxPreferredLogicalWidth() here seems wrong. The comment right below
> says that it needs to be called prior to calling
> scaledWidthFromPercentColumns().
> 
> I'm pretty sure stretching needs to take place as a separate step, rather than
> in the middle of max preferred width handling.

Done.

[dgrogan, 2016-11-30 01:03:22]

https://codereview.chromium.org/2528253003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html
(right):

https://codereview.chromium.org/2528253003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:33:
<p>Regular block container of a table element and 1 implicit row and
column.<br>The align-self property doesn't apply to block-level boxes.</br>The
justify-self propery has its initial/default value 'normal', which behaves like
'start'.</p>
s/propery/property/ and 2 other places

https://codereview.chromium.org/2528253003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2528253003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:309: if
(hasStretchedLogicalWidth()) {
I don't know if this is the right behavior or not. (I don't know anything about
flex/grid.) But if it is correct then it looks like you can totally avoid the
calls to maxPreferredLogicalWidth() and scaledWidthFromPercentColumns() in the
hasStretchedLogicalWidth() case?

[jfernandez, 2017-01-13 12:45:56]

New patch, with the suggested changes, sent out for review.

[jfernandez, 2017-01-13 16:09:42]

Patchset #3 (id:40001) has been deleted

[mstensho (USE GERRIT), 2017-02-22 13:37:04]

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html
(right):

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:38:
<body onload="checkLayout('.block')">
Probably no need for the onload handler. How about just inserting a <script> at
the end of the document that does checkLayout('.block') instead?

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:143:
</html>
No HTML start tag found. We usually avoid explicit HTML and BODY elements in our
tests.

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (left):

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1320: virtual ItemPosition
selfAlignmentNormalBehavior() const {
Is this about resolving align-self:auto? This method deserves some
documentation.

[jfernandez, 2017-03-02 11:12:58]

Submitted a new patch for review, PTAL.

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html
(right):

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:38:
<body onload="checkLayout('.block')">
On 2017/02/22 13:37:04, mstensho wrote:
> Probably no need for the onload handler. How about just inserting a <script>
at
> the end of the document that does checkLayout('.block') instead?

Done.

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-must-consider-stretch-alignment.html:143:
</html>
On 2017/02/22 13:37:04, mstensho wrote:
> No HTML start tag found. We usually avoid explicit HTML and BODY elements in
our
> tests.

Done.

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (left):

https://codereview.chromium.org/2528253003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1320: virtual ItemPosition
selfAlignmentNormalBehavior() const {
On 2017/02/22 13:37:04, mstensho wrote:
> Is this about resolving align-self:auto? This method deserves some
> documentation.

Well, not exactly. It's a virtual method to define the 'actual
behavior' of a new keyword that CSS Alignment spec editor added
to deal with the different alignment default behavior depending
on the layout model (flex, grid, regular blocks or absolute 
positioned). 

I think the name of the function is self-explanatory, once you
realize about the existence of this new 'normal' keyword.

[mstensho (USE GERRIT), 2017-03-02 12:26:01]

lgtm

https://codereview.chromium.org/2528253003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2528253003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:639: LayoutUnit
currentAvailableLogicalHeight =
In the future, please push separate patch sets for rebasing and addressing
review issues.

[jfernandez, 2017-03-02 12:36:58]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2017-03-02 12:37:14]

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-03-02 13:34:10]

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

[commit-bot: I haz the power, 2017-03-02 13:34:11]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jfernandez, 2017-03-04 00:19:36]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-03-04 00:19:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2528253003/#ps120001
(title: "Removed another incorrect assert.")

[commit-bot: I haz the power, 2017-03-04 00:19:58]

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-03-04 01:36:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488586776145960,
"parent_rev": "a9d4fa4f3133c0fbd0a48d01846ec8968cfe2926", "commit_rev":
"a90dba1f2dd094e51229dd2e5e059c6b53cee93a"}

[commit-bot: I haz the power, 2017-03-04 01:37:38]

Description was changed from

==========
[table] Stretching tables when needed due to self-alignment properties

The Self-Alignment properties, align-self and justify-self, apply to
tables when they are flex or grid items.

This patch addresses only the issue of applying the stretch value, which
effectively affects the size of the table in those cases.

The justify-self applies also to tables when they are block-level
boxes, but the alignment feature is still not implemented for that
case, so for now the related test cases expect no stretching.

Other values of alignment aren't implemented yet for tables, but those
will be part of future CLs, once somebody files the appropriated bug.

BUG=667785
==========

to

==========
[table] Stretching tables when needed due to self-alignment properties

The Self-Alignment properties, align-self and justify-self, apply to
tables when they are flex or grid items.

This patch addresses only the issue of applying the stretch value, which
effectively affects the size of the table in those cases.

The justify-self applies also to tables when they are block-level
boxes, but the alignment feature is still not implemented for that
case, so for now the related test cases expect no stretching.

Other values of alignment aren't implemented yet for tables, but those
will be part of future CLs, once somebody files the appropriated bug.

BUG=667785

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

[commit-bot: I haz the power, 2017-03-04 01:37:39]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a90dba1f2dd094e51229dd2e5e05...

[jfernandez, 2017-03-09 09:31:43]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/2740063003/ by jfernandez@igalia.com.

The reason for reverting is: This patch is causing several important
regressions..