New issue
Advanced search Search tips

Issue 875235 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 14
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 874753



Sign in to add a comment

[LayoutNG] Implement fieldset/legend

Project Member Reported by zcorpan@gmail.com, Aug 17

Issue description

Implement fieldset/legend in LayoutNG.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fffa35f66e61818a4c84119157711ecbea109739

commit fffa35f66e61818a4c84119157711ecbea109739
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue Sep 25 18:58:34 2018

[LayoutNG] Fieldset layout algorithm.

This implements the basics for a layout algorithm for the FIELDSET
element. It doesn't yet contain min/max calculation, legend inline
alignment, or block fragmentation support. Nor do we attempt to
paint/hit-test/etc anything correctly. Painting is likely to require
some rather persuasive code on the legacy side, especially if we want to
get paint layers right (for scrolling or e.g. relatively positioned
legends).

This new implementation is not enabled by default, but can be enabled
via the (new) LayoutNGFieldset runtime flag.

There has been some recent spec work for FIELDSET / LEGEND [1], and we
aim to accommodate for all of that. Among other things it's been decided
that a fieldset may be a flex or grid container. We are also expected to
support multicol and scrollable containers. None of these things are
supported by legacy layout.

The NG implementation is designed to handle all of that (although, with
this CL, it handles none of them). This is the reason why we need an
anonymous fieldset content wrapper child, which contains all the
fieldset contents. The rendered legend is placed on the outside of that.
This is both for convenience reasons and pure necessity.

Necessity: The legend is expected NOT to scroll together with the
contents if the fieldset is scrollable. That'd obviously look silly.

Conveniece: Taking the legend out from the fieldset contents means that
none of the other layout algorithms (block layout, multicol, flex, grid)
need to support legends in their own peculiar ways.

A fieldset element generates a fragment with up to two child fragments;
first the rendered legend (if any), then the fieldset content (if any).

Given this markup:

<fieldset>
  <legend>leder</legend>
  <div>hosen</div>
</fieldset

The fragment tree will look like this:

                     +--------------------+
                     | Fieldset container |
                     | (FIELDSET DOM node)|
                     +--------------------+
                       /                \
                      /                  \
   +--------------------------+   +-------------------+
   | Fieldset content wrapper |   | Rendered legend   |
   |         (anonymous)      |   | (LEGEND DOM node) |
   +--------------------------+   +-------------------+
                |                          |
                |                          |
             +-----+                   +-------+
             | DIV |                   | leder |
             +++++++                   +-------+
                |
                |
            +-------+
            | hosen |
            +-------+

The fieldset content wrapper can be a regular block container, or,
eventually, a scrollable container, a multicol container, a flex
container, or a grid container. Fieldset padding is applied on the
anonymous wrapper, NOT on the fieldset container. The reason for this is
that padding is on the inside of the scrollport (if any), so since the
wrapper establishes the scrollport (in order to exclude the legend; we
don't want it to scroll with the rest), the padding needs to go there as
well. At the same time, the legend needs to take fieldset padding into
consideration, so some extra code is required for this.

[1] https://github.com/whatwg/html/pull/3934

Bug:  875235 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: If952215459016e8528b2d94b74bca1c76e2fb4d6
Reviewed-on: https://chromium-review.googlesource.com/1236221
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594032}
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/html/forms/html_field_set_element.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/html/forms/html_field_set_element.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/BUILD.gn
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_box.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_box.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_fieldset.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_fieldset.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_object_factory.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/layout_object_factory.h
[add] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/layout_ng_fieldset.cc
[add] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/layout_ng_fieldset.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/legacy_layout_tree_walking.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/legacy_layout_tree_walking.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_block_node.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_block_node.h
[add] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm.cc
[add] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm.h
[add] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm_test.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.cc
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.h
[modify] https://crrev.com/fffa35f66e61818a4c84119157711ecbea109739/third_party/blink/renderer/platform/runtime_enabled_features.json5

Summary: [LayoutNG] Implement fieldset/legend (was: Implement fieldset/legend in LayoutNG)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09a02c72ed6997671d44654b4e6f26a06530d9a1

commit 09a02c72ed6997671d44654b4e6f26a06530d9a1
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Wed Sep 26 23:12:41 2018

[LayoutNG] Fieldset min/max intrinsic size calculation.

Extra care is needed when the legend is an orthogonal writing mode root.
We need to do what we do in min/max calculation for such children in the
regular block layout algorithm.

Fieldset padding is also special, since it's included in the fieldset
content anonymous child, but we have to take care of it ourselves
anyway, in case there is no such child (empty fieldset, or fieldset with
just a legend).

Bug:  875235 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: If26d8a59ae1514085f40e5a9e1ecee007997e7ad
Reviewed-on: https://chromium-review.googlesource.com/1245363
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594514}
[modify] https://crrev.com/09a02c72ed6997671d44654b4e6f26a06530d9a1/third_party/blink/renderer/core/layout/min_max_size.cc
[modify] https://crrev.com/09a02c72ed6997671d44654b4e6f26a06530d9a1/third_party/blink/renderer/core/layout/min_max_size.h
[modify] https://crrev.com/09a02c72ed6997671d44654b4e6f26a06530d9a1/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm.cc
[modify] https://crrev.com/09a02c72ed6997671d44654b4e6f26a06530d9a1/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm_test.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e9786b59f962e14750d2682e264401cef4f5feb

