971655 - Containing box of <fieldset> with <legend> for absolute positioning has the "bottom:0" point below the bottom of the <fieldset>

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot

As I filed bug 971653, the top edge of containing box of <fieldset> with <legend> for absolute positioning is different from other browsers.

According to this testcase, the containing box is just moved down without size adjustment.
http://jsfiddle.net/d_toybox/X2U8J/3/

The internal <div> element has "top: 0; bottom: 0; left: 0; right: 0;". So, the <div> (red dashed border's box) represents the containing box size.

Even if bug 971653 is INVALID or WONTFIX, I believe that this is a bug.

Comment 1

[Daniel Holbert [:dholbert]]

Here's a somewhat-more-reduced testcase:

data:text/html,<fieldset style="position:relative; height: 100px; width:100px;"><legend>l</legend><div style="position:absolute; bottom: 0">abspos

The abspos text has "bottom:0", so it should be (just) inside the fieldset border, but instead it's pushed out (by the <legend>).

mozregression says this behavior changed with this window:
{
Last good nightly: 2013-10-25
First bad nightly: 2013-10-26

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9f8233fcce1d&tochange=ef3f5669b53e
}

So: (unsurprisingly) caused by bug 261037.

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

We do compensate the height of the inner frame for the legend offset here:
http://hg.mozilla.org/mozilla-central/annotate/879038dcacb7/layout/forms/nsFieldSetFrame.cpp#l457
but then CalculateContainingBlockSizeForAbsolutes adds in the padding
for the FieldsetFrame here (since they have the same content):
http://hg.mozilla.org/mozilla-central/annotate/879038dcacb7/layout/generic/nsBlockFrame.cpp#l893
although it's the inner (-moz-fieldset-content) frame that is the
abs.pos. containing block.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix+reftest

https://tbpl.mozilla.org/?tree=Try&rev=1de5f2f4bdf2

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8375761 [details] [diff] [review]
fix+reftest

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

::: layout/generic/nsBlockFrame.cpp
@@ +895,5 @@
>      const nsHTMLReflowState* aLastRS = &aReflowState;
>      const nsHTMLReflowState* lastButOneRS = &aReflowState;
>      while (aLastRS->parentReflowState &&
> +           aLastRS->parentReflowState->frame->GetContent() == frame->GetContent() &&
> +           aLastRS->parentReflowState->frame->GetType() != nsGkAtoms::fieldSetFrame) {

Please add a comment explaining why we're doing this.

Comment 5

[Mats Palmgren (inactive)]

Added a code comment:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af811e70b7ad

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af811e70b7ad

Comment 7

[Mats Palmgren (inactive)]

Might be worth uplifting this and bug 971933 to Aurora.  What do you think?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8375761 [details] [diff] [review]
fix+reftest

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 261037
User impact if declined: wrong layout for abs.pos. children in <fieldset>
Testing completed (on m-c, etc.): on m-c since 2014-02-15
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04e182a4426b