Closed Bug 1470176 Opened 6 years ago Closed 6 years ago

Implement contain:size for fieldset objects

Categories

(Core :: Layout: Form Controls, enhancement, P3)

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1470176 - Implement contain:size for fieldset objects.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
Helper bug for implementing contain:size for GridContainerFrame and Select objects.
Status: NEW → ASSIGNED
Summary: Implement contain:size for GridContainerFrame, Select → Implement contain:size for GridContainerFrame, Select, Fieldset
Chrome supports contain:size for fieldset objects, so this bug will also address the implementation of contain:size for those.
Summary: Implement contain:size for GridContainerFrame, Select, Fieldset → Implement contain:size for Select, Fieldset objects
Still working on Selects; just pushing to mozreview because its easier to hg import from the link there than it is to export + scp my patch when I want to debug remotely with rr on the linux machine. No need to review yet :)
Attachment #8988522 - Flags: review?(dholbert)
Comment on attachment 8988522 [details]
Bug 1470176 - Implement contain:size for Select.

https://reviewboard.mozilla.org/r/253600/#review264208

::: layout/forms/nsComboboxControlFrame.cpp:829
(Diff revision 4)
>  {
>    nscoord minISize;
>    DISPLAY_MIN_WIDTH(this, minISize);
> +  if (StyleDisplay()->IsContainSize()) {
> +    // XXX We probably want to modify GetIntrinsicISize, or
> +    // the subsequent call to nsBlockFrame::GetPrefISize,

s/GetPrefISize/GetMinISize/
Blocks: 1476127
Component: Layout → Layout: Form Controls
Comment on attachment 8988522 [details]
Bug 1470176 - Implement contain:size for Select.

https://reviewboard.mozilla.org/r/253782/#review264464

::: layout/forms/nsComboboxControlFrame.cpp:827
(Diff revision 4)
> +  if (StyleDisplay()->IsContainSize()) {
> +    // XXX We probably want to modify GetIntrinsicISize, or
> +    // the subsequent call to nsBlockFrame::GetPrefISize,
> +    // to take into account the default size of an empty
> +    // select object (which is nonzero). See bug 1476127.
> +    minISize = 0;
> +  } else {
> -  minISize = GetIntrinsicISize(aRenderingContext, nsLayoutUtils::MIN_ISIZE);
> +    minISize = GetIntrinsicISize(aRenderingContext, nsLayoutUtils::MIN_ISIZE);
> +  }
>    return minISize;
>  }
>  
>  nscoord
>  nsComboboxControlFrame::GetPrefISize(gfxContext *aRenderingContext)
>  {
>    nscoord prefISize;
>    DISPLAY_PREF_WIDTH(this, prefISize);
> +  if (StyleDisplay()->IsContainSize()) {
> +    // XXX We probably want to modify GetIntrinsicISize, or

Rather than making this same change in two different places, could we make a single change in a more targeted spot, in their common helper-function -- in GetIntrinsicISize()?  It lives just above these two functions.

Looking at that function: perhaps contain:size should make us skip its calls to...
 nsLayoutUtils::IntrinsicForContainer(...)
 mDropdownFrame->GetMinISize(...)
 mDropdownFrame->GetPrefISize(...)
...and behave as if those all returned 0, since those seem to be the parts where we take child sizing into account.

(I think we need to preserve its parts about setting aside space for possible-scrollbars, since those seem to be independent of our children.)

Once you've done this, I imagine it might fix some of the cases that were too small & tracked in bug 1476127 (maybe this scrollbar sizing thing is what accounts for the size difference?) though maybe not.

::: layout/generic/nsBlockFrame.cpp:492
(Diff revision 4)
> -  if (StyleDisplay()->IsContainSize()) {
> +  if (StyleDisplay()->IsContainSize() && !IsComboboxControlFrame()) {
>      return false;
>    }

This change merits an explanatory code-comment, at least... (On the surface, it looks like this change lets us leak baseline information from inside of a select element -- does it do that?  If not, where do we prevent that, if not here?)

::: layout/generic/nsBlockFrame.cpp:1640
(Diff revision 4)
> -  else if (aReflowInput.mStyleDisplay->IsContainSize()) {
> +  else if (aReflowInput.mStyleDisplay->IsContainSize() && !IsComboboxControlFrame()) {
>      // If we're size-containing and we don't have a specified size, then our
>      // final size should actually be computed from only our border and padding,
>      // as though we were empty.
>      // Hence this case is a simplified version of the case below.
> +    // We want to avoid resizing comboboxes (which call this reflow method)
> +    // because we handle resizing elsewhere in their class.

I'm confused about how we handle resizing for comboboxes elsewhere... I don't see any changes to combobox reflow anywhere here.

Also, this chunk of code isn't about "resizing" -- it's simply computing the final BSize at the end of reflow. And if we skip this case, we still computing some final  BSize, and we just do it it in a way that takes the child into account...  So I don't see what the purpose of avoiding this special case is.

(Maybe we want to take the child into consideration, if the child happens to be a helper-box that is responsible for the "real" ignoring of our children at its level of granularity? (I don't know if that's the case, just guessing.)  Still: if that's the case, I don't see where/how that helper-box manages this, since this seems to be the only change to reflow-related code in this patch...)

::: layout/reftests/w3c-css/submitted/contain/contain-size-select-001.html:31
(Diff revision 4)
> +    <option>CSS Test: A size-contained floated select with specified width and no specified height should render as if it had no contents.</option>
> +    <option>a</option>

Wording tweak (throughout both testcases):
 s/render/size itself/

"should render as if it had no contents" isn't strictly correct, in general, because rendering includes painting, and normally the contents would be something paintable & **would** impact the rendering of the select element (though they're transparent here of course)

::: layout/reftests/w3c-css/submitted/contain/contain-size-select-001.html:38
(Diff revision 4)
> +  <select class="iFlexWidth">
> +    <option>CSS Test: A size-contained inline-flex select with specified width and no specified height should render as if it had no contents.</option>

We should get rid of this iFlexWidth part of the test -- I don't think Firefox (or probably any browser) does anything special rendering-wise for "display:inline-flex" styling on select elements.  We just treat it as inline-level, and let the select do its layout the way it wants to, without any "flex" flavoring.

(We do have a special case for buttons, because people wanted to use flexbox to lay out button children -- so that's why that was an interesting edge case to test "contain:size" on there.  But it's not meaningful for select, so it's probably not worth dedicating a piece of a test to.)

::: layout/reftests/w3c-css/submitted/contain/contain-size-select-002-ref.html:25
(Diff revision 4)
> +  <select class="iFlexBasic">
> +  </select>
> +  <br>

As noted for -001:  let's get rid of iFlexBasic, because it's not a meaningful display value on a select element.

::: layout/reftests/w3c-css/submitted/contain/contain-size-select-002.html:39
(Diff revision 4)
> +  <select class="iFlexBasic">
> +    <option>CSS Test: A size-contained inline-flex select with auto width and no specified height should render as if it had no contents.</option>
> +    <option>a</option>
> +    <option>b</option>
> +    <option>c</option>
> +  </select>
> +  <br>

As noted for -001:  let's get rid of iFlexBasic, because it's not a meaningful display value on a select element.
Attachment #8988522 - Flags: review?(dholbert) → review-
Comment on attachment 8988522 [details]
Bug 1470176 - Implement contain:size for Select.

https://reviewboard.mozilla.org/r/253782/#review264464

> Rather than making this same change in two different places, could we make a single change in a more targeted spot, in their common helper-function -- in GetIntrinsicISize()?  It lives just above these two functions.
> 
> Looking at that function: perhaps contain:size should make us skip its calls to...
>  nsLayoutUtils::IntrinsicForContainer(...)
>  mDropdownFrame->GetMinISize(...)
>  mDropdownFrame->GetPrefISize(...)
> ...and behave as if those all returned 0, since those seem to be the parts where we take child sizing into account.
> 
> (I think we need to preserve its parts about setting aside space for possible-scrollbars, since those seem to be independent of our children.)
> 
> Once you've done this, I imagine it might fix some of the cases that were too small & tracked in bug 1476127 (maybe this scrollbar sizing thing is what accounts for the size difference?) though maybe not.

The non-zero ISize of an empty select comes from the nsLayoutUtils::IntrinsicForContainer call which I've addded a ( ... && !StyleDisplay()->IsContainSize()) condition to. Right now, I'm mimicing the same behaviour as having getPrefISize and getMinISize return 0 on size-contained elements, but this doesn't fix the issue in bug 1476127; I think that exists deeper down the callstack from nsLayoutUtils::IntrinsicForContainer. My comment below might also have something to do with this.

> I'm confused about how we handle resizing for comboboxes elsewhere... I don't see any changes to combobox reflow anywhere here.
> 
> Also, this chunk of code isn't about "resizing" -- it's simply computing the final BSize at the end of reflow. And if we skip this case, we still computing some final  BSize, and we just do it it in a way that takes the child into account...  So I don't see what the purpose of avoiding this special case is.
> 
> (Maybe we want to take the child into consideration, if the child happens to be a helper-box that is responsible for the "real" ignoring of our children at its level of granularity? (I don't know if that's the case, just guessing.)  Still: if that's the case, I don't see where/how that helper-box manages this, since this seems to be the only change to reflow-related code in this patch...)

I think this was left from when I was modifying the calls in nsLayoutUtils::IntrinsicForContainer, which I've removed/avoided re: your first piece of feedback in comment 8. 
I think the reason I want to avoid that chunk of code (now) is because it was added specifically for size-contained block frames which don't have the same default padding/button handling/etc. that comboboxes do. That said, Combobox::Reflow calls BlockFrame::Reflow, so that case ends up getting triggered for size-contained comboboxes. In debugging I found my reference case for comboboxes takes the second-to-last case in that suite (else if (aState.mReflowStatus.IsComplete()) and I wanted to mimic that behaviour for the test case. There is some child-relative stuff in there, though, so maybe its better I take a look there. 
It's hard to pinpoint, though, because the value that differs from the reference case is aReflowInput.ComputedISize(), not the finalSize.ISize value.
Attachment #8988522 - Attachment is obsolete: true
Comment on attachment 8993099 [details]
Bug 1470176 - Implement contain:size for fieldset objects.

https://reviewboard.mozilla.org/r/257926/#review264844

Review notes for fieldset patch:

::: layout/forms/nsFieldSetFrame.cpp:266
(Diff revision 1)
> -
> -  if (nsIFrame* legend = GetLegend()) {
> +  nsIFrame* legend = GetLegend();
> +  if (legend && !StyleDisplay()->IsContainSize()) {
>      // We want to avoid drawing our border under the legend, so clip out the
>      // legend while drawing our border.  We don't want to use mLegendRect here,
>      // because we do want to draw our border under the legend's inline-start and
>      // -end margins.  And we use GetNormalRect(), not GetRect(), because we do
>      // not want relative positioning applied to the legend to change how our
>      // border looks.
> +    // If we're size contained, we want to render the border anyway since
> +    // legends are technically children, and they should be ignored
> +    // if size-containment is applied.

I think you want to revert your changes here -- this is a painting-specific function, so it doesn't seem like contain:size should influence its behavior.

::: layout/forms/nsFieldSetFrame.cpp:599
(Diff revision 1)
> +    // Because fieldset objects have some inherent padding, and
> +    // because we want to ignore our children (inner), we manually adjust
> +    // our BSize and padding here for size-contained objects.
> +    // If unspecified, our BSize should be as small as
> +    // possible (max(0, MinBSize)), otherwise we'll use the
> +    // BSize we're given. In both cases, we add in room for padding
> +    // and the border.

I think this comment needs some tweaks -- in particular:
 - "inherent padding" isn't really relevant (buttons have some default padding as well and yet they don't have this same sort of special-case).
 - This code doesn't really "adjust...our padding" -- really, it has to *account for* our padding when computing our BSize.

I would suggest rephrasing this comment along these lines, focusing on the child-ignoring stuff & with an afterthought about why this particular frame class has to specially account for the padding here:
====
If we're size-contained, then we must set finalSize to be what it'd be if we had no children (i.e. if we had no legend and if 'inner' were empty).  Note: normally the fieldset's own padding (which we still must honor) would be accounted for as part of inner's size (see kidReflowInput.Init() call above).  So: since we're disregarding sizing information from 'inner', we need to account for that padding ourselves here.
====

::: layout/forms/nsFieldSetFrame.cpp:606
(Diff revision 1)
> +    nscoord toAdd = aReflowInput.ComputedBSize() == NS_UNCONSTRAINEDSIZE
> +      ? aReflowInput.ApplyMinMaxBSize(0)
> +      : aReflowInput.ComputedBSize();
> +    finalSize.BSize(wm) = border.BStartEnd(wm) + toAdd +
> +      aReflowInput.ComputedLogicalPadding().BStartEnd(wm);
> +  }

Two nits:
 (1) "toAdd" is too vague of a name -- let's call it "contentBoxBSize", since I think that's what it represents.

 (2) You can get rid of the "border" usage here, and instead add  aReflowInput.ComputedLogicalBorderPadding().BStartEnd(wm);

(aReflowInput already has border+padding added together for you in this handy accessor.  The other codepath splits them apart for separate handling because it defers the padding to the inner frame, but we don't have to do that in this clause -- we might as well just use the pre-added version.)

::: layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-001.html:12
(Diff revision 1)
> +    border: 1em solid green;
> +    background: lightblue;

As discussed in our 1:1, you might want to make the border transparent here, at least for pieces of the test where you're testing that <legend> doesn't influence the intrinsic height/width of size-contained fieldsets (and have <legend> in the testcase vs. no-legend in the reference case).

Otherwise, as we observed, <legend> will interrupt the border's painting and that'll make a visual difference between the testcase vs. reference case which we don't want to have in a reftest.

(And you should also include a testcase [here or in another file] that has a fieldset with a legend and a border (the default border is fine if you like), which is size-contained & perhaps has a fixed height for predictable rendering.  The goal of this part would be to be sure that we still draw the border/legend in the same spot, regardless of size containment.)
Comment on attachment 8993099 [details]
Bug 1470176 - Implement contain:size for fieldset objects.

https://reviewboard.mozilla.org/r/257926/#review264844

> I think you want to revert your changes here -- this is a painting-specific function, so it doesn't seem like contain:size should influence its behavior.

I reverted this and ran into some issues with the call to VisualBorderRectRelativeToSelf(); in the case that there is a legend, and there isn't a wide-enough border to place the legend in, the legend eats up some of that default padding so that the border wraps to the middle of the legend (so, the legend gets shifted down into that padding as far as needed to make that happen, and the content box gets painted behind the legend rect). We probably don't want to change that function (^) because it'd cause us to draw legends differently than we would if we weren't size-contained. That said, this causes the reftest to fail because even though the legends are 'visibility:hidden' in the test case, the fact that they exist at all makes the painting happen differently. I think the functionality is what we want, but I'm not sure how to alter the reftests to test that the height/width with legends + containment matches height/width with no legends or containment. If you have any ideas, please let me know!
Flags: needinfo?(dholbert)
Possible solution to the above: 
I could write the reftests testing height/width with borders large enough for the call to VisualBorderRectRelativeToSelf() to have no effect. That way, they'd only be testing "if legends are guaranteed to not change how our content box renders, but other children may (ie. the innerContents div), do we still get the expected dimensions?". I'd leave the last testcase we talked about adding in comment 11 with the default border and fixed height in both the ref and test cases to test that the legend is drawn and placed the same in both cases.
I don't think we want to use a tall-enough-border-so-the-legend-doesn't-matter -- we want to test a case where the legend *would* matter if it weren't for contain:size.

It does seem like the legend ends up (correctly) stealing space from the fieldset-inner-frame's painted background area, so we can't rely on that inner-frame's painted background to tell us how big the fieldset is. So maybe we can measure the fieldset size by wrapping the whole fieldset element in a wrapper-div (with a border on that wrapper div), and making the fieldset visibility:hidden, and then having a legend and contain:size in the testcase vs. no legend & no "contain" in the reference case.
Flags: needinfo?(dholbert)
I tried to work on selects a bit more yesterday; specifically I tried to find out why that (IsContainSize && !IsCombobox) condition was necessary in calculating the baseline. Looks like on the last 'line' (option?) of every combobox, we trigger the else case here (https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/layout/generic/nsBlockFrame.cpp#508) and the inner if case, too, so we return true. I'm not sure if this is related to that XXX comment, but the reason we hit the true case on the last iteration (and why it makes the box draw as expected in the ref case but not the test case) is non-obvious to me.

Because I don't want to submit the patch with the weird hack-y condition without a comment (and because Chrome doesn't actually implement contain on selects), I think it'd be a good idea to move selects to that follow-up bug and land fieldsets (pending review). What do you think?
Flags: needinfo?(dholbert)
Sure, that sounds good to me.
Flags: needinfo?(dholbert)
Summary: Implement contain:size for Select, Fieldset objects → Implement contain:size for fieldset objects
No longer blocks: 1476127
Comment on attachment 8993099 [details]
Bug 1470176 - Implement contain:size for fieldset objects.

https://reviewboard.mozilla.org/r/257926/#review265800

::: layout/forms/nsFieldSetFrame.cpp:319
(Diff revision 2)
> +  if (!StyleDisplay()->IsContainSize()) {
> +  // Both inner and legend are children, and if the fieldset is size-contained
> +  // they should not contribute to the intrinsic size.
> -  if (nsIFrame* legend = GetLegend()) {
> +    if (nsIFrame* legend = GetLegend()) {

The comment needs 2 more spaces of indentation here.

::: layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-001.html:16
(Diff revision 2)
> +  .container {
> +    border: 10px solid green;
> +  }

Two nits:
 (1) Please add a code-comment explaining the reason for having this .container box in the test.

 (2) Right now, the .container is always the full width of the viewport, even in your "specified width" example.  Since the container is the only visible thing, that means we can't actually see the effects of the specified width.  To fix this, how about we make .container be "display: inline-block" -- that'll make it be intrinsically sized, to reveal the intrinsic width of the fieldset inside of it.

::: layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-002.html:37
(Diff revision 2)
> +  <!--CSS Test: A size-contained floated fieldset element with no specified size should size itself as if it had no contents.-->
> +  <div class="container"><fieldset class="contain floatLBasic">
> +    <legend>legend</legend>
> +    <div class="innerContents">inner</div>
> +  </fieldset></div>
> +  <br>
> +
> +  <!--CSS Test: A size-contained floated fieldset element with specified width and no specified height should size itself as if it had no contents.-->
> +  <div class="container"><fieldset class="contain floatLWidth">
> +    <legend>legend</legend>
> +    <div class="innerContents">inner</div>
> +  </fieldset></div>
> +  <br>

This part already "passes" in Nightly (and in release firefox), because floats don't contribute to the height of their container.  So this isn't effectively testing anything.

However, I think this chunk can probably go away, if you agree with my suggestion to add "display:inline-block" to the .container{} wrapper in the other testcase (-001).  With that change, this chunk  will kinda be redundant. ("float" is just one way of triggering the intrinsic-sizing codepath -- and another way is to be inside of a display:inline-block container.)
Attachment #8993099 - Flags: review-
Comment on attachment 8993741 [details]
Bug 1470176 - Implement contain:size for Select.

https://reviewboard.mozilla.org/r/258450/#review266428

(Canceling review on part 2 here - when you get a chance, I think you want to just selectively push the fieldset patch, so that mozreview realizes that this is now a 1-patch bug, per comment 16.)
Attachment #8993741 - Flags: review?(dholbert)
Attachment #8993741 - Attachment is obsolete: true
Changing the container class to inline-block also revealed some baseline alignment issues, so I updated baseline methods in the fieldset class to match what I'd done for size-contained block frames and button frames. Other than addressing your mozreview feedback, that's the only additional change to the patch.
Comment on attachment 8993099 [details]
Bug 1470176 - Implement contain:size for fieldset objects.

https://reviewboard.mozilla.org/r/257926/#review266502

r+ with nits addressed:

::: layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-002.html:5
(Diff revision 3)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: size' on fieldset elements should cause them to be sized and baseline-aligned as if they had no contents.</title>

Drop "sized and" here.

(Right now, this "sized and" is present here in the title but not in the actual HTML comment next to the one fieldset in this test.  Sizing isn't really directly tested here, so probably best to just drop the mention of it and leave the other testcase to test that.)

::: layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-002.html:38
(Diff revision 3)
> +  <!--Note: The following .container class is used to help test if size-contained
> +  fieldsets and non-contained fieldsets have the same size. Normally, we'd test
> +  that a fieldset with children and size-containment is drawn identically to a
> +  fieldset without containment or children. However, when we have a legend as
> +  a child, border placement and padding of the fieldset are changed.
> +  To check the dimensions between the ref-case and test-case without
> +  failing because of the border/padding differences, we make the fieldset
> +  {visibility:hidden; border:none;} and add a .container wrapper div.-->
> +

This comment is out of place here, I think...  This testcase doesn't seem to use a .container wrapper div.  Probably remove this comment?


(Same goes for the ".container {...}" rule up in this test's <style> block -- that rule seems to be unused and wants to be removed.)
Attachment #8993099 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca5ab353b5d8
Implement contain:size for fieldset objects. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca5ab353b5d8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: