138057 - nsBlockFrame::RememberFloaterDamage doesn't note changes to a float's width

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Chris Waterson]

nsBlockFrame::RememberFloaterDamage doesn't note damage in the space manager
when a floater's width changes in such a way as to not affect the line's
combined area.

Comment 1

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

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=79566 is a testcase,
once bug 129115 lands.

Comment 2

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

Also note that this should allow us to remove the PrepareResizeReflow hack when
we get an incremental reflow through a floater.

Comment 3

[Chris Waterson]

Attachment: possible solution

This is a sketch of a patch; intended for expository purposes only!

The basic idea here is to save the line's floater cache before reflowing the
line, and then use this in RememberReflowDamage to determine if any of the
floaters have changed horizontally. This fixes the problem that dbaron points
out in bug 129115 comment 116: it allows us to notice that the floater has
changed horizontally (even though the line's combined area hasn't changed), and
so we add the floater's area to the space manager.

So here's what needs more thought:

- This is obviously inefficient, so we need to either prove that `n' will
  stay small, or we need to come up with a better way to compare the old
  and new floater caches; e.g.,

- Perhaps the two floater caches will stay in sync -- or at least in sync
  enough that we can walk both lists in parallel. For example, is it
  possible for a floater to be added to or removed from the new floater
  cache? (Almost certainly, content could be added or removed, or the line
  may wrap differently.) Is it possible for the new floater cache to be
  re-arranged? (Probably not, as this would entail content being shuffled;
  but in that case, the frames would be re-created.)

- I used the floater cache's combined area. Is this right? (It looks like
  `mRegion' might be more appropriate.)

Anyway, food for discussion.

Comment 4

[Chris Waterson]

Also, the above patch (once it's done right) will probably allow us to remove
the call to nsBlockFrame::PrepareResizeReflow from PrepareChildIncrementalReflow.

Comment 5

[Christine Hoffman]

Changing QA contact

Comment 6

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

I *think* the approach here is insufficient because it will only propagate
damage that is within the block closest to the floater.  In other words, I think
it would handle a horizontal expansion of:

<div>
  <tall-float />
  text that must wrap
</div>

but it wouldn't work for:

<div>
  <div>
    <tall-float />
  </div>
  text that must wrap
</div>

since the only floater cache that points to "tall-float" (if my understanding is
correct) is the one associated with the line of the inner div.

Comment 7

[Chris Waterson]

Attachment: more radical approach

Here's a more radical solution, which eliminates RememberFloatDamage
altogether. Specifically, floater damage is recorded in the space manager
during nsBlockReflowContext::FlowAndPlaceFloater.

Comment 8

[Chris Waterson]

Well, the above patch makes it through the regression tests as well as a couple
simple DHTML float resizing animation tests that I've concocted (including the
one dbaron cites in comment #6).

Comment 9

[Chris Waterson]

So, what's going on here?

When an incremental reflow occurs, a floater's size may change. If a floater's
size changes, we need to note (in the space manager) the veritical `band' that
has been affected by the change. ReflowDirtyLines uses this information to know
when a line that is otherwise `clean' (i.e., not directly impacted by the
reflow) must be reflowed.

dbaron's first implementation of this used a routine called
RememberFloaterDamage that would note that the vertical size of a linebox had
changed. The delta was recorded in the space manager. The drawback of this
approach is that horizontal changes to the floater (e.g., the float gains or
loses width, but not height) go unnoticed.

The fix is to eliminate RememberFloaterDamage altogether, and `directly' note
that a floater's size has changed during FlowAndPlaceFloater. Specifically, we
snapshot the floater's size before reflowing the float, and then compare this
size with the float's size after reflow. If the floater's dimensions change, we
record the information in the space manager.

This additionally allows us to remove the hack in PrepareChildIncrementalReflow
that `punts' any incremental reflow targeted at line's float to a full resize
reflow. (This was done because the PrepareResizeReflow code marks each line that
is impacted by a floater as requiring reflow: extremely conservative.)

Comment 10

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

Comment on attachment 80450 [details] [diff] [review]
more radical approach

>Index: nsBlockReflowState.cpp

>+    // XXXwaterson cast! you know it's a block frame, don't you?

It could be an outer table frame, right?

>+    // XXXwaterson don't you only need to recur if NS_BLOCK_SPACEMGR is set?

No, since the floater cache is associated only with the line immediately
containing the placeholder.  (I think this is the case because only lines
that are |IsInline| can have a floater cache.  I didn't trace through all
the code.)

>@@ -846,6 +848,12 @@
>   // for it to live.
>   nsRect region;
> 
>+  // The floater's old region, so we can propagate damage.
>+  nsRect oldRegion;
>+  floater->GetRect(region);
>+  region.width += aFloaterCache->mMargins.left + aFloaterCache->mMargins.right;
>+  region.height += aFloaterCache->mMargins.top + aFloaterCache->mMargins.bottom;

Don't you want to be filling in |oldRegion| here rather than |region|?

>+
>   // Advance mY to mLastFloaterY (if it's not past it already) to
>   // enforce 9.5.1 rule [2]; i.e., make sure that a float isn't
>   // ``above'' another float that preceded it in the flow.
>@@ -1001,6 +1009,17 @@
> #endif
>     mSpaceManager->AddRectRegion(floater, region);
>     NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "bad floater placement");
>+  }
>+
>+  // If the floater's dimensions have changed, note the damage in the
>+  // space manager.
>+  // XXXwaterson conservative: we could probably get away with noting
>+  // less damage; e.g., if only height has changed, then only note the
>+  // area into which the float has grown or from which the float has
>+  // shrunk.
>+  if (oldRegion.height != region.height || oldRegion.width != region.width) {
>+    nscoord height = NS_MAX(oldRegion.height, region.height);
>+    mSpaceManager->IncludeInDamage(region.y, region.y + height);
>   }
> 

What happens if the horizontal position of the floater changes, for example,
because its margin changes?  It seems to me that you want to track x and y
as well, so that the first chunk of code would also have:

oldRegion.x -= aFloaterCache->mMargins.left;
oldRegion.y -= aFloaterCache->mMargins.top;

Then the second part could be a little less conservative and say:

if (oldRegion.width != newRegion.width) {
  nscoord top = NS_MIN(oldRegion.y, region.y);
  nscoord bottom = NS_MAX(oldRegion.YMost(), region.YMost());
  mSpaceManager->IncludeInDamage(top, bottom);
} else {
  if (oldRegion.y != newRegion.y) {
     /* maybe refactor into |inline void
      * SortPair(nscoord a, nscoord b, nscoord &smaller, nscoord &larger)|
      */
     nscoord top, bottom;
     if (oldRegion.y > newRegion.y) {
       top = newRegion.y;
       bottom = oldRegion.y;
     } else {
       top = oldRegion.y;
       bottom = newRegion.y;
     }
     mSpaceManager->IncludeInDamage(top, bottom);
  }
  /* ... and the same for YMost() as for y ... */
}


I like the patch in general, though.

Comment 11

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

Comment on attachment 80450 [details] [diff] [review]
more radical approach

>+  // The floater's old region, so we can propagate damage.
>+  nsRect oldRegion;
>+  floater->GetRect(region);
>+  region.width += aFloaterCache->mMargins.left + aFloaterCache->mMargins.right;
>+  region.height += aFloaterCache->mMargins.top + aFloaterCache->mMargins.bottom;

Or better yet:

nsRect oldRegion;
floater->GetRect(oldRegion);
oldRegion.Inflate(aFloaterCache->mMargins);

Comment 12

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

> It could be an outer table frame, right?

Or an image (or object) frame with 'display: block', I would think.  And
probably a few other things as well.  (Maybe nsBoxFrame?)

Comment 13

[Chris Waterson]

Attachment: clean up in response to dbaron's comments

Whew! I'm glad to report that the patch still seems to work when we properly
initialize `oldRegion'. I've updated the patch to fix that, as well as use the
tidier nsRect::Inflate method. I've removed my `XXX' confusion: that is
unrelated to this bug.

dbaron wrote:

> What happens if the horizontal position of the floater changes, for example,
> because its margin changes?  It seems to me that you want to track x and y
> as well...

It seems to me that we only care if there is a change to the size of the region
which the float occupies. If someone twiddles the margins, either they'll do so
in a way that alters the region that the float occupies (e.g., changing only
margin-left), or they won't (e.g., adding some amount margin-left and removing
the same amount from margin-right). In the former case, we'll note a change in
the region, and dirty the region in the space manager. In the latter case, the
region won't have changed, so there is no need to dirty the lines that flow
around the float. Right?

Comment 14

[Chris Waterson]

(IOW, I can't think of a way that region.x or region.y would change without the
width or height changing, too...)

Comment 15

[Chris Waterson]

Here's a silly test case:

  <http://www.maubi.net/~waterson/mozilla/bugs/float-animated-margin.html>

With paint-flashing on (and this patch, along with the patch from 129115) you'll
see we only invalidate the float's area, (Ironically, we're off by a fair bit --
the invalidation rect is too high. I ought to track that down at some point).

Comment 16

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

I guess comment 13 makes sense, although you're still damaging too much for a
height change (and for that you'll want the old x).

Comment 17

[Marc Attinasi]

Comment on attachment 80871 [details] [diff] [review]
clean up in response to dbaron's comments

Looks OK to me - sr=attinasi

Comment 18

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

Attachment: testcase showing why you need to check origin in addition to size

This testcase shows why you need to check if the position of the float changed
in addition to whether the size changed.  The float can change position due to
a change in size of another float.  See comment 10 / comment 11.

Comment 19

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

(And, FWIW, in a current build (without the patch on this bug, when the damage
is propagated correctly), we have some repainting issues on that testcase.)

Comment 20

[Chris Waterson]

dbaron: good, good. :-)

Comment 21

[Chris Waterson]

Attachment: address dbaron's test case

kin also mentioned a (currently broken) case in editor where floats are
dyanmically changed from `float: left' to `float: right'. Maybe this will help
there, too.

Comment 22

[Chris Waterson]

I should note that dbaron's test case (attachment 81061 [details]) illustrates some
invalidation problems: if you set the window's width to ~725px, and then run the
test, you'll see some paint artifacts when the second <div> resizes. Forcing an
expose clears up the problem, so somewhere we're bungling an invalidate. (N.B.
that this happens with or without the patch: I'll file a bug if there isn't one
on file already...)

Comment 23

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

Comment on attachment 81079 [details] [diff] [review]
address dbaron's test case

r=dbaron

Comment 24

[Chris Waterson]

Attachment: let's go crazy with the cheese whiz.

This patch goes even more nuts:

- There's no reason to dirty _all_ of the lines in the block if the child 
  specified to ReflowDirtyChild is a float. This removes that logic.

- Once that's been simplified, it's possible to pare down FindLineFor to simply

  return a line_iterator. We have to publicly expose nsBlockFrame::end_lines
  because FindLineFor is used from nsHTMLReflowState.

I've placed a couple of test cases that explicitly test _dirty_ reflows (as
opposed to the style-changed reflows we've been testing to date) at

  <http://www.maubi.net/mozilla/bugs/index.html#floater-incremental-reflow>

and they seem to work.

dbaron, it's getting hot in here. Are you still with me? :-)

Comment 25

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

Comment on attachment 81096 [details] [diff] [review]
let's go crazy with the cheese whiz.

r=dbaron

I suspect some (or all?) of the callers of nsBlockFrame::FindLineFor really
only need nsLineBox::FindLineContaining, but it may not be easy to tell.

Comment 26

[Marc Attinasi]

Comment on attachment 81096 [details] [diff] [review]
let's go crazy with the cheese whiz.

sr=attinasi

Comment 27

[Chris Waterson]

Changes checked in, thanks.

Comment 28

[Chris Waterson]

Filed bug 142029 on the invalidation issue in comment 22.