1490747 - <legend> margins affects its border-box alignment with the <fieldset> border

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

Background: https://github.com/whatwg/html/issues/3929

Testcase: data:text/html,<fieldset><legend style=margin-top:10px>LEGEND

Expected: the fieldset border aligns with the center of the "LEGEND" text.
Actual: it appears we align it to the center of the legend's margin-box.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Center the <legend> border-box within the <fieldset> border area (not its margin-box)

A couple of notes on this patch:
I have a patch that also handles block-axis auto-margins on the <legend>
in a way that I feel makes sense, but I think the spec around this is
too vague at the moment, so I'm intentionally leaving that out for now.
I'm hoping that we can move the spec towards a "css-align align-self"
model, like we did for handling the inline-axis (mapping @align to
justify-self).

The mLegendRect/mLegendSpace members are only used within the Reflow
method.  I'll file a follow-up bug on converting those to local vars.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - Center the <legend> border-box within the <fieldset> border area (not its margin-box). (tests)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb2bc4b63d08a08df3ef8df4d9c2ba0e5eccf75a

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9013123 [details] [diff] [review]
part 1 - Center the <legend> border-box within the <fieldset> border area (not its margin-box)

Review of attachment 9013123 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsFieldSetFrame.cpp
@@ +50,5 @@
> +    auto legendMargin = legend->GetLogicalUsedMargin(wm);
> +    nscoord legendStartMargin = legendMargin.BStart(wm);
> +    nscoord legendEndMargin = legendMargin.BEnd(wm);
> +    nscoord border = GetUsedBorder().Side(wm.PhysicalSide(eLogicalSideBStart));
> +    nscoord off = (legendStartMargin + legendSize / 2) - border / 2;

Could you add documentation for what this initial calculation for |off| is meant to represent here? It's a bit confusing, in part because it seems to be the opposite of what |off| represents in the reflow version of this code (in the other main chunk of this patch).

(the rough oppositeness being: at this point in the code, a large value for 'off' means our legend margin-box is a lot larger than the border, I think.  Whereas in reflow, a large value for 'off' means our legend margin-box is a lot *smaller* than the border.)

(After staring, I think this calculation is finding the distance (if any) that the legend's margin-box protrudes above the fieldset's border, in order for the legend's border-box to be centered within that border... is that right?)

@@ +56,5 @@
> +    if (off > nscoord(0)) {
> +      // We don't want to display our border below our border area.
> +      nscoord marginBoxSize = legendStartMargin + legendSize + legendEndMargin;
> +      nscoord borderAreaSize = std::max(border, marginBoxSize);
> +      off -= std::max(nscoord(0), off + border - borderAreaSize);

I don't immediately follow what this arithmetic is doing.

Though I do notice one interesting thing which creates an opportunity for clarity/simplification: If `border` is larger than `marginBoxSize`, then I think this code ends up necessarily setting |off| to 0, which makes the `r` adjustments no-ops.  (This is because we'll have done borderAreaSize = border, which means `border - borderAreaSize` will be 0, which means our `off -=` expression will simplify to "off -= off" which is effectively `off = 0`.)  And in that case, the rest of this clause is a no-op.  So it might be clearer to express that more directly?  E.g. maybe right after you compute marginBoxSize, we'd want something like:

  if (marginBoxSize > border) {
    // Adjust `off` to [do what?]
    off -=  std::max(nscoord(0), off + border - marginBoxSize);
    r.BStart(wm) += off;
    r.BSize(wm) -= off;
  }

(I think this is a simplified version of your logic, but I'm not 100% sure...

@@ +458,1 @@
>      // figure out the legend's rectangle

While you're here, maybe clarify what "the legend's rectangle" actually means here.

I think it wants to say "The legend's margin-box rectangle".  (AFAICT, nowhere is it documented that mLegendRect is the margin-box, so I initially assumed it was the border or content-box, which made the |off| calculation a bit hard to follow/understand.)

@@ +462,5 @@
>                    legendDesiredSize.ISize(wm) + legendMargin.IStartEnd(wm),
>                    legendDesiredSize.BSize(wm) + legendMargin.BStartEnd(wm));
>      nscoord oldSpace = mLegendSpace;
>      mLegendSpace = 0;
> +    nscoord borderAreaSize = border.BStart(wm);

this variable might want a rename (perhaps to something like "borderStartThickness"?)

Right now its name sounds like it refers to the size of the border-box area. (But it doesn't.)

@@ +466,5 @@
> +    nscoord borderAreaSize = border.BStart(wm);
> +    if (mLegendRect.BSize(wm) > borderAreaSize) {
> +      // mLegendSpace is the space to subtract from our content-box size below.
> +      mLegendSpace = mLegendRect.BSize(wm) - borderAreaSize;
> +      borderAreaSize = mLegendRect.BSize(wm);

This last assignment ("borderAreaSize = mLegendRect.BSize(wm)") seems unnecessary, right? The borderAreaSize is never used after this point (except in our 'else' clause that we're not visiting).

So let's get rid of this assignment, both because it's unnecessary & because it makes it a little more confusing to reason about what this variable represents.

@@ +471,5 @@
>      } else {
> +      // Center the legend's border-box to the border, but keep the legend's
> +      // margins within the Alignment Container (borderAreaSize).
> +      nscoord off = borderAreaSize / 2 -
> +                   (legendMargin.BStart(wm) + legendDesiredSize.BSize(wm) / 2);

The comment here is good, but it's not obvious how it maps to the code.

I think this would be clearer (and have fewer divisions & hence less rounding-error) if you split out the border-box-centering logic from the legendMargin BStart logic. How about:
      // Find border-box position that would center legend's
      // border-box within the fieldset border:
      nscoord off = (borderAreaSize - legendDesiredSize.BSize(wm)) / 2;

      // Convert this to a margin-box position:
      off -= legendMargin.BStart(wm);

@@ +475,5 @@
> +                   (legendMargin.BStart(wm) + legendDesiredSize.BSize(wm) / 2);
> +      if (off > 0) {
> +        nscoord sz = mLegendRect.BSize(wm);
> +        off -= std::max(nscoord(0), off + sz - borderAreaSize);
> +        mLegendRect.BStart(wm) += off;

I had to stare at this part for a little while to figure out what it was doing & why, and to conceptualize the change to the meaning of |off| across this code.

After staring for a bit, I think I understand it:
 * `off + sz - borderAreaSize` is how far the legend's margin-box would overflow off of the fieldset-border thickness (if we were to use `off` without further adjustments).
 * So: if there is any such overflow, we subtract it from `off` to snap the end of the margin-box to the end of the fieldset's border.

Can you add a comment to make that clearer?

Also: could you add an assertion to demonstrate that |off| is *still* nonnegative, directly after the "-=" adjustment?  (This wasn't obvious to me, but I think it has to be nonnegative at that point, given that we know that the legend's total margin-box size is smaller than the fieldset's border thickness.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 1 - Center the <legend> border-box within the <fieldset> border area (not its margin-box)

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Also: could you add an assertion to demonstrate that |off| is *still*
> nonnegative, directly after the "-=" adjustment?

I think it should be obvious now after cleaning up the code a bit.
Asserting things about nscoord values isn't that useful in any case,
since integer overflow easily triggers false positives.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 9015275 [details] [diff] [review]
part 1 - Center the <legend> border-box within the <fieldset> border area (not its margin-box)

Review of attachment 9015275 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Bug 1490747 part 1 - Center the <legend> border-box within the <fieldset> border area (not its margin-box)

Right now, the parenthetical at the end is ambiguous -- it can easily be misunderstood to be saying "not its [the <fieldset>'s] margin-box", but really it means to say "not its [the <legend>'s margin-box]".

Could you move the parenthetical to be earlier in the sentence, to make that unambiguous? Like so:
  "Center the <legend> border-box (not its margin-box) within the <fieldset> border area"

::: layout/forms/nsFieldSetFrame.cpp
@@ +59,5 @@
> +      nscoord marginBoxSize = legendStartMargin + legendSize + legendEndMargin;
> +      if (marginBoxSize > border) {
> +        // We don't want to display our border below our border area,
> +        // so we align it to the block-axis end if that happens.
> +        nscoord overflow = off + border - marginBoxSize;

I think this comment might have some wrong words -- at least, I don't understand what this code means about the possibility of "display[ing] our border below our border area" & how that maps to what the code here is actually doing.

I think perhaps this comment wants to say something like:
"If our border is thinner than the legend's margin-box, then be sure that the border offset doesn't make us draw our border below the legend's margin-box. If that would happen, then align our border to the legend's margin-box block-axis end".

(That seems to be what this code is checking/handling, unless I'm misunderstanding.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 9013124 [details] [diff] [review]
part 2 - Center the <legend> border-box within the <fieldset> border area (not its margin-box). (tests)

Review of attachment 9013124 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't sanity-check the arithmetic & hardcoded margin values here, but I'll assume it's good.

The commit message nit from comment 6 ("not its margin-box") applies here as well, since the commit message is mostly the same.

::: testing/web-platform/tests/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/legend-block-margins-2-ref.html
@@ +37,5 @@
> +<body>
> +
> +<f>
> +<div>
> +  <span class="fieldset" style="margin-top:9px"><span class="legend" style="top:-11px"></span><c style="margin-top:9px"></c></div>

view-source highlights this as invalid html, because the outermost <span> is never closed. I think your </div> at the end of this line (and every line like it) is supposed to be a </span>, right?

Looks like you can fix them all by doing search-and-replace to convert "</c></div>"  to "</c></span>".

@@ +113,5 @@
> +  <span class="fieldset" style="margin-top: 4px"><span class="legend" style="margin-top: -16px"><x style="top:3px"></x></span><c></c></div>
> +</div>
> +
> +<div>
> +  <span class="fieldset"><span class="legend" style="margin-top: -16px"><x><x></span><c></c></div>

View-source highlights something else in this line as an error -- the "<x><x></span>".  I think the second <x> is supposed to be a close-tag, </x>, right?

@@ +136,5 @@
> +<div>
> +  <span class="fieldset"><span class="legend" style="margin-top: -16px"><x></x></span><c></c></div>
> +</div>
> +
> +</f>

[Minor nit, only mentioning since you'll already be touching this file]:

There's an arbitrary whitespace difference between testcase vs. reference case here. Could you fix so that the two files are more usefully diffable?

(The reference case has a single blank line before + after </f>; whereas, the testcase has no blank line before & 2 blank lines after.)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7a45eb3e51
part 1 - Center the <legend> border-box (not its margin-box) within the <fieldset> border area.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/83fca840ea7f
part 2 - Center the <legend> border-box (not its margin-box) within the <fieldset> border area. (tests) r=dholbert

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13497 for changes under testing/web-platform/tests

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8f7a45eb3e51
https://hg.mozilla.org/mozilla-central/rev/83fca840ea7f

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13497
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Aos057XwRvqi89H7Y7gdkw)

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged