119311 - Reconsider line-level invalidation

Status: RESOLVED DUPLICATE of bug 539356

(Core :: Layout: Block and Inline, defect)

Description

[Brian Ryner (not reading)]

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

Comment 1

[Chris Waterson]

Attachment: hack

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.

Comment 2

[Chris Waterson]

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

Comment 3

[Chris Waterson]

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

Comment 4

[Chris Waterson]

Attachment: less of a hack

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.

Comment 5

[Chris Waterson]

cc'ing block folks. What do you think?

Comment 6

[Chris Waterson]

Comment on attachment 65130 [details] [diff] [review]
less of a hack

Seeing some weirdness in plaintext editor.

Comment 7

[Chris Waterson]

Specifically, in mail-news compose.

Comment 8

[Chris Waterson]

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?

Comment 9

[rbs]

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

Comment 10

[Chris Waterson]

Attachment: work-in-progress patch

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.

Comment 11

[Chris Waterson]

Attachment: whoops

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.

Comment 12

[Chris Waterson]

Damn, this breaks expando.

  <http://www.mozilla.org/newlayout/demo/expando.html>

Comment 13

[Chris Waterson]

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!

Comment 14

[Chris Waterson]

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. :-(

Comment 15

[Chris Waterson]

Attachment: status check for work-in-progress

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.