Closed
Bug 135082
Opened 23 years ago
Closed 21 years ago
[FIXr]{ib}rel pos inlines not leading to right containing block for abs pos kids
Categories
(Core :: Layout: Positioned, defect, P1)
Core
Layout: Positioned
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: florian.hartig, Assigned: bzbarsky)
References
(Blocks 3 open bugs, )
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
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.
Assignee: rogerl → jst
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM Style
Ever confirmed: true
QA Contact: pschwartau → ian
Assignee | ||
Comment 2•23 years ago
|
||
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.
Assignee: jst → attinasi
Component: DOM Style → Layout
OS: Windows 2000 → All
QA Contact: ian → petersen
Hardware: PC → All
Summary: document.getElementById(elem).style.display can't be switched → {ib}document.getElementById(elem).style.display can't be switched
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Assignee | ||
Comment 5•22 years ago
|
||
This is a problem with reflow of abs pos elements with auto top but _not_ auto
bottom which are inside a rel pos inline...
Assignee | ||
Comment 6•22 years ago
|
||
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).
Attachment #96949 -
Attachment is obsolete: true
Assignee | ||
Comment 7•22 years ago
|
||
David, what do you think? Is there a reasonable way we could use the right
containing block here?
Comment 8•22 years ago
|
||
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
Assignee | ||
Comment 9•22 years ago
|
||
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?
Assignee | ||
Updated•22 years ago
|
Summary: {ib}document.getElementById(elem).style.display can't be switched → {ib}rel pos inlines not leading to right containing block for abs pos kids
Assignee | ||
Comment 10•22 years ago
|
||
.
Assignee: waterson → position
Status: ASSIGNED → NEW
Component: Layout → Layout: R & A Pos
Priority: P4 → P2
QA Contact: moied → ian
Target Milestone: Future → ---
Updated•21 years ago
|
Target Milestone: --- → Future
Comment 11•21 years ago
|
||
*** Bug 78880 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 12•21 years ago
|
||
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.
Assignee | ||
Comment 13•21 years ago
|
||
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.
Assignee | ||
Comment 14•21 years ago
|
||
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?
Attachment #142550 -
Flags: superreview?(dbaron)
Attachment #142550 -
Flags: review?(dbaron)
Assignee | ||
Comment 15•21 years ago
|
||
Attachment #142550 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #142622 -
Flags: superreview?(dbaron)
Attachment #142622 -
Flags: review?(dbaron)
Assignee | ||
Updated•21 years ago
|
Attachment #142550 -
Flags: superreview?(dbaron)
Attachment #142550 -
Flags: superreview-
Attachment #142550 -
Flags: review?(dbaron)
Attachment #142550 -
Flags: review-
Comment 16•21 years ago
|
||
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•21 years ago
|
||
(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•21 years ago
|
||
(I'm not going to be able to review this before the freeze -- it looks quite
complicated.)
Comment 19•21 years ago
|
||
(And, that said, if I could, I'm not sure I'd want it to go in before the freeze.)
Assignee | ||
Comment 20•21 years ago
|
||
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...
Attachment #142622 -
Flags: superreview?(dbaron)
Attachment #142622 -
Flags: superreview-
Attachment #142622 -
Flags: review?(dbaron)
Attachment #142622 -
Flags: review-
Assignee | ||
Comment 21•21 years ago
|
||
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).
Assignee | ||
Updated•21 years ago
|
Attachment #142622 -
Attachment is obsolete: true
Assignee | ||
Comment 22•21 years ago
|
||
Attachment #143908 -
Attachment is obsolete: true
Assignee | ||
Comment 23•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #143962 -
Flags: superreview?(dbaron)
Attachment #143962 -
Flags: review?(dbaron)
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.
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? :-)
Assignee | ||
Comment 26•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #143962 -
Flags: superreview?(dbaron)
Attachment #143962 -
Flags: superreview+
Attachment #143962 -
Flags: review?(dbaron)
Attachment #143962 -
Flags: review+
Assignee | ||
Comment 28•21 years ago
|
||
> 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...
Assignee: core.layout.r-and-a-pos → bzbarsky
Priority: P2 → P1
Summary: {ib}rel pos inlines not leading to right containing block for abs pos kids → [FIXr]{ib}rel pos inlines not leading to right containing block for abs pos kids
Target Milestone: Future → mozilla1.8alpha
Assignee | ||
Comment 29•21 years ago
|
||
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)).
Attachment #143962 -
Attachment is obsolete: true
Assignee | ||
Comment 30•21 years ago
|
||
Checked in. Marking fixed, though the containing block is not quite "right"
still. Bug 5016 should track the remaining work.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•