Add grid/flex layout support for <fieldset>

third_party/WebKit/Source/core/layout/LayoutFieldset.cpp

Index: third_party/WebKit/Source/core/layout/LayoutFieldset.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp b/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp
index 1af7515adb104d99aa17dda22498d3289939bd56..6797f89a7f32e13395f5aa75b015a9f9d5ee0952 100644
--- a/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp
@@ -35,26 +35,57 @@ namespace blink {
 using namespace HTMLNames;
 
 LayoutFieldset::LayoutFieldset(Element* element)
-    : LayoutBlockFlow(element)
+    : LayoutFlexibleBox(element), m_innerBlock(nullptr)

[jfernandez, 2016/07/20 14:59:57]

What's the rationale of inheriting from Flexbox ?

[cbiesinger, 2016/07/20 20:30:49]

LayoutButton inherits from flexbox for the same purpose. The main reason is so
that its inner block will automatically size to the same size as the button.

Do you have a better suggestion for how to do this?

[Manuel Rego, 2016/07/21 10:28:43]

What are the actual reasons Flexbox will be needed for this case?
I might be missing some cases but I've seen so far:
* <fieldset> behaves like a regular element taking all
  the available width and adapting its height to the content.
* If the width is smaller than any of the <fieldset> children
  (<legend> or other) the width is forced to be the min-content width
  of the children (somehow similar to "min-width: auto;" on flex items).

Also, current LayoutFieldset doesn't use Flexbox and it's been working
fine so far.

I understand that we need an anonymous element now,
in order to allow the <fieldset> to be a Flexbox or Grid,
but I still don't get why the <fieldset> has to always be a flexbox.

Wouldn't be possible to do the required things manually
in LayoutFieldset (like we're doing now)?
And of course add the anonymous element with the required type inside.
So the layout tree would be something like:
* LayoutFieldset
  * <legend>
  * Anonymous (block, flexbox or grid)
    * children


BTW, just playing a little bit more with the patch. E.g.:
  <fieldset style="display: flex;">
    <div style="background: magenta;">i1</div>
    <div style="background: cyan;">i2</div>
    <div style="background: yellow;">i3</div>
  </fieldset>

If I add "flex-direction: column;" to the <fieldset> I don't see
any change. The same happens if I use "justify-content: space-around;".
I guess that's not the expected behavior.

[Gleb Lanbin, 2016/07/21 18:25:31]

The anonymous block needs to use all available height/width. So looking into
other examples(e.g. LayoutButton) this has been changed to use
LayoutFlexibleBox. If there is another better way to achieve that with
LayoutBlockFlow please let me know.

re: flex-direction, justify-content etc.
cbiseinger@ commented about it below. fixed.

 {
 }
 
-void LayoutFieldset::computePreferredLogicalWidths()
+int LayoutFieldset::baselinePosition(FontBaseline baseline, bool firstLine, LineDirectionMode direction, LinePositionMode position) const
 {
-    LayoutBlockFlow::computePreferredLogicalWidths();
+    return LayoutBlock::baselinePosition(baseline, firstLine, direction, position);

[jfernandez, 2016/07/20 14:59:57]

So you want to avoid the LayoutFlexibleBox::baselinePosition to be called ? It's
a strange way to manage inheritance, since you are overriding a virtual method
but not implementing any specific behavior.

[cbiesinger, 2016/07/20 20:30:49]

flexbox has a different baseline behavior which most elements don't want :(

+}
+
+void LayoutFieldset::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
+{
+    if (m_innerBlock)
+        computeChildPreferredLogicalWidths(*m_innerBlock, minLogicalWidth, maxLogicalWidth);
+
     if (LayoutBox* legend = findInFlowLegend()) {
-        int legendMinWidth = legend->minPreferredLogicalWidth();
+        LayoutUnit minPreferredLegendLogicalWidth;
+        LayoutUnit maxPreferredLegendLogicalWidth;
+        computeChildPreferredLogicalWidths(*legend, minPreferredLegendLogicalWidth, maxPreferredLegendLogicalWidth);
+        LayoutUnit margin = marginIntrinsicLogicalWidthForChild(*legend);
+        minPreferredLegendLogicalWidth += margin;
+        maxPreferredLegendLogicalWidth += margin;
+        minLogicalWidth = std::max(minLogicalWidth, minPreferredLegendLogicalWidth);
+        maxLogicalWidth = std::max(maxLogicalWidth, maxPreferredLegendLogicalWidth);
+    }
+    maxLogicalWidth = std::max(minLogicalWidth, maxLogicalWidth);
 
-        Length legendMarginLeft = legend->style()->marginLeft();
-        Length legendMarginRight = legend->style()->marginRight();
+    // Due to negative margins, it is possible that we calculated a negative intrinsic width. Make sure that we
+    // never return a negative width.
+    minLogicalWidth = std::max(LayoutUnit(), minLogicalWidth);
+    maxLogicalWidth = std::max(LayoutUnit(), maxLogicalWidth);
 
-        if (legendMarginLeft.isFixed())
-            legendMinWidth += legendMarginLeft.value();
+    LayoutUnit scrollbarWidth(scrollbarLogicalWidth());
+    maxLogicalWidth += scrollbarWidth;
+    minLogicalWidth += scrollbarWidth;
+}
 
-        if (legendMarginRight.isFixed())
-            legendMinWidth += legendMarginRight.value();
+void LayoutFieldset::setLogicalLeftForChild(LayoutBox& child, LayoutUnit logicalLeft)
+{
+    if (isHorizontalWritingMode()) {
+        child.setX(logicalLeft);
+    } else {
+        child.setY(logicalLeft);
+    }
+}
 
-        m_minPreferredLogicalWidth = max(m_minPreferredLogicalWidth, legendMinWidth + borderAndPaddingWidth());
+void LayoutFieldset::setLogicalTopForChild(LayoutBox& child, LayoutUnit logicalTop)
+{
+    if (isHorizontalWritingMode()) {
+        child.setY(logicalTop);
+    } else {
+        child.setX(logicalTop);
     }
 }
 
@@ -106,6 +137,7 @@ LayoutObject* LayoutFieldset::layoutSpecialExcludedChild(bool relayoutChildren,
 
         LayoutUnit legendLogicalTop;
         LayoutUnit collapsedLegendExtent;
+        LayoutUnit innerBlockPadding;
         // FIXME: We need to account for the legend's margin before too.
         if (fieldsetBorderBefore > legendLogicalHeight) {
             // The <legend> is smaller than the associated fieldset before border
@@ -114,8 +146,16 @@ LayoutObject* LayoutFieldset::layoutSpecialExcludedChild(bool relayoutChildren,
             // Firefox completely ignores the margins in this case which seems wrong.
             legendLogicalTop = (fieldsetBorderBefore - legendLogicalHeight) / 2;
             collapsedLegendExtent = max<LayoutUnit>(fieldsetBorderBefore, legendLogicalTop + legendLogicalHeight + marginAfterForChild(*legend));
+            innerBlockPadding = marginAfterForChild(*legend) ? marginAfterForChild(*legend) - legendLogicalTop : LayoutUnit();
         } else {
             collapsedLegendExtent = legendLogicalHeight + marginAfterForChild(*legend);
+            innerBlockPadding = legendLogicalHeight - borderAfter() + marginAfterForChild(*legend);
+        }
+
+        if (isHorizontalWritingMode()) {
+            m_innerBlock->mutableStyleRef().setPaddingTop(Length(innerBlockPadding, Fixed));
+        } else {
+            m_innerBlock->mutableStyleRef().setPaddingLeft(Length(innerBlockPadding, Fixed));
         }
 
         setLogicalTopForChild(*legend, legendLogicalTop);
@@ -153,4 +193,73 @@ void LayoutFieldset::paintMask(const PaintInfo& paintInfo, const LayoutPoint& pa
     FieldsetPainter(*this).paintMask(paintInfo, paintOffset);
 }
 
+void LayoutFieldset::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
+{
+    LayoutFlexibleBox::styleDidChange(diff, oldStyle);
+    adjustInnerStyle();
+}
+
+void LayoutFieldset::updateLogicalWidth()
+{
+    LayoutFlexibleBox::updateLogicalWidth();
+    LogicalExtentComputedValues computedValues;
+    computeLogicalWidth(computedValues);
+
+    ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef();

[jfernandez, 2016/07/20 14:59:57]

Altering the ComputedStyle during layout is not a very good idea. I'm aware
there is already similar logic, but we are trying to avoid that in new code.

[cbiesinger, 2016/07/20 20:30:49]

Yeah, I don't understand why this is necessary? Doesn't the
flex-grow/flex-shrink take care of this?

[Gleb Lanbin, 2016/07/21 18:25:31]

done, removed

I needed it to pass
//src/third_party/WebKit/LayoutTests/imported/wpt/html/rendering/non-replaced-elements/the-fieldset-element-0/min-width-not-important.html

you're right, it looks flex-grow/flex-shrink take care of this. thanks

+    if (style()->isHorizontalWritingMode())
+        innerStyle.setWidth(Length(computedValues.m_extent, Fixed));
+    else
+        innerStyle.setHeight(Length(computedValues.m_extent, Fixed));
+}
+
+void LayoutFieldset::addChild(LayoutObject* newChild, LayoutObject* beforeChild)
+{
+    if (!m_innerBlock)
+        createInnerBlock();
+
+    if (isHTMLLegendElement(newChild->node())) {
+        LayoutFlexibleBox::addChild(newChild);
+        setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged);
+    } else {
+        m_innerBlock->addChild(newChild, beforeChild);
+        DCHECK(m_innerBlock == firstChild());
+    }
+
+    if (AXObjectCache* cache = document().existingAXObjectCache())

[cbiesinger, 2016/07/20 20:30:49]

Hm, why is this necessary? We don't call it in other addChild implementations?

[Gleb Lanbin, 2016/07/21 18:25:31]

done.

I copied it from LayoutMenuList as I was under impression that we need to call
this method to make the accessibility object cache happy.

[cbiesinger, 2016/07/21 19:54:16]

Interesting. Looking at some archaeology:
https://chromium.googlesource.com/chromium/src/+/04fcd9c130c9211216da5a1418f0...
https://bugs.webkit.org/show_bug.cgi?id=87028

Based on that I suspect this was menulist specific.

+        cache->childrenChanged(this);
+}
+
+void LayoutFieldset::adjustInnerStyle()
+{
+    if (!m_innerBlock)
+        createInnerBlock();
+
+    ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef();

[jfernandez, 2016/07/20 14:59:57]

Ditto.

[Gleb Lanbin, 2016/07/21 18:25:31]

this is similar to LayoutButton::updateAnonymousChildStyle
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

+    innerStyle.setFlexShrink(1);

[cbiesinger, 2016/07/20 20:30:49]

Use 1.0f, since these are floats, not ints.

[Gleb Lanbin, 2016/07/21 18:25:31]

Done.

+    innerStyle.setFlexGrow(1);

[cbiesinger, 2016/07/20 20:30:49]

I think you'll want to setMinWidth to Length(0, Fixed), also. Compare
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

On which note, shouldn't you transfer the various flex properties also, like
that function does?

[Gleb Lanbin, 2016/07/21 18:25:31]

Done.

+}
+
+void LayoutFieldset::createInnerBlock()
+{
+    if (m_innerBlock) {
+        DCHECK(firstChild() == m_innerBlock);
+        return;
+    }
+    m_innerBlock = createAnonymousBlock(style()->display());
+    LayoutFlexibleBox::addChild(m_innerBlock);
+}
+
+void LayoutFieldset::removeChild(LayoutObject* oldChild)
+{

[Manuel Rego, 2016/07/20 06:00:23]

I didn't review all the changes in this file,
but this kind of things seem really scary to me.
We've had bad experiences in the past while developing Grid Layout,
due to this kind of stuff.

I've just played a little bit with some examples and found 2 issues:
* If you remove the legend, the fieldset is not properly relayout:
  http://output.jsbin.com/qolohi/1
* Removing all the children of a filedset I got a crash:
  http://output.jsbin.com/tiroqom

[Gleb Lanbin, 2016/07/21 18:25:31]

done. 

after submitting this patch for review I found the same issues that you found
and already fixed them. thanks

[Manuel Rego, 2016/07/22 06:16:06]

Shouldn't we add tests to check that this kind of things are
working as expected?

Also some texts with a more complex flexbox using other properties
like "flex-direction" and/or "justify-content" and verify that
it's working as expected.

[Gleb Lanbin, 2016/07/22 22:16:15]

we already have test to check this
src/third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-disabled.html
that's how I found out that this functionality is broken.
done.

+    if (isHTMLLegendElement(oldChild->node())) {
+        LayoutFlexibleBox::removeChild(oldChild);
+        setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged);

[cbiesinger, 2016/07/20 20:30:49]

Admittedly I don't fully understand our invalidation story but why is this
needed when other addChild/removeChild implementations don't need it?

[Gleb Lanbin, 2016/07/21 18:25:31]

so usually <fieldset> looks like this
+-- Legend ------------+
|   Content            |
+----------------------+
when <legend> gets removed we need to repaint <fieldset> because it doesn't have
box decoration anymore.

[cbiesinger, 2016/07/21 19:54:16]

Hm... I see. But, in general when we remove a child we need to do a layout and
paint anyway. So this is not covered by the general mechanism, such as
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Conta...
?

[Gleb Lanbin, 2016/07/21 20:49:01]

done. removed.

you're right. I don't need it anymore.

+    } else if (oldChild == m_innerBlock) {
+        LayoutFlexibleBox::removeChild(oldChild);
+        m_innerBlock = nullptr;
+    } else if (m_innerBlock) {
+        m_innerBlock->removeChild(oldChild);
+    }
+}
+
 } // namespace blink