Closed Bug 96228 Opened 23 years ago Closed 23 years ago

Block elements with float style properties overwrite each other

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.5

People

(Reporter: tdowling, Assigned: waterson)

References

()

Details

(Keywords: regression, Whiteboard: [PDT+] ETA: ready now. fix is on the trunk. Need on the 0.9.4 branch for bugscape 6989)

Attachments

(5 files, 5 obsolete files)

From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.3+) Gecko/20010820 BuildID: 2001082008 Starting with 0.9.3, Mozilla mis-renders content from floating DIV elements. It correctly renders the elements' boxes, but keeps overwriting content at the upper left of the window. Reproducible: Always Steps to Reproduce: Happens for me when a document has any block elements aligned with the CSS1 float property. Expected Results: Content for each floated block element should begin rendering where the box is created. This worked correctly for me in every build up through 0.9.2.
Over to layout. ccing waterson. Still a problem in linux build 2001-09-07-12
Assignee: pierre → attinasi
Status: UNCONFIRMED → NEW
Component: Style System → Layout
Ever confirmed: true
OS: Windows 2000 → All
QA Contact: ian → petersen
Hardware: PC → All
I'll take a first crack at this one...
Assignee: attinasi → waterson
Keywords: regression
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
Status: NEW → ASSIGNED
Attached file reporter's test case as an attachment (deleted) —
Attached file minimized test case (deleted) —
Attached file further minimized (deleted) —
It looks to me like all the frames and views have the right coordinates; however, the widget for the ScrollPortFrame _doesn't_. The problem appears to be that moving the view for the parent of the ScrollPortFrame (a GfxScroll, which doesn't itself have a widget) is not updating the position of any child widgetry.
Attached file frame and view dumps, with commentary (deleted) —
Okay, so I hacked nsView::MoveTo() such that a call on a view without a widget will recursively look for children _with_ widgets, and upon finding one, move that widget (and terminate the recursion). This appears to fix the problem reported in this bug, but is also causing other artifacts. Bonsai indicates that no serious work has gone on in mozilla/view/src, so either I'm not understanding the design of the view system (likely!), or this worked in mozilla-0.9.2 because it was masked by extra view moves that have since been optimized away.
This is an area where the view system relies on the frames to maintain consisten cy. During reflow the positioning of a view by it's associated frame synchroni zes the widget's position. It is assumed that if the view has descendants with w idgets there will be a frame associated with the descendant view which will rese t the views position which will cause it's associated widget to be re-positioned . I believe this was done for performance reasons early on in the implementation of the view system by michaelp. Views are positioned so frequently that having them walk the view hierarchy to reposition the widgets was thought to be expensi ve. We may need to special case the gfx scrollframe since it appears that this assum ption is broken. CC'ing evaughan creator/master of the gfxscrollframe. Eric also added the code w hich batches all of the widget changes. He was not able to turn on the widget ch ange batching because it caused anomalies. Eric, any ideas on this one? ~
> It is assumed that if the view has descendants with widgets there will be a > frame associated with the descendant view which will reset the views position > which will cause it's associated widget to be re-positioned. Okay, perhaps this is what nsContainerFrame::SyncFrameViewAfterReflow() is intended to do? Would we ever want to move a view _during_ reflow? (I.e., why are there calls to PositionFrameView() in reflow-related code?) Can this work in general? If a case exists where the position of a frame cannot be known until after the frame is flowed, this will break down. Specifically, we'd have to position the frame's view on the way _out_ of the Reflow() method, after we've visited its children (and would therefore need to revisit the children to synchronize their views). I'm not sure that this ever happens, though. What about the case where an incremental reflow causes more vertical space to be inserted somewhere in the middle of a block? This would imply that we'd need to visit the _entire_ frame tree after the insertion point, as opposed to just the top-level frames contained in the lineboxes following the insertion point. (Not optimal for typing performance; e.g.)
Never mind the comment about SyncFrameViewAfterReflow(). And it looks like almost everyone in core layout (except the absolute containing block reflow, which does it conditionally -- bug?) calls PositionFrameView() _before_ descending to its children to reflow. I don't understand box reflow well enough yet to ensure that this is the case for box. I'll poke around some more. Thanks for the tip, Kevin!
> If a case exists where the position of a frame cannot > be known until after the frame is flowed, this will break down. Off the top of my head, two such cases are vertical positioning of all inline content (which depends on other content in the line and the 'vertical-align' property) and horizontal and vertical positioning of floats (the position depends on the final size, which can vary if the float has 'auto' width). Isn't what we do now constantly reposition all the views in all the descendants every time we move anything?
Looks to be, although I think we get it wrong if the frame we're moving (via nsContainerFrame::PositionFrameView or nsContainerFrame::FinsihReflowChild) has a view. Specifically, we don't recur to the child views in these cases. (FWIW, I recently modified that code, but I believe it was broken in the same way before I changed it.) I'm not sure I believe that making the frames be responsible for this was a good trade-off. The view tree is typically trivial compared to the frame tree, and recursion can be terminated when we encounter a view with a widget. But that's probably food for another bug.
Attachment #49384 - Attachment is obsolete: true
Okay, I may have caused this regression with my changes for bug 55086. Even if not, fixing this is bound up with the fix for bug 99722. When fixing bug 55086, I misunderstood how views were placed, and assumed that placing a parent view was sufficient. It is only sufficient if it happens before we flow and place any of the child frames; if we need to place a view _after_ we've processed the children, then we'll need to revisit the children so that any widgets attached to those views get synchronized. I've attached work-in-progress patch that fixes this problem, as well as bug 99722, but will regress bug 55086. I'm still investigating to figure out why: the block reflow code calls nsContainerFrame::PositionFrameView() before reflowing the frame (so any children should be able to correctly update their positions). Furthermore, it calls nsContainerFrame::FinishChildReflow() after reflowing the frame (which ought to sync child views when the block frame is moved post-reflow).
Blocks: 99722
So, by dumb luck, I think that my first attempt at fixing bug 55086 (attachment 47466 [details] [diff] [review]) was probably the closest to being right. karnaze, it looks like the rule with views is that the container will place the child's view (by calling nsContainerFrame::PositionView) after moving the child's frame, but before calling the child's Reflow method. As kmcclusk describes, nsContainerFrame::PositionView will position _only_ the view associated with the child frame, under the assumption that descendant frames will also position their views to keep any widgetry synchronized. In bug 55086, the block frame was simply sliding the table vertically in response to the growth of the previous line. Knowing what I know now, I believe that the code in nsBlockReflowContext held up its part of the bargain: specifically, it positioned the view for the table frame (well, none existed), and then called the table frame's Reflow method. The real problem was that the outer table frame over-zealously left Reflow without recursively synchronizing its own views. I think we ought to change nsOuterTableFrame::Reflow to call nsContainerFrame::PositionChildViews in the ``fast path'' case where it detects that the table's width has not changed. To mitigate any performance issues (nsContainerFrame::PositionChildViews currently crawls the frame's entire subtree!), we probably ought to make use of the NS_FRAME_HAS_CHILD_WITH_VIEW flag in that routine to avoid descending into subtrees without views. Patch anon: what do you think?
Doh! nsContainerFrame::PositionChildViews *already* makes use of the NS_FRAME_HAS_CHILD_WITH_VIEW flag.
Attachment #49426 - Attachment is obsolete: true
This patch isn't quite right. I'm noticing some anomalies with scrolled <textarea> frames (e.g., on long comments in bugzilla). Probably need to restore some of the code in nsBox::SetBounds...
waterson, I still don't think it is correct to always call PositionChildViews in a frame's (eg. outer table frame) reflow method. I think if you call nsContainerFrame::FinishReflowChild and don't set NS_FRAME_NO_MOVE_VIEW in aFlags, that will accomplish the same thing. This also handles the cases where a frame cannot know the position of a child until after it reflows the child.
Attachment #49457 - Attachment is obsolete: true
Attached patch as above, with fixup for scrolled frames (obsolete) (deleted) — Splinter Review
This patch is still not complete: I'm noticing some artifacts around comboboxes and selects; however, it does fix the problem that I was seeing typing into <textarea>s. That problem was as follows. nsBlockFrame::PostPlaceLine is repositioning the view for _each_ frame in the line. I've now made nsContainerFrame::PositionChildViews unconditionally descend into children (recall that before it was stopping at the first view it encountered during the recursive traversal). Because we now descend into scrolled views, the call to nsViewManager::MoveViewTo() would reset the scrolled child's position to the frame's offest. The fix is to detect that the parent view is scrollable, and adjust the origin accordingly. karnaze: where do you propose that we call nsContainerFrame::FinishReflow? N.B. that the block code _is_ properly calling this after reflowing the table, and this (when appropriate) will recursively update the positions of any child views *if the child frame's coordinates change after we Reflow it*. nsBlockReflowContext::ReflowBlock is properly synchronizing the table's view with the table's frame's position before calling Reflow on the table. According to the contract kmcclusk describes, it is the onus of the *table* to reposition any views in its cells. As an aside, I'm starting to wonder if the cure is worse than the disease now, and phear the performance implications of this change. Certainly this appears to normalize what appears to have been somewhat schizophrenic code. AFAICT, I'm not the only one who thought that positioning a view was sufficient to properly synchronize all of the view's children, regardless that this was never the intent. OTOH, the fact that nsBlockFrame::PostPlaceLine completely negates this optimization by requiring recursive fixup makes me wonder if the optimization is really worth while.
It is very common to call ReflowChild, do some stuff, call FinishReflowChild. I admit that we usually try to set NS_FRAME_NO_MOVE_VIEW for performance reasons, but in your case involving the table frame it seems that the same amount of work will be done (when the table really moves, that is) either way. I even wonder why the block is reflowing the table with exactly the same available size as last time. Ususally a frame needs to be reflowed when something changes, not when it only needs to be moved. I think we should have a frame/view discussion and include kmcclusk. I suspect that we are repositioning views redundantly all over the place, and the real costs are a mystery to me.
WRT reflowing table just to slide it vertically: I don't think that the block code distinguishes between a line that is dirty because its dimensions have changed, and a line that is ``dirty'' because its position has changed. But I may have overlooked optimizations that would handle this case.
Block code does distinguish between reflowing something and sliding it (see nsBlockFrame::SlideLine, currently called from nsBlockReflowState::RecoverStateFrom, although that will change with my patch for bug 86947). However, there are plenty of cases where we mark things dirty because something *might* have changed.
But doesn't that only happen during incremental reflow state recovery? Or can it happen to lines that occur ``after'' a dirty line as well?
We should continue to do state recovery after a dirty line, as long as the lines after it don't get marked dirty. (I should try some experiments sometime after I land bug 86947.) And, FWIW, my patch also adds a MarkTopMarginDirty (essentially, marking the position dirty) -- since the old code assumed the top margin was *always* dirty.
Attachment #49660 - Attachment is obsolete: true
Attached patch as above, with fixup for <select> (obsolete) (deleted) — Splinter Review
Allright, attachment 49866 [details] [diff] [review] finally gets it right (knock on wood). The artifacts I was seeing in <select> elements had to do with the fact that the math to fix up ``anonymous'' views (inserted by nsScrollFrame) was backwards. This was probably never noticed because the anonymous views weren't being synchronized with their frames until somebody actually _scrolled_ the frame, at which point the ScrollTo() logic would clobber the bogus offset. I'd still like to - Run some performance tests to see if this makes any noticable difference - See if I can clean up the logic that's duplicated between nsContainerFrame::SyncFrameViewAfterReflow and nsContainerFrame::PositionFrameView. - See if I can move the hack to deal with nsScrollFrame out of nsContainerFrame. After that, I'll get some bugs filed as per <news://news.mozilla.org/3BA7AA47.10502@netscape.com> and at least start evaluating some of the performance implications of our (possibly over-zealous) post-reflow view positioning.
Attachment #49866 - Attachment is obsolete: true
Allright, I wimped out on getting rid of the goofiness in PositionFrameView, but at least got rid of the copy-n-paste logic there. Could people take a look at attachment 49910 [details] [diff] [review] with an eye on review? To summarize: 1. Remove recursion from PositionFrameView(), it now positions _only_ the view for its frame argument. 2. Modify callers that position views after Reflow() to recursively position child views. 3. Account for scrolled views in PositionFrameView, a bug that was unmasked by our more aggressive after-reflow positioning. 4. Correctly account for native nsScrollViews, which insert an ``anonymous'' view (as I noted above, the math was backwards). 5. Re-fix bug 55086 by making the table call PositionChildViews in its fast path reflow case. (I know this makes karnaze cringe.) I've run iBench (jrgm's tests weren't producing all NaN's for me last night), and didn't notice any speedup or slowdown.
Keywords: patch
Chris: since you removed the (containingView != parentView) logic which explicitly positions the combobox dropdown's view you should make sure that combo box drop's appear in the correct position. You need to test cases where the combobox can not be popped down and must be popped up (This happens when the combobox is at the bottom of the window.) Rod do you have other combobox test cases that Chris should look at that force positioning of the dropdown in a non-standard position. All of the other view changes look fine with me.
That logic is still there: the code in SyncFrameViewAfterReflow was redundant with PositionFrameView, so I just made the one call the other. Nevertheless, any funky combobox test cases would be appreciated!
waterson, the 2 table changes look ok to me, but at some point, I'll need to revisit the issue of table's calling PositionChildViews too often.
Comment on attachment 49910 [details] [diff] [review] as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic sr=attinasi, but I'm curious why the BoxToBlockAdaptor::PlaceChild method was removed?
Comment on attachment 49910 [details] [diff] [review] as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic sr'd
Attachment #49910 - Flags: superreview+
Comment on attachment 49910 [details] [diff] [review] as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic r=kmcclusk@netscape.com
Attachment #49910 - Flags: review+
nsBoxToBlockAdaptor::PlaceChild() was dead code, so I just removed it. Thanks for the reviews!
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 102235 has been marked as a duplicate of this bug. ***
Reopening for consideration on the branch, re: bug 102235.
Status: RESOLVED → REOPENED
Keywords: nsbranch
Resolution: FIXED → ---
Marking nsbranch+. This fixes the bugscape 6989 regression http://bugscape.netscape.com/show_bug.cgi?id=6989
Whiteboard: ETA: ready now. fix is on the trunk. Need on the 0.9.4 branch for bugscape 6989
PDT: the fix for bug 96228 has been on the trunk for about a week now (since 9/20/2001), and has not caused any known regressions.
Keywords: nsbranchnsbranch+
Thanks for the update Kevin. I'll try and get a plus for this one @ noon today.
pls check this one in - PDT+
Whiteboard: ETA: ready now. fix is on the trunk. Need on the 0.9.4 branch for bugscape 6989 → [PDT+] ETA: ready now. fix is on the trunk. Need on the 0.9.4 branch for bugscape 6989
Fixed on MOZILLA_0_9_4_BRANCH.
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Verified on the Oct 4th branch builds: Mac OS X (2001-04-04) and Windows ME (10-04-06).
Keywords: vtrunk
Verified on Linux Redhat 6.2 (2001-10-04-04)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: