96228 - Block elements with float style properties overwrite each other

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[tdowling]

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

[Boris Zbarsky [:bzbarsky]]

Over to layout.  ccing waterson.  Still a problem in linux build 2001-09-07-12

Comment 2

[Chris Waterson]

I'll take a first crack at this one...

Comment 3

[Chris Waterson]

Attachment: reporter's test case as an attachment

Comment 4

[Chris Waterson]

Attachment: minimized test case

Comment 5

[Chris Waterson]

Attachment: further minimized

Comment 6

[Chris Waterson]

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.

Comment 7

[Chris Waterson]

Attachment: frame and view dumps, with commentary

Comment 8

[Chris Waterson]

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.

Comment 9

[Chris Waterson]

Attachment: patch that ``fixes'' bug; for illustrative purposes only!

Comment 10

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

I agree with your diagnosis.

Comment 11

[Kevin McCluskey (gone)]

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?
~

Comment 12

[Chris Waterson]

> 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.)

Comment 13

[Chris Waterson]

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

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

> 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?

Comment 15

[Chris Waterson]

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.

Comment 16

[Chris Waterson]

Attachment: work-in-progress patch: remove recursion from PositionFrameView(); explicitly call PositionChildViews() when we're moving the frame after processing children

Comment 17

[Chris Waterson]

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).

Comment 18

[Chris Waterson]

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?

Comment 19

[Chris Waterson]

Doh! nsContainerFrame::PositionChildViews *already* makes use of the
NS_FRAME_HAS_CHILD_WITH_VIEW flag.

Comment 20

[Chris Waterson]

Attachment: Remove recursion from nsContainerFrame::PositionFrameView; explicitly position child views when moving a frame after Reflow; fix nsOuterTableFrame over-optimization

Comment 21

[Chris Waterson]

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

[karnaze (gone)]

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. 

Comment 23

[Chris Waterson]

Attachment: as above, with fixup for scrolled frames

Comment 24

[Chris Waterson]

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

[karnaze (gone)]

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. 

Comment 26

[Chris Waterson]

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

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

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.

Comment 28

[Chris Waterson]

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

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

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.

Comment 30

[Chris Waterson]

Attachment: as above, with fixup for <select>

Comment 31

[Chris Waterson]

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/[e-mail]> and at least start
evaluating some of the performance implications of our (possibly over-zealous)
post-reflow view positioning.

Comment 32

[Chris Waterson]

Attachment: as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic

Comment 33

[Chris Waterson]

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

[Kevin McCluskey (gone)]

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.

Comment 35

[Chris Waterson]

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

[karnaze (gone)]

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

[Marc Attinasi]

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

[Marc Attinasi]

Comment on attachment 49910 [details] [diff] [review]
as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic

sr'd

Comment 39

[Kevin McCluskey (gone)]

Comment on attachment 49910 [details] [diff] [review]
as above, but make SyncFrameViewAfterReflow call PositionFrameView instead of cut-n-pasting logic

r=[e-mail]

Comment 40

[Chris Waterson]

nsBoxToBlockAdaptor::PlaceChild() was dead code, so I just removed it. Thanks
for the reviews!

Comment 41

[Chris Waterson]

Fix checked in.

Comment 42

[Chris Waterson]

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

Comment 43

[Chris Waterson]

Reopening for consideration on the branch, re: bug 102235.

Comment 44

[Kevin McCluskey (gone)]

Marking nsbranch+. This fixes the bugscape 6989 regression
http://bugscape.netscape.com/show_bug.cgi?id=6989

Comment 45

[Kevin McCluskey (gone)]

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

[Jaime Rodriguez, Jr.]

Thanks for the update Kevin. I'll try and get a plus for this one @ noon today.

Comment 47

[Jaime Rodriguez, Jr.]

pls check this one in - PDT+

Comment 48

[Chris Waterson]

Fixed on MOZILLA_0_9_4_BRANCH.

Comment 49

[Chris Petersen]

Verified on the Oct 4th branch builds: Mac OS X (2001-04-04) and Windows ME
(10-04-06).

Comment 50

[Chris Petersen]

Verified on Linux Redhat 6.2 (2001-10-04-04)