Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(240)

Issue 2150003005: Add grid/flex layout support for <fieldset> (Closed)

Created:
4 years, 5 months ago by Gleb Lanbin
Modified:
4 years, 4 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, blink-reviews-layout_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, svillar, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add grid/flex layout support for <fieldset> This adds grid/flex layout support for <fieldset> by introducing an anonymous inner block that is used to layout fieldset's underlying blocks without interfering with the special paint flow used to render <legend> element. W3C/CSS WG discussion: https://github.com/w3c/csswg-drafts/issues/321 Testing: - All affected tests' results were verified to make sure that there are no differences in pixel test results between old and new version of LayoutFieldSet. - 2 more tests were added (fieldset-display-{flex|grid}.html) - verified interoperability of fieldset(display: flex) with Firefox(one of 2 browsers that support fieldset's flex layout) BUG=262679, 375693 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html Committed: https://crrev.com/c6d69f896f406c9a7801b29cb8c02a88e5b01770 Cr-Commit-Position: refs/heads/master@{#409303}

Patch Set 1 : patch with failed tests #

Patch Set 2 : updated TestExpectations and fixed legend-after-margin-vertical-writing-mode.html #

Total comments: 48

Patch Set 3 : fixed comments #

Patch Set 4 : fixed comments: removed the call to 'setNeedsLayoutAndFullPaintInvalidation', added '<!DOCTYPE html… #

Total comments: 6

Patch Set 5 : fixed comments: updated addChild() method #

Total comments: 6

Patch Set 6 : fixed comments and accessibility tests #

Patch Set 7 : synced to the head #

Total comments: 2

Patch Set 8 : revert a11y test expectations & explicitely set a11y fieldset role to GroupRole #

Total comments: 2

Patch Set 9 : explicitly call AXObjectCache::childrenChanged on addChild #

Patch Set 10 : put the legend first #

Patch Set 11 : Add grid/flex layout support for <fieldset> #

Unified diffs Side-by-side diffs Delta from patch set Stats (+456 lines, -39 lines) Patch
A third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html View 1 2 3 4 5 1 chunk +107 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex-expected.html View 1 2 3 4 5 1 chunk +105 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html View 1 2 3 1 chunk +23 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid-expected.html View 1 2 3 1 chunk +23 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlock.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp View 1 2 3 4 5 6 7 8 9 10 4 chunks +2 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFieldset.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +20 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFieldset.cpp View 1 2 3 4 5 6 7 8 9 10 5 chunks +138 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp View 1 2 3 4 5 6 7 8 9 10 7 chunks +17 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutObject.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 84 (49 generated)
Gleb Lanbin
4 years, 5 months ago (2016-07-19 21:25:30 UTC) #7
eae
This looks great but cbiesinger would be a better reviewer.
4 years, 5 months ago (2016-07-19 21:27:32 UTC) #8
eae
4 years, 5 months ago (2016-07-19 21:27:42 UTC) #10
cbiesinger
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp#newcode1220 third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1220: LayoutObject* childToExclude = layoutSpecialExcludedChild(relayoutChildren, layoutScope); Thanks for doing this! ...
4 years, 5 months ago (2016-07-20 01:42:31 UTC) #11
Manuel Rego
As explained in the comments I don't like the change. In any case the description ...
4 years, 5 months ago (2016-07-20 06:00:23 UTC) #13
jfernandez
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode38 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) What's the rationale of inheriting from ...
4 years, 5 months ago (2016-07-20 14:59:57 UTC) #15
cbiesinger
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode38 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) On 2016/07/20 14:59:57, jfernandez wrote: > ...
4 years, 5 months ago (2016-07-20 20:30:50 UTC) #16
Manuel Rego
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode38 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) On 2016/07/20 20:30:49, cbiesinger wrote: > ...
4 years, 5 months ago (2016-07-21 10:28:43 UTC) #17
cbiesinger
The reason it uses flexbox is for correct sizing of the anonymous inner block -- ...
4 years, 5 months ago (2016-07-21 18:14:14 UTC) #18
Gleb Lanbin
thanks for your comments. Please take another look. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html#newcode3 third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:3: <head> ...
4 years, 5 months ago (2016-07-21 18:25:32 UTC) #21
cbiesinger
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html#newcode1 third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:1: <!DOCTYPE html> You should keep this line; we don't ...
4 years, 5 months ago (2016-07-21 19:54:17 UTC) #22
Gleb Lanbin
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html#newcode1 third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:1: <!DOCTYPE html> On 2016/07/21 19:54:16, cbiesinger wrote: > You ...
4 years, 5 months ago (2016-07-21 20:49:02 UTC) #23
cbiesinger
https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/LayoutTests/TestExpectations#newcode15 third_party/WebKit/LayoutTests/TestExpectations:15: crbug.com/262679 accessibility/computed-role.html [ NeedsRebaseline ] To double check, did ...
4 years, 5 months ago (2016-07-21 21:17:51 UTC) #24
Gleb Lanbin
https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/LayoutTests/TestExpectations#newcode15 third_party/WebKit/LayoutTests/TestExpectations:15: crbug.com/262679 accessibility/computed-role.html [ NeedsRebaseline ] On 2016/07/21 21:17:51, cbiesinger ...
4 years, 5 months ago (2016-07-21 21:25:20 UTC) #25
cbiesinger
lgtm https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode224 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:224: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); Similar to my question about remove, is ...
4 years, 5 months ago (2016-07-21 21:42:42 UTC) #26
Gleb Lanbin
thanks for the review. https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode224 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:224: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); On 2016/07/21 21:42:42, cbiesinger ...
4 years, 5 months ago (2016-07-21 23:05:09 UTC) #28
Manuel Rego
On 2016/07/21 18:14:14, cbiesinger wrote: > The reason it uses flexbox is for correct sizing ...
4 years, 5 months ago (2016-07-22 06:16:07 UTC) #33
cbiesinger
I tried using fill-available for <button>: https://codereview.chromium.org/2177563003/ There are some minor positioning differences that could ...
4 years, 5 months ago (2016-07-22 22:03:30 UTC) #38
Gleb Lanbin
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode253 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:253: { On 2016/07/22 06:16:06, Manuel Rego wrote: > On ...
4 years, 5 months ago (2016-07-22 22:16:15 UTC) #41
Gleb Lanbin
Alice, could you review accessibility changes. thank you.
4 years, 5 months ago (2016-07-23 04:36:27 UTC) #50
Gleb Lanbin
Alice, just a friendly reminder that I'm waiting for your review.
4 years, 4 months ago (2016-07-26 17:08:59 UTC) #52
aboxhall
https://codereview.chromium.org/2150003005/diff/190001/content/browser/accessibility/accessibility_win_browsertest.cc File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/2150003005/diff/190001/content/browser/accessibility/accessibility_win_browsertest.cc#newcode977 content/browser/accessibility/accessibility_win_browsertest.cc:977: AccessibleChecker grouping1_checker(std::wstring(), ROLE_SYSTEM_CLIENT, I'm guessing this is a result ...
4 years, 4 months ago (2016-07-27 17:21:50 UTC) #53
aboxhall
4 years, 4 months ago (2016-07-27 17:21:54 UTC) #54
aboxhall
https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode225 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:225: m_innerBlock->addChild(newChild, beforeChild); I think you may need to explicitly ...
4 years, 4 months ago (2016-07-27 21:44:45 UTC) #55
aboxhall
lgtm
4 years, 4 months ago (2016-07-27 21:48:20 UTC) #56
Gleb Lanbin
Alice, thanks for the review! https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp#newcode225 third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:225: m_innerBlock->addChild(newChild, beforeChild); On 2016/07/27 ...
4 years, 4 months ago (2016-07-27 21:55:08 UTC) #57
Gleb Lanbin
4 years, 4 months ago (2016-07-29 02:42:20 UTC) #71
Gleb Lanbin
On 2016/07/29 02:42:20, glebl wrote: Dominic, just a friendly reminder that I'm waiting for your ...
4 years, 4 months ago (2016-08-01 18:50:36 UTC) #72
dmazzoni
lgtm
4 years, 4 months ago (2016-08-02 15:58:59 UTC) #73
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2150003005/290001
4 years, 4 months ago (2016-08-02 16:20:14 UTC) #76
commit-bot: I haz the power
Committed patchset #10 (id:290001)
4 years, 4 months ago (2016-08-02 20:22:55 UTC) #78
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/c6d69f896f406c9a7801b29cb8c02a88e5b01770 Cr-Commit-Position: refs/heads/master@{#409303}
4 years, 4 months ago (2016-08-02 20:26:28 UTC) #80
inferno
A revert of this CL (patchset #10 id:290001) has been created in https://codereview.chromium.org/2206793004/ by inferno@chromium.org. ...
4 years, 4 months ago (2016-08-03 20:28:39 UTC) #81
mstensho (USE GERRIT)
Although this got reverted, I have to ask: Why all this, instead of just deferring ...
4 years, 4 months ago (2016-08-08 10:45:41 UTC) #83
Gleb Lanbin
4 years, 4 months ago (2016-08-08 15:26:15 UTC) #84
Message was sent while issue was closed.
On 2016/08/08 10:45:41, mstensho wrote:
> Although this got reverted, I have to ask: Why all this, instead of just
> deferring to regular layout object creation if the it's not going to be a
block
> container? I.e. just call LayoutObject::createObject() if "display" isn't
> "block" or "inline-block". Also: what about display:table?
> 
> Judging from the tests, you don't seem to care about LEGEND handling for
> flex/grid, so why does it have to be a LayoutFieldset at all for such display
> types?

the new version of this patch with fixed ClusterFuzz tests can be found here
http://crrev.com/2215133005. Please use that new issue for all other questions.
we do care about <legend> while displaying <fieldset> as grid/flex. The only
reason it's not included in the tests because we prefer html tests over pixel
tests and because <legend> layout is widely covered in other tests.
If you think we need some pixel tests to demonstrate that <legend> works fine
with flex/grid then please add a comment at http://crrev.com/2215133005 and I
will add them.

Powered by Google App Engine
This is Rietveld 408576698