191151 - [FIXr]Make nsCSSFrameConstructor::GetGeometricParent()

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P2)

Description

[John Keiser (jkeiser)]

There are a number of places in nsCSSFrameConstructor that get the "geometric
parent" of a frame--the parent frame in the visual hierarchy rather than the
logical content hierarchy.  Nearly all of them do this in exactly the same way.
 It should be factored out into GetGeometricParent().

Comment 1

[John Keiser (jkeiser)]

Attachment: Patch v0.5

This patch runs and builds happily and survives under my typical mail / web
daily usage.  However, it does change one thing: many places that were
calculating the geometric parent were not taking floating into account with the
geometric parent, so I made them do that.  However, I do not yet have
sufficient knowledge of the intricacies of floating to construct testcases that
may fail when this happens.  troy checked in the fix that made most elements do
this, and indicated that (at least at that time) there was an actual problem. 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp&rev=1.156#2665


Two things I'd like to do as well:
- stop passing around aIsAbsolutelyPositioned and aIsFixedPositioned--maybe
even stop grabbing them in the main function if it turns out to be useful
- make nsIFrame have virtual method(s) to determine whether the frame supports
positioning / floating--that information does *not* belong in
nsCSSFrameConstructor where it currently resides (canBePositioned).

Testcases for the above problem or hints on how to make them are appreciated.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I was about to file this bug.  Taking.  I'll attach a slew of testcases for the
various places in CSSFrameConstructor that get this wrong... John's patch is a
good place to start.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Floated blocks with overflow inside inlines don't work right...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fieldsets and selects manage to work somehow, but I'm not sure how.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Alex, would you review the SVG changed?  David, would you review the rest?

I'm not set on the svg stuff, since my changes may have some memory/perf
impact... so if people don't like it, I'll revert it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

ccing the svg folks and rbs (for the mathml changes, but they were
non-substantive, except for floated mathml).

Comment 8

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

I can review this if David wants to delegate it to me.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Actually, the svg changes are wrong, because we may end up with an <svg> inside
HTML inside a <foreignobject>.	Fixed patch coming up in a sec.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that doesn't change SVG behavior

The only change this makes to svg is to make it possible to float or
fixed-position "outermost" <svg> elements.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160154 [details] [diff] [review]
Patch that doesn't change SVG behavior

Pinging roc for review, since he volunteered (and since there are no longer
substantive SVG changes here, r+sr should be ok, I think).

Comment 12

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

Comment on attachment 160154 [details] [diff] [review]
Patch that doesn't change SVG behavior

+  // If there is no container for a fixed, absolute, or floating table because
+  // it is the root content frame, we will ignore the positioning.

Rephrase this to make it clear that as implemented, this hack applies to all
elements, not just tables.

+    // XXXbz roc says stacks do, or should....

Take this out since I'm not sure...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk for 1.8a5.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note: this broke absolute positioning of tables.  See bug 262898 (which has a fix).