commit 6e9786b59f962e14750d2682e264401cef4f5feb
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Mon Oct 01 11:37:20 2018

[LayoutNG] Remove unfinished multicol+fieldset code.

Didn't intend to submit this.

TBR=cbiesinger@chromium.org

Bug:  875235 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Iaadb3d155e24d3b15e5ccd5586c052d316131540
Reviewed-on: https://chromium-review.googlesource.com/1253981
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595426}
[modify] https://crrev.com/6e9786b59f962e14750d2682e264401cef4f5feb/third_party/blink/renderer/core/layout/ng/layout_ng_fieldset.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fb3a4758ff68f3645139ef8d8031099fa2e0d28

commit 4fb3a4758ff68f3645139ef8d8031099fa2e0d28
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Thu Oct 04 13:09:35 2018

[LayoutNG] Support for fieldset painting and hit-testing.

Writing back to legacy layout is a bit bumpy, because legacy layout has
the rendered legend inside the anonymous fieldset content wrapper, while
in NG it's a direct child fragment of the fieldset container.

This solution uses NG painting of block children, which isn't really
ready yet. The NG paint code, when first entered, will attempt to
descend into everything that doesn't establish a new formatting context.
Luckily, both the fieldset container and the anonymous child wrapper
establish new formatting contexts, and we have to make sure to mark them
as such, to help the NG paint code fall back to legacy painting.
Introduced a new NGBoxType that is kMinimumBlockFormattingContextRoot or
greater, since none of the other existing types were right (this isn't
floated, atomic inline, OOF, etc.)

There's no support for self-painting layers established by legends. Most
likely better to wait for NG to support layers natively, rather than
hacking something up for legends. Similarly no support for fieldsets
that establish scrollable containers (also layers), which are supposed
to be delegated to the anonymous fieldset content wrapper, and the paint
code doesn't want scrollable containers to be established by anonymous
objects.

The new painting code is inspired by FieldsetPainter and
NGBoxFragmentPainter.

Added some virtual tests that enable NGFieldset. Rebaselined those that
only had irrelevant differences in render tree dumps.

Bug:  875235 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I49e9eab8bb4aa469c76a13f15bc16d63c80d5940
Reviewed-on: https://chromium-review.googlesource.com/c/1252581
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596639}
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/VirtualTestSuites
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/linux/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-legend-padding-unclipped-fieldset-border-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/linux/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-with-float-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/linux/virtual/layout_ng_experimental/fast/forms/fieldset/float-before-fieldset-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/mac/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-legend-padding-unclipped-fieldset-border-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/mac/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-with-float-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/mac/virtual/layout_ng_experimental/fast/forms/fieldset/float-before-fieldset-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/win/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-legend-padding-unclipped-fieldset-border-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/win/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-with-float-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/platform/win/virtual/layout_ng_experimental/fast/forms/fieldset/float-before-fieldset-expected.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/virtual/layout_ng_experimental/external/wpt/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/README.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/virtual/layout_ng_experimental/fast/forms/fieldset/README.txt
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/WebKit/LayoutTests/virtual/layout_ng_experimental/fast/forms/fieldset/fieldset-crash-expected.txt
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/layout_block.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/layout_block.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/layout_block_flow.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/geometry/ng_box_strut.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/geometry/ng_box_strut.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_block_node.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_fieldset_layout_algorithm.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_fragment_builder.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_fragment_builder.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_layout_result.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_physical_fragment.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/layout/ng/ng_physical_fragment.h
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/paint/BUILD.gn
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/paint/block_painter.cc
[modify] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/paint/ng/ng_fieldset_painter.cc
[add] https://crrev.com/4fb3a4758ff68f3645139ef8d8031099fa2e0d28/third_party/blink/renderer/core/paint/ng/ng_fieldset_painter.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9d93641214c95596d4c5c1b445ccfcd8a4468a7

commit d9d93641214c95596d4c5c1b445ccfcd8a4468a7
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Fri Oct 05 20:16:24 2018

[LayoutNG] Disable scrollable overflow for NG-fieldsets.

The paint code doesn't like anonymous scrollable containers, so just
disable non-visible overflow support for LayoutNGFieldset.

Bug:  875235 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ice03f866bd3e61d8b0b8137089c4de254c0b81f5
Reviewed-on: https://chromium-review.googlesource.com/c/1264661
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597266}
[modify] https://crrev.com/d9d93641214c95596d4c5c1b445ccfcd8a4468a7/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d9d93641214c95596d4c5c1b445ccfcd8a4468a7/third_party/blink/renderer/core/layout/ng/layout_ng_fieldset.cc

Status: Fixed (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2456278c66d02e2547c7e8a7a6c2a9f08dfb6d8a

commit 2456278c66d02e2547c7e8a7a6c2a9f08dfb6d8a
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Thu Jan 03 11:06:05 2019

Move layout_ng_experimental expectations lines around a bit.

Place them together with their friends. :)
And use appropriate bug numbers.

TBR=eae@chromium.org

Bug: 829028,  875235 ,  918663 
Change-Id: Ib8c5164747dc433e145984c869ce1fd19557686d
Reviewed-on: https://chromium-review.googlesource.com/c/1394305
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619616}
[modify] https://crrev.com/2456278c66d02e2547c7e8a7a6c2a9f08dfb6d8a/third_party/blink/web_tests/TestExpectations

Sign in to add a comment