Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionAdd 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> #Messages
Total messages: 84 (49 generated)
Description was changed from ========== Add grid/flex layout support for <fieldset> BUG=262679 ========== to ========== 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 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ========== to ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ==========
glebl@chromium.org changed reviewers: + eae@chromium.org
This looks great but cbiesinger would be a better reviewer.
eae@chromium.org changed reviewers: + cbiesinger@chromium.org
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1220: LayoutObject* childToExclude = layoutSpecialExcludedChild(relayoutChildren, layoutScope); Thanks for doing this! I'll take a closer look tomorrow but briefly, I don't think calling this inside this function works because we call it (potentially) multiple times during layout of a single flexbox, whereas we probably only want to layout the special excluded child once. I would call it from layoutFlexItems and then just pass in the LayoutObject to this function so we can then skip it.
rego@igalia.com changed reviewers: + rego@igalia.com
As explained in the comments I don't like the change. In any case the description doesn't mention that now LayoutFieldset inherits from LayoutFlexibleBox (which is a quite important change). Nit about the description, you should wrap lines at 72 characters. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:3: <head> You don't need html, head and body tags on this test, you can simply get rid of them. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:4: <title>Test Case with FIELDSET as flex container</title> We don't usually use the title to describe the test, for that we use the file name. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:7: padding: 1em; This is not needed. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:11: It's usually nice to add a paragraph explaining what the test is checking and the expected result. In this case the expected result is clear in the text below though. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:20: </html> And last thing about the test, I don't see any good reason to not be a reference test. The -exptected file could have something like: <fieldset> <legend>Fieldset with display: flex</legend> <div>these fieldsshouldn't bestacked vertically</div> </fieldset> Reference tests are much better as we won't need all the rebaselines, and they're platform independent. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:4: <title>Test Case with FIELDSET as grid container</title> Same comments about tags and title like in the previous test. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:15: .nw{ Please use more descriptive names for the CSS classes. Something like firstRowFirstColumn is much clearer. For Grid Layout you could reuse some CSS classes from: fast/css-grid-layout-resources/grid.css (anyway you can define your own ones, not a big deal). Nit: Missing space between class name and bracket. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:38: <body> In this case we clearly miss a description of the expected result. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:253: { 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 https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.h (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.h:31: class LayoutFieldset final : public LayoutFlexibleBox { Are we sure we want to do this change? Inheriting from Flexbox might be a bad idea thinking on the long term. What's the main reason to do this? We've been getting rid of this kind of inheritance as they cause issues when you're developing things on Flexbox. Some changes on Flexbox might affect the classes inheriting for him, so you might be broken things or you might need to tweak things in many places. IMHO, we should try to avoid this kind of things as the code becomes very complex. For example, we're using Flexbox for full-screen and we want to avoid it, and this was causing issues when implementing some alignment stuff.
jfernandez@igalia.com changed reviewers: + jfernandez@igalia.com
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) What's the rationale of inheriting from Flexbox ? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:44: return LayoutBlock::baselinePosition(baseline, firstLine, direction, position); 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:208: ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef(); 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:237: ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef(); Ditto.
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) On 2016/07/20 14:59:57, jfernandez wrote: > What's the rationale of inheriting from Flexbox ? 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? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:44: return LayoutBlock::baselinePosition(baseline, firstLine, direction, position); On 2016/07/20 14:59:57, jfernandez wrote: > 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. flexbox has a different baseline behavior which most elements don't want :( https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:208: ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef(); On 2016/07/20 14:59:57, jfernandez wrote: > 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. Yeah, I don't understand why this is necessary? Doesn't the flex-grow/flex-shrink take care of this? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:228: if (AXObjectCache* cache = document().existingAXObjectCache()) Hm, why is this necessary? We don't call it in other addChild implementations? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:238: innerStyle.setFlexShrink(1); Use 1.0f, since these are floats, not ints. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:239: innerStyle.setFlexGrow(1); 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? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:256: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); Admittedly I don't fully understand our invalidation story but why is this needed when other addChild/removeChild implementations don't need it?
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) On 2016/07/20 20:30:49, cbiesinger wrote: > On 2016/07/20 14:59:57, jfernandez wrote: > > What's the rationale of inheriting from Flexbox ? > > 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? 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.
The reason it uses flexbox is for correct sizing of the anonymous inner block -- the one that this patch introduces. In particular you want the inner block to be the same size as the outer block even when it is explicitly sized. This is more about the height than the width. That's why fieldset didn't need it before. It could probably be done manually too, sure. Maybe you're right that for <fieldset> that would be easier (less clear for <button>) The comment about flex-direction column etc. is what would be fixed by one of my previous comments :-)
Description was changed from ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ========== to ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ==========
Description was changed from ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ========== to ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ==========
thanks for your comments. Please take another look. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:3: <head> On 2016/07/20 06:00:23, Manuel Rego wrote: > You don't need html, head and body tags on this test, > you can simply get rid of them. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:4: <title>Test Case with FIELDSET as flex container</title> On 2016/07/20 06:00:23, Manuel Rego wrote: > We don't usually use the title to describe the test, > for that we use the file name. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:7: padding: 1em; On 2016/07/20 06:00:23, Manuel Rego wrote: > This is not needed. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:11: On 2016/07/20 06:00:23, Manuel Rego wrote: > It's usually nice to add a paragraph explaining what the test is checking > and the expected result. > > In this case the expected result is clear in the text below though. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:20: </html> On 2016/07/20 06:00:23, Manuel Rego wrote: > And last thing about the test, > I don't see any good reason to not be a reference test. > The -exptected file could have something like: > <fieldset> > <legend>Fieldset with display: flex</legend> > <div>these fieldsshouldn't bestacked vertically</div> > </fieldset> > > Reference tests are much better as we won't need all the rebaselines, > and they're platform independent. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:4: <title>Test Case with FIELDSET as grid container</title> On 2016/07/20 06:00:23, Manuel Rego wrote: > Same comments about tags and title like in the previous test. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:15: .nw{ On 2016/07/20 06:00:23, Manuel Rego wrote: > Please use more descriptive names for the CSS classes. > Something like firstRowFirstColumn is much clearer. > > For Grid Layout you could reuse some CSS classes from: > fast/css-grid-layout-resources/grid.css > (anyway you can define your own ones, not a big deal). > > Nit: Missing space between class name and bracket. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-grid.html:38: <body> On 2016/07/20 06:00:23, Manuel Rego wrote: > In this case we clearly miss a description of the expected result. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:38: : LayoutFlexibleBox(element), m_innerBlock(nullptr) On 2016/07/21 10:28:43, Manuel Rego wrote: > On 2016/07/20 20:30:49, cbiesinger wrote: > > On 2016/07/20 14:59:57, jfernandez wrote: > > > What's the rationale of inheriting from Flexbox ? > > > > 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? > > 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. 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:208: ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef(); On 2016/07/20 20:30:49, cbiesinger wrote: > On 2016/07/20 14:59:57, jfernandez wrote: > > 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. > > Yeah, I don't understand why this is necessary? Doesn't the > flex-grow/flex-shrink take care of this? 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 https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:228: if (AXObjectCache* cache = document().existingAXObjectCache()) On 2016/07/20 20:30:49, cbiesinger wrote: > Hm, why is this necessary? We don't call it in other addChild implementations? 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:237: ComputedStyle& innerStyle = m_innerBlock->mutableStyleRef(); On 2016/07/20 14:59:57, jfernandez wrote: > Ditto. this is similar to LayoutButton::updateAnonymousChildStyle https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:238: innerStyle.setFlexShrink(1); On 2016/07/20 20:30:49, cbiesinger wrote: > Use 1.0f, since these are floats, not ints. Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:239: innerStyle.setFlexGrow(1); On 2016/07/20 20:30:49, cbiesinger wrote: > 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? Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:253: { On 2016/07/20 06:00:23, Manuel Rego wrote: > 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 done. after submitting this patch for review I found the same issues that you found and already fixed them. thanks https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:256: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); On 2016/07/20 20:30:49, cbiesinger wrote: > Admittedly I don't fully understand our invalidation story but why is this > needed when other addChild/removeChild implementations don't need it? 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.h (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.h:31: class LayoutFieldset final : public LayoutFlexibleBox { On 2016/07/20 06:00:23, Manuel Rego wrote: > Are we sure we want to do this change? > Inheriting from Flexbox might be a bad idea thinking on the long term. > What's the main reason to do this? > > We've been getting rid of this kind of inheritance as they cause issues > when you're developing things on Flexbox. > Some changes on Flexbox might affect the classes inheriting for him, > so you might be broken things or you might need to tweak things in many places. > IMHO, we should try to avoid this kind of things as the code becomes > very complex. > > For example, we're using Flexbox for full-screen and we want to avoid it, > and this was causing issues when implementing some alignment stuff. please see the reply from cbiesenger@ on a similar question below. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1220: LayoutObject* childToExclude = layoutSpecialExcludedChild(relayoutChildren, layoutScope); On 2016/07/20 01:42:31, cbiesinger wrote: > Thanks for doing this! I'll take a closer look tomorrow but briefly, I don't > think calling this inside this function works because we call it (potentially) > multiple times during layout of a single flexbox, whereas we probably only want > to layout the special excluded child once. I would call it from layoutFlexItems > and then just pass in the LayoutObject to this function so we can then skip it. Done.
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:1: <!DOCTYPE html> You should keep this line; we don't usually want to test in quirks mode (for all your tests) https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:228: if (AXObjectCache* cache = document().existingAXObjectCache()) On 2016/07/21 18:25:31, glebl wrote: > On 2016/07/20 20:30:49, cbiesinger wrote: > > Hm, why is this necessary? We don't call it in other addChild implementations? > > 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. 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. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:256: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); On 2016/07/21 18:25:31, glebl wrote: > On 2016/07/20 20:30:49, cbiesinger wrote: > > Admittedly I don't fully understand our invalidation story but why is this > > needed when other addChild/removeChild implementations don't need it? > > 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. 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... ?
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/fieldset/fieldset-display-flex.html:1: <!DOCTYPE html> On 2016/07/21 19:54:16, cbiesinger wrote: > You should keep this line; we don't usually want to test in quirks mode (for all > your tests) Done. https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:256: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); On 2016/07/21 19:54:16, cbiesinger wrote: > On 2016/07/21 18:25:31, glebl wrote: > > On 2016/07/20 20:30:49, cbiesinger wrote: > > > Admittedly I don't fully understand our invalidation story but why is this > > > needed when other addChild/removeChild implementations don't need it? > > > > 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. > > 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... > ? done. removed. you're right. I don't need it anymore.
https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:15: crbug.com/262679 accessibility/computed-role.html [ NeedsRebaseline ] To double check, did you verify that the only difference for these tests is the additional anonymous block, and that the sizing and images are staying the same?
https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:15: crbug.com/262679 accessibility/computed-role.html [ NeedsRebaseline ] On 2016/07/21 21:17:51, cbiesinger wrote: > To double check, did you verify that the only difference for these tests is the > additional anonymous block, and that the sizing and images are staying the same? yes, here is what I did 1) ran this patch through try bots. git cl try 2) verified that there are no differences in pixel tests and that layout text dump has the anonymous block
lgtm https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:224: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); Similar to my question about remove, is this needed here? https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.h (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.h:45: int inlineBlockBaseline(LineDirectionMode) const override { return -1; } Why this? Maybe add a comment?
Description was changed from ========== 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 TEST=fast/forms/fieldset/fieldset-display-flex.html; fast/forms/fieldset/fieldset-display-grid.html ========== to ========== 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 ==========
thanks for the review. https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:224: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::FieldsetChanged); On 2016/07/21 21:42:42, cbiesinger wrote: > Similar to my question about remove, is this needed here? Done. https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.h (right): https://codereview.chromium.org/2150003005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.h:45: int inlineBlockBaseline(LineDirectionMode) const override { return -1; } On 2016/07/21 21:42:42, cbiesinger wrote: > Why this? Maybe add a comment? I have to override 2 functions. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... updated the comment
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/07/21 18:14:14, cbiesinger wrote: > The reason it uses flexbox is for correct sizing of the anonymous inner block -- > the one that this patch introduces. In particular you want the inner block to be > the same size as the outer block even when it is explicitly sized. This is more > about the height than the width. That's why fieldset didn't need it before. The inner block is an anonymous block we've just created so I guess it cannot be explicitly sized. The thing is that we need to make the inner block to fill all the available height. For that we could use something like "fill-available" maybe, dunno. > It could probably be done manually too, sure. Maybe you're right that for > <fieldset> that would be easier (less clear for <button>) I don't know about the case of <button> I'd need to investigate it. But it's weird to me to inherit from Flexbox just for reusing 1 or 2 fetaures from it (stretch the height by default, avoid shrink-to-fit for smaller widths). It seems the thing about the widths is already implemented by <fieldset>. So it'd be "only" to use all the available height for the inner block. Flexbox is quite complex already, we're adding some extra complexity on the class itself due to this change. In addition, fieldset class is becoming much more complex after the patch. Are we sure this is better than providing this functionality manually? https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:253: { On 2016/07/21 18:25:31, glebl wrote: > On 2016/07/20 06:00:23, Manuel Rego wrote: > > 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 > > done. > > after submitting this patch for review I found the same issues that you found > and already fixed them. thanks 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. https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1146: // Ruby text needs to find their legend and position it inside the border of the object. This comment doesn't seem accurate. What has to do the legend with ruby? https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:39: void setInnerBlockPadding(bool isHorizontalWritingMode, const LayoutObject* innerBlock, const LayoutUnit& padding) Why you need "namespace {" here? Couldn't be enough with just using a static method? https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:62: virtual LayoutObject* layoutSpecialExcludedChild(bool /*relayoutChildren*/, SubtreeLayoutScope&) {return nullptr;}; This kind of comment is weird. If it's a bool better have a proper name for the argument.
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
I tried using fill-available for <button>: https://codereview.chromium.org/2177563003/ There are some minor positioning differences that could surely be solved, but also: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... So it looks like that doesn't work. Similarly, 100% doesn't work -- if nothing else it fails in quirks mode. This patch reuses flexbox in a fairly simple way; a little more complex than most subclasses: https://cs.chromium.org/search/?q=%22:+public+LayoutFlexibleBox%22&sq=package... But still, not bad. So I actually like it this way. It is, I expect, simpler than reimplementing the desired functionality. https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:39: void setInnerBlockPadding(bool isHorizontalWritingMode, const LayoutObject* innerBlock, const LayoutUnit& padding) On 2016/07/22 06:16:07, Manuel Rego wrote: > Why you need "namespace {" here? > Couldn't be enough with just using a static method? Style guide prefers anonymous namespace over static methods: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:253: { On 2016/07/22 06:16:06, Manuel Rego wrote: > On 2016/07/21 18:25:31, glebl wrote: > > On 2016/07/20 06:00:23, Manuel Rego wrote: > > > 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 > > > > done. > > > > after submitting this patch for review I found the same issues that you found > > and already fixed them. thanks > > Shouldn't we add tests to check that this kind of things are > working as expected? > 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. > 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. done. https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1146: // Ruby text needs to find their legend and position it inside the border of the object. On 2016/07/22 06:16:07, Manuel Rego wrote: > This comment doesn't seem accurate. What has to do the legend with ruby? Done. https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right): https://codereview.chromium.org/2150003005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:62: virtual LayoutObject* layoutSpecialExcludedChild(bool /*relayoutChildren*/, SubtreeLayoutScope&) {return nullptr;}; On 2016/07/22 06:16:07, Manuel Rego wrote: > This kind of comment is weird. If it's a bool better have a proper name > for the argument. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #7 (id:170001) has been deleted
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
glebl@chromium.org changed reviewers: + aboxhall@chromium.org
Alice, could you review accessibility changes. thank you.
glebl@chromium.org changed reviewers: - eae@chromium.org, jfernandez@igalia.com, rego@igalia.com
Alice, just a friendly reminder that I'm waiting for your review.
https://codereview.chromium.org/2150003005/diff/190001/content/browser/access... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/2150003005/diff/190001/content/browser/access... content/browser/accessibility/accessibility_win_browsertest.cc:977: AccessibleChecker grouping1_checker(std::wstring(), ROLE_SYSTEM_CLIENT, I'm guessing this is a result of the "anonymous inner block" being added. Ideally, the results of these tests shouldn't change at all, just like the pixel tests haven't changed - this is going to affect the user experience for assistive technology users. We may need to explore if some extra work needs to be done in modules/accessibility in Blink. https://codereview.chromium.org/2150003005/diff/190001/content/test/data/acce... File content/test/data/accessibility/aria/aria-posinset-expected-mac.txt (right): https://codereview.chromium.org/2150003005/diff/190001/content/test/data/acce... content/test/data/accessibility/aria/aria-posinset-expected-mac.txt:28: ++++AXGroup AXRoleDescription='group' Similarly, this is going to affect the user experience. Why has the label moved after the content it's labelling?
https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:225: m_innerBlock->addChild(newChild, beforeChild); I think you may need to explicitly call if (AXObjectCache* cache = document().existingAXObjectCache()) cache->childrenChanged(this); here, similarly to LayoutMenuList::addChild(), because you're overriding addChild() which would (indirectly) call that normally.
lgtm
Alice, thanks for the review! https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutFieldset.cpp (right): https://codereview.chromium.org/2150003005/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutFieldset.cpp:225: m_innerBlock->addChild(newChild, beforeChild); On 2016/07/27 21:44:45, aboxhall wrote: > I think you may need to explicitly call > > if (AXObjectCache* cache = document().existingAXObjectCache()) > cache->childrenChanged(this); > > here, similarly to LayoutMenuList::addChild(), because you're overriding > addChild() which would (indirectly) call that normally. Done.
Patchset #8 (id:210001) has been deleted
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:270001) has been deleted
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
glebl@chromium.org changed reviewers: + dmazzoni@chromium.org
On 2016/07/29 02:42:20, glebl wrote: Dominic, just a friendly reminder that I'm waiting for your review.
lgtm
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2150003005/#ps290001 (title: "put the legend first")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c6d69f896f406c9a7801b29cb8c02a88e5b01770 Cr-Commit-Position: refs/heads/master@{#409303}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:290001) has been created in https://codereview.chromium.org/2206793004/ by inferno@chromium.org. The reason for reverting is: Causing use-after-frees on ClusterFuzz..
Message was sent while issue was closed.
mstensho@opera.com changed reviewers: + mstensho@opera.com
Message was sent while issue was closed.
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?
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. |
