Steps to reproduce: - Turn on XBL form controls by setting the pref "nglayout.debug.enable_xbl_forms" to true in all.js - Go to a page with a listbox, such as http://bugzilla.mozilla.org/query.cgi - Scroll one of the listboxes down. Note that the entire listbox is invalidated (easiest to see with paint flashing turned on).

Well, if we never set |forceInvalidate| during an incremental reflow, we won't force the text frame to reflow. I'm now trying to figure out why we thought we needed |forceInvalidate|. Cursory use (including attaching this patch) shows no artifacts from its removal.

I meant to say, ``...we'll never force the select frame to invalidate.''

Okay, removal of this code regresses bug 2222: we leave cruft after pulling lines up.

So I *think* that the problem here is a sub-optimal invalidate implementation in nsBlockFrame::ReflowLine. Specifically, if |aDamageDirtyArea| is set, we'll _union_ the line's old combined area with its new combined area and invalidate the entire thing. I think what we want to do here is take the _difference_ of the old combined area and the new combined area (i.e., old combined area - new combined area), and invalidate _that_. IOW, if the new combined area is smaller than the old combined area, then the line shrank, and we need to invalidate the portions of the old combined area outside the shrinkage. This fixes bryner's problem without regression bug 2222.

cc'ing block folks. What do you think?

Comment on attachment 65130 [details] [diff] [review] less of a hack Seeing some weirdness in plaintext editor.

Specifically, in mail-news compose.

Well, the above patch ain't right. After digging a bit futher, I'm now realizing that the block-and-line's design is based on doing the invalidation at the line-level. More or less, the frame classes don't do any of their own invalidation during reflow. (Text frame is an exception, but I think that's been hacked -- poorly, too. The invalidation area is always empty during the text frame's initial reflow. During an incremental reflow, the invalidation area doesn't include the frame's x and y coordinates: for continuations, that code is likely to be invalidating some other random area of the screen.) Doing invalidation at the line level seems fairly reasonable: doing it at a finer grain could be complicated, and would probably result in more invalidates being issued. So I'm gonna try barking up a different tree for a bit. Specifically, why does nsBoxFrame need to call ReflowDirtyChild?

> Text frame is an exception, but I think that's been hacked -- poorly, too. I happened to have hunted for some up pointers about this on bug 103266. (At the time, I was curious about the textarea that sometimes doesn't repaint.)

Mmm, I bet _everyone_ is excited to see me cough up a large patch to block & line two weeks before I go on sabbatical. :-) I think I'm getting warmer to fixing bryner's problem. Here's what this patch does: 1. Alters nsBox so that it never calls ReflowDirtyChild on its parent. Instead, it schedules a dirty reflow (or style change reflow), with itself as the target. 2. Fixes block so that we are more conservative about force-invalidating a dirty line during an incremental reflow. Specifically, a block will now only force-invalidate a dirty line during an incremental reflow if the target frame is contained _directly_ within the box that is the target of the reflow. That is, blocks ``higher up'' along the reflow chain do not force-invalidate their lines. (I need to prove that this is guaranteed to work, or find a counter-example.) 3. Removes the nsTextFrame::Reflow Invalidate hack, which should no longer be necessary. 4. Hoists force-invalidate computation out of the loop in ReflowDirtyLines. 5. Propagates a reflow reason of `dirty' (as opposed to `resize') when we're reflowing a block that is the target of an incremental reflow. This guarantees that we'll force-invalidate the dirty lines in that block, and in any dirty block children of that block.

Oops, I'd re-written some of the code without testing it first. (At least I didn't check it in, like I did last night :-/.) This patch fixes a bug in NeedsForceInvalidate.

Damn, this breaks expando. <http://www.mozilla.org/newlayout/demo/expando.html>

Sigh, the last patch (attachment 65534 [details] [diff] [review]) isn't right either. It was incorrectly returning `false' from NeedsForceInvalidate because aState.mNextRCFrame is _never_ going to be aBlockFrame: it's the next frame _inside_ the block, dummy!

The more I think about this, the less I think that we can handle this at the block level given the current way we've designed reflow and invalidation. (That's not to say these are immutable, but...it may be easier to pursue a solution at the scrollbar level here.) The block frame has been designed to handle invalidation at the line-level. In other words, when a line is marked dirty, the entire line is invalidated so that individual inline frames don't need to concern themselves with taking care of that task. This may not be optimal for typing, DHTML, or reflowing select frames, but it's a reasonable trade-off given that most of the time we're blasting HTML to the screen (imagine invalidating each individual span of text while rendering an LXR page, for example). Since the line containg the <select>'s box frame has been marked dirty (it has to be marked dirty for ReflowDirtyLines to notice it, and propagate the incremental reflow down to it), we invalidate the line. The whole line, including the select's box frame. Perhaps instead of dirtying the line, we could maintain a mNextRCLine member and test for _that_ during ReflowDirtyLines (I'll do a quick experiment to try that, but I don't immediately see how this would generalize to typing, etc). Or perhaps this is another argument for preferring a dirty-bit design over the reflow command design. Either way, we're looking at a hefty change. Kicking this out to `future'. Sorry, bryner. :-(

Here's a work-in-progress patch, if anyone wants to pick it up. It doesn't solve bryner's problem, though. I'll probably pick this up after I get back from sabbatical as (IMO) it cleans up some stuff that warranted cleanin'.

This looks like it's block/inline-ish.... Leaving in this component for now, though.

roc, as long as we're defining the invalidation model, we should keep this in mind.

I don't know how to evaluate if bug 539356 fixed this. I think that dlbi made us reconsider line-level invalidation and did something different, and so I'm going to close this.