263406 - [FIX]abs/fixed pos handling (placeholder, etc) needs consolidation

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We have the same code copied in N different places in the frame constructor to
check for abs/fixed pos, create a placeholder, and put things in the right lists.

This should be combined into a single function (probably on the constructor state).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for broken position:fixed root table

I "broke" fixed/absolute table roots with the checkin for bug 191151.  Will be
fixed in this bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: position:absolute root table

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Positioned table inside another table is broken

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase exercising random positioning stuff

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing fixed-pos not working when root is a table

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing fixed-pos block not working when root is a table

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161549 [details] [diff] [review]
Proposed patch

Asking for two reviews, mostly because this site is so fickle...

The changes in this patch are as follows:

1)  Eliminate the canBePositioned boolean in ConstructHTMLFrame in favor of
!important rules in ua.css.  Use this to simplify GetGeometricParent().

2)  Introduce an AddChild() function on the frame constructor state.  This puts
the new child in the right frame list based on its position/float values.

3)  Convert callers to use this function.  Fix them as needed to pass in the
right in-flow frameitems list. 

4)  Minor change to ConstructDocElementTableFrame to make it easier later on to
make it do sane things, hopefully.

For item 3, which is the meat of the patch, I had to mess a bit with
ConstructTableFrame and ConstructTableForeignFrame to make sure that the call
was happening at the place that had the right information.  I'm not completely
happy with the ConstructTableFrame changes; I would have preferred to have the
function simply put the frame in the right place (like
ConstructTableForeignFrame does), but I need the frame it returns in
ConstructFrameByDisplayType to set the primary frame map entry.  I suppose I
could use a boolean there to skip over the "add to frame lists" block,
though... then callers of ConstructTableFrame don't all have to mess with the
pseudo-frame lists like they do now.

The only other part of this patch that I'm not completely happy with is the
MathML code.  Here, I'm just not quite sure what the code wants to be doing, so
I just left it as-is.  I'll file a followup bug on rbs on it once this patch is
in, I guess.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note: this patch either fixes or doesn't regress any more all testcases attached
to this bug (mostly "fixes" the first two to show content again and really fixes
the third one).  I haven't run the regression tests on this yet, but I can do
that tomorrow if desirable.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161549 [details] [diff] [review]
Proposed patch

Actually, this diff has a few hunks that should not be in it...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for real

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161581 [details] [diff] [review]
Patch for real

David, it looks like roc's not going to get to this anytime too soon, so if you
could do r+sr....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161581 [details] [diff] [review]
Patch for real

Actually, this has one more issue... Before calling Destroy() on the frames on
failure I need to clean up their kids.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Latest-and-greatest

Comment 15

[Boris Zbarsky [:bzbarsky]]

That last patch asserts on http://www.mozilla.org/query.cgi
And on https://bugzilla.mozilla.org/query.cgi

I'll check those out tomorrow.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing broken handling of floated table inside table

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: With lots of testing and all

This patch fixes first, second, third, and seventh testcase on this bug, fixes
a crash in the sixth testcase, doesn't break the fourth, and doesn't make the
third any worse than it already is.

It also fixes the following testcases in the regression tests (to not assert
and to have correct layout):

http://www.hixie.ch/tests/adhoc/css/box/float/029.html
http://www.hixie.ch/tests/adhoc/css/box/float/029.xml
http://www.hixie.ch/tests/adhoc/css/box/float/030.html
http://www.hixie.ch/tests/adhoc/css/box/float/030.xml
http://www.hixie.ch/tests/adhoc/css/box/float/031.html
http://www.hixie.ch/tests/adhoc/css/box/float/031.xml

Finally, I've run the regression tests with this patch and there were no
problems (the mismatches were either due to bug 261605 being fixed by this
patch, due to the above bugs being fixed, or to noise).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162975 [details] [diff] [review]
With lots of testing and all

Robert, would you review?  Note that this diff includes the changes in bug
263846 -- I just didn't have enough trees around to keep the diffs separate...
so it may be easier to review that first...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch without changes from bug 263846

Diffed against current tip.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  This was a tiny codesize win, no noticeable perf impact so far.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I added the testcases attached to this bug to the regression tests.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Note: this caused regression bug 266850.

Comment 23

[Thomas O'Connor]

Can you please remove " ! important" from the float property of the legend
element in the forms.css file (change shown at
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/html/document/src&command=DIFF_FRAMESET&file=forms.css&rev2=3.92&rev1=3.91
]?

From what I have heard float wasn't causing the problems in the first place, and
it is the only way in which designers can break legend out of its rigid position.

Your consideration on this matter is appreciated.