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)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: florian.hartig, Assigned: bzbarsky)

References

(Blocks 3 open bugs, )

Details

Attachments

(3 files, 5 obsolete files)

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...
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
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
Taking.
Assignee: attinasi → waterson
Changing QA contact
QA Contact: petersen → moied
Priority: -- → P4
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Attached file Testcase (obsolete) (deleted) —
This is a problem with reflow of abs pos elements with auto top but _not_ auto bottom which are inside a rel pos inline...
Attached file Better testcase (deleted) —
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
David, what do you think? Is there a reasonable way we could use the right containing block here?
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
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?
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
Blocks: 71150
Blocks: 78880
.
Assignee: waterson → position
Status: ASSIGNED → NEW
Component: Layout → Layout: R & A Pos
Priority: P4 → P2
QA Contact: moied → ian
Target Milestone: Future → ---
Blocks: 93060
Target Milestone: --- → Future
Blocks: 210746
Blocks: 213556
Blocks: 202068
Blocks: 224832
Blocks: 225018
Blocks: 112015
Blocks: 225265
Blocks: 148095
Blocks: 231026
Blocks: 235460
*** Bug 78880 has been marked as a duplicate of this bug. ***
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.
Attached patch Partial fix (obsolete) (deleted) — Splinter Review
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 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)
Blocks: 5016
Attached patch Oops, I had a sign off (obsolete) (deleted) — Splinter Review
Attachment #142550 - Attachment is obsolete: true
Attachment #142622 - Flags: superreview?(dbaron)
Attachment #142622 - Flags: review?(dbaron)
Attachment #142550 - Flags: superreview?(dbaron)
Attachment #142550 - Flags: superreview-
Attachment #142550 - Flags: review?(dbaron)
Attachment #142550 - Flags: review-
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).
(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.)
(I'm not going to be able to review this before the freeze -- it looks quite complicated.)
(And, that said, if I could, I'm not sure I'd want it to go in before the freeze.)
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-
Blocks: 236966
Attached file Another testcase (obsolete) (deleted) —
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).
Attachment #142622 - Attachment is obsolete: true
Attached file Real another testcase (deleted) —
Attachment #143908 - Attachment is obsolete: true
Attached patch Patch updated to comments (obsolete) (deleted) — Splinter Review
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.
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? :-)
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.
Blocks: 157526
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+
> 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
Attached patch Patch that I checked in (deleted) — Splinter Review
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
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
Depends on: 300816
Depends on: 419072
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: