regressed from reflow branch landing. text on the input box disappeared when history on sidebar is open. the text will disappeared only when there is a long text on the <p> about the <input> tag. steps to reproduce: 1. load testcase 2. press ctrl+h to load history on sidebar 3. if you can't reproduce this bug, try to adjust the width (wider) of the sidebar.

Thanks for the testcase, but this is a testcase where you don't need to tamper with the history bar or change the window size.

This patch fixes it for me. Without it, the view of the scrollframe inside the textcontrol doesn't get repositioned when we go from two lines of text to one line of text and consequently slide up the table outer frame, because the block line in question is dirty and everyone seems to assume that _someone_ else will reposition views in that case... or something. I'm really not sure how this used to work before reflow branch.

It used to work via nsBlockFrame::SlideLine calling ::PlaceFrameView() from here, right? http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#2380

I've tried the patch, and it also seems to fix bug 355413, which is also a branch problem. So if the patch is any good, an can be applied on branch, it might be a good idea to ask for branch approval.

SlideLine isn't being called here, though, since the line is dirty. Perhaps it didn't use to be dirty... or something.

So the comments there seem to suggest that that position isn't even certain to be the final position, although perhaps that's not true anymore. (The case I can think of where it could be the case is with blocks that flow around floats.) I think part of the theory of what was supposed to happen here is that, since the descendants were being reflowed, any descendant that wasn't dirty should have views within it repositioned. It may have worked pre-reflow-branch because I added incremental layout handling to a frame class that didn't have it before, and didn't write the correct "not dirty" codepath (which probably needs to be put in one place and called from the rest -- I know there are multiple copies of it in the table code, although they differ slightly for good reason -- basically differing between what would be called SlideTo and SlideBy).

For what it's worth, a problem with this patch is that I think it makes the view repositioning needed to reposition the view for a dirty single element with a view nested within blocks O(N^2) in that element's depth within the tree.

So who's responsibility is it to reposition views? It seems to me that whoever changes the position of a frame should handle that, since otherwise a frame has no way of knowing when it's been moved...

OK, so I did a bit more reading. The comments in nsIFrame, under "new rules of reflow", pretty much cover this case. The relevant comments are: * 4. if you move a frame (outside of the reflow process, or after reflowing it), * then you must make sure that its view (or its child frame's views) are re-positioned * as well. It's reasonable to not position the view until after all reflowing the * entire line, for example, but the frame should still be positioned and sized (and * the view sized) during the reflow (i.e., before sending the DidReflow() notification) This shouldn't be all that bad, I'd think...

This uses the final block position, and only repositions if the position has changed. That should address the concerns raised, no?

I swear, I'm going to remove views for 1.9 or die trying!

This makes nsIFrame::GetOffsetTo just not use views (at least not as obviously); that "fixes" this bug (since now all the coords end up correct) without actually fixing the view bogosity. If we're removing views anyway, maybe this is the way to go.

I don't think it's yet safe in general to allow views to have bogus geometry.

Comment on attachment 251007 [details] [diff] [review] Better fix, I think r+sr=dbaron, although I'm not a fan of the assertion at the start (feel free to drop it). It seems like we might want the other patch too. Do you want to ask Eli or roc to review it? Or is it obsolete by now?

I filed bug 376662 on the GetOffsetTo change. It's not quite ready to go in yet... we have places where the frame geometry is wrong (popups, to be exact).

Fixed. Not quite sure how to test this....