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)
Core
Layout
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)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
kmcclusk
:
review+
attinasi
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•23 years ago
|
||
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
Assignee | ||
Comment 2•23 years ago
|
||
I'll take a first crack at this one...
Assignee: attinasi → waterson
Keywords: regression
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
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.
Assignee | ||
Comment 7•23 years ago
|
||
Assignee | ||
Comment 8•23 years ago
|
||
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.
Assignee | ||
Comment 9•23 years ago
|
||
I agree with your diagnosis.
Comment 11•23 years ago
|
||
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?
~
Assignee | ||
Comment 12•23 years ago
|
||
> 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.)
Assignee | ||
Comment 13•23 years ago
|
||
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!
Comment 14•23 years ago
|
||
> 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?
Assignee | ||
Comment 15•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #49384 -
Attachment is obsolete: true
Assignee | ||
Comment 17•23 years ago
|
||
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
Assignee | ||
Comment 18•23 years ago
|
||
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?
Assignee | ||
Comment 19•23 years ago
|
||
Doh! nsContainerFrame::PositionChildViews *already* makes use of the
NS_FRAME_HAS_CHILD_WITH_VIEW flag.
Assignee | ||
Updated•23 years ago
|
Attachment #49426 -
Attachment is obsolete: true
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
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...
Comment 22•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Attachment #49457 -
Attachment is obsolete: true
Assignee | ||
Comment 23•23 years ago
|
||
Assignee | ||
Comment 24•23 years ago
|
||
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.
Comment 25•23 years ago
|
||
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.
Assignee | ||
Comment 26•23 years ago
|
||
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.
Comment 27•23 years ago
|
||
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.
Assignee | ||
Comment 28•23 years ago
|
||
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?
Comment 29•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Attachment #49660 -
Attachment is obsolete: true
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Attachment #49866 -
Attachment is obsolete: true
Assignee | ||
Comment 32•23 years ago
|
||
Assignee | ||
Comment 33•23 years ago
|
||
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.
Comment 34•23 years ago
|
||
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.
Assignee | ||
Comment 35•23 years ago
|
||
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!
Comment 36•23 years ago
|
||
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 37•23 years ago
|
||
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 38•23 years ago
|
||
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 39•23 years ago
|
||
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+
Assignee | ||
Comment 40•23 years ago
|
||
nsBoxToBlockAdaptor::PlaceChild() was dead code, so I just removed it. Thanks
for the reviews!
Assignee | ||
Comment 41•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 42•23 years ago
|
||
*** Bug 102235 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 43•23 years ago
|
||
Reopening for consideration on the branch, re: bug 102235.
Comment 44•23 years ago
|
||
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
Comment 45•23 years ago
|
||
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.
Comment 46•23 years ago
|
||
Thanks for the update Kevin. I'll try and get a plus for this one @ noon today.
Comment 47•23 years ago
|
||
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
Assignee | ||
Comment 48•23 years ago
|
||
Fixed on MOZILLA_0_9_4_BRANCH.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 49•23 years ago
|
||
Verified on the Oct 4th branch builds: Mac OS X (2001-04-04) and Windows ME
(10-04-06).
Comment 50•23 years ago
|
||
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.
Description
•