135082 - [FIXr]{ib}rel pos inlines not leading to right containing block for abs pos kids

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P1)

Description

[Florian Hartig]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031106

At the URL entered above there should be a "drop up" menu when you point the
mouse over the text items on the grey bar. To do this I tried to use
document.getElementById(elem).style.display = 'block'; where elem has been set
to display : none via css before.
IMHO the method I used is standardized and so this might be a bug.

Reproducible: Always
Steps to Reproduce:
1. Point your browser to the URL given above
2. Move the mouse over the menu items
3. See absolutely nothing happen

Actual Results:  Nothing - in fact that's the problem

Expected Results:  A menu should have popped up right above the menu item your
mose is over

The image used on my example page is intentionally non-existant...

Comment 1

[Phil Schwartau]

Browser, not engine. Reassigning to DOM Style; cc'ing Boris.

Confirming with Mozilla trunk build 20020330xx WinNT.

As the reporter states, he is doing this:

       document.getElementById(elem).style.display = 'block';

where elem has been set to display : none via css before.


The JavaScript Debugger shows the value 'block' is successfully
assigned in the DOM, but there is no visual response in the browser.

You can experiment by loading the URL and just using this javascript:

 javascript:void(document.getElementById('menu1').style.display='block');

In IE6 it brings up the menu; in Mozilla, it does not.
Note: no errors in the JS Console.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is the ".menu { display: inline }" rule in the stylesheet.  This
means that the outer menu boxes are inline boxes, whereas the inner boxes (the
ones whose display property is getting toggled) are block boxes.

This puts us into a block-in-inline situation, which is, as usual, very screwy.
 There're various hacks in the CSS frame constructor to handle this, but it
looks like none of them are helping in this case.... over to layout.

Comment 3

[Chris Waterson]

Taking.

Comment 4

[Christine Hoffman]

Changing QA contact

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This is a problem with reflow of abs pos elements with auto top but _not_ auto
bottom which are inside a rel pos inline...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Better testcase

This makes it obvious that the problem is that we are using the parent
blockframe of the rel pos inline as the containing block. This happens to have
an unconstrained height, so we get what we see here (the child div of the
leftmost menu1 is just 7*1e7 pixels down from its parent).

Comment 7

[Boris Zbarsky [:bzbarsky]]

David, what do you think?  Is there a reasonable way we could use the right
containing block here?

Comment 8

[Marek Stępień [:marcoos, inactive]]

I have been experiencing the same problem.

After invoking document.getElementById(its_id).style.display='none'
it is switched to none (I checked that with
alert(document.getElementById(its_id).style.display) and it showed "none") but
it is still visible.

My example page for this bug can be found at:
http://www.marcoos.zwm.punkt.pl/bugzilla/mozilla-style-display.html

Comment 9

[Boris Zbarsky [:bzbarsky]]

Marek, I see no relatively posisioned inline elements on your page.  Therefore
it's a different bug (unless I just missed them).  Please file a bug and cc me, ok?

Comment 10

[Boris Zbarsky [:bzbarsky]]

.

Comment 11

[Benjamin Smedberg]

*** Bug 78880 has been marked as a duplicate of this bug. ***

Comment 12

[Boris Zbarsky [:bzbarsky]]

The problems first start when we use the parentReflowState->mCBReflowState as
the containing block in nsHTMLReflowState::InitConstraints...  The problem is
that a rel pos inline is not ever a containing block except for abs pos frames.

The more I think about it, the more implementing this sucks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial fix

This makes mCBReflowState be the containing block of _this_ frame, not of the
parent.  That allows us to adjust mCBReflowState for abs pos elements as
needed...

The rest is mostly dealing with fallout and fixing some other random bugs in
ComputeHypotheticalBox.

This still has one problem -- placeholders are not yet placed in
nsPositionedInlineFrame::Reflow when it goes to reflow the abs pos kids.  I'm
not quite sure how to deal with that, and as a result auto offsets on abs pos
things that were initially inlines don't work quite right when the cb is rel
pos.

This patch fixes most of the issues in this bug and its dependancies, other
than that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142550 [details] [diff] [review]
Partial fix

David, what do you think?  This is enough of an improvement to be worth
checking in, imo (for one thing, no more NS_UNCONSTRAINEDSIZE being used as an
offset....)  Does the general approach seem reasonable?  Should I just remove
the copy constructor?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops, I had a sign off

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you run the regression tests?

You should just remove the copy-constructor and copy-assignment operator and
comment in the header file that the compiler-generated ones are OK (or use the
comment from revision 3.31 of the header file).

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, I never liked the way mCBReflowState worked -- this is much better.  But
when I fixed bug 143706, the current way was a much easier way to do it.)

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm not going to be able to review this before the freeze -- it looks quite
complicated.)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And, that said, if I could, I'm not sure I'd want it to go in before the freeze.)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142622 [details] [diff] [review]
Oops, I had a sign off

Yeah, this is alpha-fodder at this point.  I'll run the regression tests.  I
also just caught another sign error; I'll give the whole patch another
once-over...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase

This patch doesn't fix that testcase... the problem is that the offsets are
measured differently if the containing block is a positioned inline....  I
probably need to handle this by munging the offsets for cases when that happens
(that is, having mComputedOffsets be the offsets from the padding box as they
are for a block-level containing block and adusting absolutely specified
offsets).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Real another testcase

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

OK, I removed the constructor and fixed the sign errors and the behavior on
that last testcase I attached.

I also ran the regression tests.  It seems that doing so now requires
approximately 1.7 GB of hard drive space (!) for the rgd files and that some of
Ian's bogus char tests really push the limits of memory usage on my machine
(with 784 MB of RAM).  Also, one of Ian's tests opens a popup window (we should
probably figure out which and remove it).

Anyway, this patch passes.  The only testcases it really changes are some of
Ian's abs-pos-in-rel-pos-inline tests, all for the better.

This patch still isn't quite perfect, because auto offset handling is a bit off
(see XXX comments in the patch), but I'd like to work on that in a separate
patch.

Comment 24

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

I tried this patch and it fixed the testcases in bug 224832. Great!

It doesn't fix the scenario I outlined here:
http://bugzilla.mozilla.org/show_bug.cgi?id=237343#c11
That'll be rather hard to fix, but it should wait until after this lands, for sure.

Also, 'right' and 'bottom' should be relative to the edges of the last-in-flow
frame for the element, but aren't ... but I guess that's what you meant by this:
+      // XXXbz we should be taking the in-flows into account too, but
+      // that's very hard.

Comment 25

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

One way to deal with the last-in-flows and the justification problem would be to
delay reflowing absolute children of positioned inlines until the end of the
reflow of the block containing those inlines.

Although, hey, what if the inlines span a page boundary? :-)

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yeah, the page boundary case is exactly why I didn't even bother trying to
address the in-flow problems for inlines.  Between that and the fact that
depending on how line-breaking works you could have your abs pos element's
containing block width vary by the width of the browser window, I still feel
that what CSS specifies here makes absolutely no sense and that time would be
better spent making the spec sane (and interoperably implementable) rather than
trying to implement it as-is.

As for the issue in bug 237343 comment 11, perhaps that is in fact the right
place to hook into the "post-normal-reflow" layout of our abs pos kids... that
may also fix the XXX comment in my patch about placeholders not being
reflown/positioned yet when we hit CalculateHypotheticalBox.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143962 [details] [diff] [review]
Patch updated to comments

>+    NS_ASSERTION(NS_FRAME_GET_TYPE(mFrameType) != NS_CSS_FRAME_TYPE_ABSOLUTE ||
>+                 mCBReflowState->mStyleDisplay->IsPositioned(),
>+                 "Absolutely positioned frame must have positioned containing block!");

It's not obvious to me what prevents this assertion from firing all the time. 
Should there be a precondition about parentReflowState?  Other than that,
r+sr=dbaron.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Should there be a precondition about parentReflowState?

There is an early return out of this function if parentReflowState is null... 
So by the time we get to that assert we know that we _do_ have a parent reflow
state.  The assert would fire if we have a frame with a display struct that
returns true for IsAbsolutelyPositioned() and has a parent whose display struct
doesn't return true for IsPositioned().  Come to think of it, that may happen
for fixed-pos frames; I'll look into that.  But except for that case, I think
this assert is correct...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that I checked in

I just removed the assert; it was firing for fixed-pos frames and for abs pos
table frames (since the containing block was the outer frame, which was not
positioned) and could have possibly fired in some other edge cases like that...
I decided that it wasn't worth trying to figure out when it should "really"
fire.

I also added two lines to enforce that the computed width/height of an abs pos
frame is nonnegative (it was ending up negative if it was auto and the offsets
were non-auto and "left + right" (respectively "top + bottom") was bigger than
the available width (respectively height)).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in. Marking fixed, though the containing block is not quite "right"
still.  Bug 5016 should track the remaining work.