383883 - {inc} Issue with block moving through non-moving float

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Eli Friedman]

Testcase coming up.  I'm not really sure what the cause is; maybe the code for sliding a block isn't doing enough to propagate damage.

Comment 1

[Eli Friedman]

Attachment: Testcase

Comment 2

[Eli Friedman]

Attachment: WIP

This patch fixes this bug, bug 383885, bug 373383, and possibly others.  The issue is that it's probably slower in some cases because it's significantly more conservative.

The aDeltaY check is not a valid optimization because aDeltaY doesn't determine whether a line is moving relative to the nearest float containing block.  The wasImpactedByFloat != isImpactedByFloat check is invalid because in the case where both wasImpactedByFloat and isImpactedByFloat are true, the amount of impact could change.

It would be possible to restore these optimizations with some extra work, although I don't know whether it's worthwhile.

Comment 3

[Eli Friedman]

This patch also happens to fix the testcase in bug 383884, but that would break again if we optimize based on the amount of impact on the line.

Comment 4

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

The second part looks right. The first part looks like it's going to really hurt performance: together with the second part, this means any lines impacted by floats will be reflowed every single time we reflow the block.

Maybe we can pass around a flag indicating whether this block has moved relative to the block formatting context? Then here we could test aDeltaY || flag.

Comment 5

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

So if the problem is that a block is moving relative to the BFC, that means that one of its ancestors was slid.  Is the problem just that we're not marking that ancestor dirty?  (It probably used to (pre-reflow-branch) be that marking the block-line containing the ancestor dirty was equivalent, but it's not anymore.)

Comment 6

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

Marking the ancestor dirty doesn't force all its lines to be reflowed (right?), so I don't think that would fix this bug.

Comment 7

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

No, marking it dirty should force its lines to be reflowed.  Dirty implies that all descendants are dirty.

Comment 8

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

I guess that would work, then.

Comment 9

[Eli Friedman]

Attachment: Patch v2

dbaron, is this what you were thinking?  I suppose it works, although it is a bit evil.

(Note that this bug is not a reflow branch regression...)

Comment 10

[Eli Friedman]

Attachment: Patch v2 (right file)

(Attached the wrong version.)

Comment 11

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

Comment on attachment 270534 [details] [diff] [review]
Patch v2 (right file)

Sure.  r+sr=dbaron.  Please add a testcase to the reftests, and adjust the comment above PropagateFloatDamage appropriately (to mention that it marks blocks dirty if aDeltaY != 0).  Sorry for the delay in getting to this.

Comment 12

[Eli Friedman]

Attachment: Modified testcase that is still broken with patch v2

I wish there were some easy way to query for the right information from the space manager.

Comment 13

[Eli Friedman]

I'm tempted to say that the easiest way to deal with this would be to completely disable line sliding optimizations for space-managers with floats.  I know that's not ideal, but I don't see any way around it without massive changes to the way the space manager works.  (I don't know whether line-sliding is actually helpful in any real-world cases, and whether those real-world cases use floats.)

Comment 14

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

Doesn't that mean basically disabling incremental reflow for block formatting contexts containing floats? 

Comment 15

[Eli Friedman]

(In reply to comment #14)
> Doesn't that mean basically disabling incremental reflow for block formatting
> contexts containing floats? 

In the insert case, yes, but not in the append case.  The question is how much we care about the insert case.

Comment 16

[Eli Friedman]

Oh, wait, I see your point... we can't tell the difference between the append case and the insert case.

Comment 17

[Eli Friedman]

Attachment: Patch v3

This patch takes a significantly different approach: it passes down the information needed to tell whether a line is actually moving.

While I was in nsBlockReflowContext, I couldn't resist cleaning it up a bit, so some of the changes aren't really required.

Comment 18

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

Seems like you should initialize mBlockDelta in both nsHTMLReflowState constructors.

Is there a way we could assert that you don't cross from a block to a non-block to a block without creating a new space manager?  It seems that you're relying on that invariant.

Why are you changing the calculation for right floats?  I agree we can remove the NS_UNCONSTRAINEDSIZE handling, but why the other changes?  Or do the mX and mY not matter anymore?

Comment 19

[Eli Friedman]

(In reply to comment #18)
> Seems like you should initialize mBlockDelta in both nsHTMLReflowState
> constructors.

Oh, oops; I'll do that.

> Is there a way we could assert that you don't cross from a block to a non-block
> to a block without creating a new space manager?  It seems that you're relying
> on that invariant.

We would run into issues if we missed creating a needed space manager even without this patch.  I can add an assertion to that effect, but note that it doesn't cover all cases where we need a space manager.

I'll also add an assertion that the delta is zero if the parent reflow state has a different space manager; I think that covers what I'm really depending on.

> Why are you changing the calculation for right floats?  I agree we can remove
> the NS_UNCONSTRAINEDSIZE handling, but why the other changes?  Or do the mX and
> mY not matter anymore?

+    // Note that the values of x and y do not matter for floats,
+    // because floats have their own space manager and don't use PlaceBlock

Should I clarify that more?  (Note that "if (aLine)" is never true for floats.)

Comment 20

[Eli Friedman]

Attachment: Patch v4

Comment 21

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

Comment on attachment 275197 [details] [diff] [review]
Patch v4

>+      // Unconditionally reflow sliding blocks; we only really need to reflow
>+      // if there's a float impacting this block, but the current space manager
>+      // makes it difficult to check that
>+      aLine->MarkDirty();

This comment should probably say something about letting the child block make its own decisions (in turn) about which lines to reflow.  Otherwise it makes it sound like you're marking the block dirty, which you're not.


Did you test that right floats with frames with views inside of them still work correctly?  And that margins on floats still work correctly?

r+sr+a1.9=dbaron

Comment 22

[Eli Friedman]

Attachment: Final patch

Comment 23

[Eli Friedman]

Attachment: Real final patch

Comment 24

[Eli Friedman]

Checked in.