266850 - [FIX]Table with position:absolute (and no top/left specified) inside div is positioned incorrectly

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Uri Bernstein]

When a table is contained within a div, the table is absolutely-positioned, and
no values are given for "top" or "left", the table is positioned regardless of
the position of the containing div.

Seeing this on:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041028
Firefox/0.9.1+

Assuming this is a valid bug, it is a recent regression. The 20041026 Firefox
trunk build displayes the table just below the text in the containing div.
(there was no 20041027 mac trunk build so I couldn't test it).

Originally saw this when investigating bug 266715.

Testcase and screenshot coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

Testcase. In recent nightlies, the table is overlayed on the text (dots) above
the div in which it is contained.

Comment 2

[Uri Bernstein]

Attachment: screenshot: 20041028

Screenshot of testcase in:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041028
Firefox/0.9.1+

Comment 3

[Uri Bernstein]

Attachment: screenshot: 20041026

Screenshot of testcase in:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041026
Firefox/0.9.1+

Comment 4

[Mats Palmgren (inactive)]

Regression window on Linux: 2004102606 -- 2004102705  (bug 263406?)

Comment 5

[Mats Palmgren (inactive)]

Applying attachment 163244 [details] [diff] [review] to a recent but pre-263406 tree introduces the error.

Comment 6

[Mats Palmgren (inactive)]

The problem is that the place holder frame has the wrong parent...

Before bug 263406:
MATS: CreatePlaceholderFrameFor aFrame=TableOuter(table)(1)@0x84d6460     
aParentFrame=Block(div)(7)@0x84d61f8

After bug 263406:
MATS: CreatePlaceholderFrameFor aFrame=TableOuter(table)(1)@0x84bae20     
aParentFrame=Area(html)(-1)@0x84b9b00

Comment 7

[Boris Zbarsky [:bzbarsky]]

Perhaps we need to add an assert to SetInitialChildList() that all the incoming
frames have our frame as the parent or something?

The issue seems to be that in ConstructTableFrame() we have both
aGeometricParent (the area frame in this case) and aContentParent (the div in
this case).  We use the aGeometricParent to create our pseudoframes as needed
(which I guess makes some sense).  But we init parentFrame to the geometric
parent too.  That's the part that's wrong, though I'm not sure what the best fix is.

I hate table pseudo-frames.  :(

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and maybe we can make the layout regression tests keep track of parent
frames?  Or do they already?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mats, I'm not sure that's right.  In particular, consider the following situation:

<div style="display: table-row">
  <div style="display: table">
  </div>
</div>

In this case aContentParent should be the table-row, but the frame we actually
want to use as the parent of the placeholder is the cell-inner block inside the
anonymous cell...

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #9)
> Oh, and maybe we can make the layout regression tests keep track of parent
> frames?  Or do they already?

Are you saying that the layout regression tests is working again?
I think this would have been caught anyway since the position is wrong.

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #10)
> In this case aContentParent should be the table-row, but the frame we actually
> want to use as the parent of the placeholder is the cell-inner block inside the
> anonymous cell...

OK, I see what you mean...

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Are you saying that the layout regression tests is working again?

They've been working all along....  Build the layout-debug extension, run
"mozilla -layoutdebug", load the .lst file is as the regression test, etc. 
There are instructions somewhere on Mozilla.org.

Note that I did run the regression tests on the patch in bug 263406.

I agree that they would have caught this anyway, except they have a _lot_ of
false positives and I was looking for frame tree changes (since I was changing
frame construction) and just skimming over the layout changes.....  so I may
well have missed this, since it wasn't flagged as a frame tree change.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I think the right thing to do is to eliminate the aGeometricParent argument
altogether.  It's only used in one place, and that's the one that's messing up.
 We recompute the geometric parent inside this method anyway.

Patch coming up in a second.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 16

[Mats Palmgren (inactive)]

(In reply to comment #13)
> They've been working all along....  Build the layout-debug extension, run
> "mozilla -layoutdebug", load the .lst file is as the regression test, etc. 

Thanks, I got it running now.

> There are instructions somewhere on Mozilla.org.

Not really, http://www.mozilla.org/newlayout/regress.html

Comment 17

[Boris Zbarsky [:bzbarsky]]

I thought we had a more up-to-date document, but maybe it was just a newsgroup
post David made?

David, bernd, do you think it would make sense to keep track of parent frames in
the regression tests?

Comment 18

[Uri Bernstein]

(Made summary more readable).

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 163999 [details] [diff] [review]
Like so

Just for fun I ran the regression tests on this patch and it
resulted in 42 failed files (not counting false positives).
I checked a few manually but saw nothing wrong.
Does everyone else also get this many false alarms?

The patch looks fine to me.

Comment 20

[Mats Palmgren (inactive)]

(In reply to comment #17)
> but maybe it was just a newsgroup post David made?

I googled for it but could not find it...

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Does everyone else also get this many false alarms?

Yes.  There are a lot of tests which simply have nondeterministic frame bounding
boxes (generally this involves comboboxes).  So they flag a bunch of stuff as a
failure, and since its nondeterministic only some of them overlap with the
baseline....  :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed