1365449 - Reflow absolutely positioned children when they need to be repaginated

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

This fixes the failure of
layout/reftests/pagination/dynamic-abspos-overflow-01-cols.xhtml with
the primary patch in bug 1308876.

Since it is an independently testable failure, I'm posting it as a
separate bug.

Without the patch, both reftests fail to rewrap in response to the
dynamic change, and the inner dark blue absolutely positioned element
remains wrapped at the wrong position when the inner light blue
relatively positioned element rewraps.  (I tested this only outside of
the reftest harness, but that should be sufficient.)

I verified manually that the height conditions were correct by modifying
both reftests to add some padding and border to #relpos and margin to
#abspos, changing the height of #abspos so that it was either exactly at
or just above the threshold where reflow was needed, and using
GECKO_DISPLAY_REFLOW_RULES_FILE debugging to verify that the reflow of
the absolutely positioned element did or didn't happen as expected.

MozReview-Commit-ID: 6ISgSEYyMiN

Comment 1

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

Attachment: Reflow absolutely positioned children when they need to be repaginated

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8868352 [details] [diff] [review]
Reflow absolutely positioned children when they need to be repaginated

>+          kidFrame->GetLogicalRect(aContainingBlock.Size()).BEnd(kidWM);

I think you're using the wrong CB for grid - you should move up the
|const nsRect& cb = isGrid ? ...| line and use |cb.Size()| instead.

(r- for this bit)

>+      if (containerWM.GetBlockDir() != kidWM.GetBlockDir()) {

This can probably be |containerWM.IsOrthogonalTo(kidWM)| instead.
I don't see why a vertical-lr/rl difference would invalidate
the logic in the else-clause.  But feel free to keep it as is.

>+        if (kidBEnd != availBSize &&
>+            (kidBEnd > availBSize || kidFrame->GetNextInFlow())) {

Nit: I think it'd be easier to follow the logic if this was written as:
        if (kidBEnd > availBSize ||
            (kidBEnd < availBSize && kidFrame->GetNextInFlow())) {

because then it reads as:
"the kid might want to be smaller now" ||
"the kid might want to be larger now"

Note that we still have a bug when |kidBEnd == availBSize|.
If we don't reflow the kid we must at least recalculate its overflow
areas.  I'll attach a testcase that triggers this...

You might want to address that while we're here, but doing it as
a separate bug is fine too.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase that shows existing overflow bug

Comment 4

[Mats Palmgren (inactive)]

> If we don't reflow the kid we must at least recalculate its overflow areas.

I think we need to reflow also for purely overflow cases to create overflow
container continuations, so if we add a nsIFrame::HasBlockAxisScrollableOverflow()
or some such, then I think the condition should be:
        if (kidBEnd > availBSize ||
            (kidBEnd < availBSize && kidFrame->GetNextInFlow()) ||
            kidFrame->HasBlockAxisScrollableOverflow()) {

Comment 5

[Mats Palmgren (inactive)]

Attachment: A testcase that requires a reflow to create an overflow container

Comment 6

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

(In reply to Mats Palmgren (:mats) from comment #2)
> >+      if (containerWM.GetBlockDir() != kidWM.GetBlockDir()) {
> 
> This can probably be |containerWM.IsOrthogonalTo(kidWM)| instead.
> I don't see why a vertical-lr/rl difference would invalidate
> the logic in the else-clause.  But feel free to keep it as is.

I think it should be as-is.  What it invalidates is the conditions -- since kidBEnd is now on the wrong side for what we're trying to test.

> Note that we still have a bug when |kidBEnd == availBSize|.
> If we don't reflow the kid we must at least recalculate its overflow
> areas.  I'll attach a testcase that triggers this...
> 
> You might want to address that while we're here, but doing it as
> a separate bug is fine too.

I'd prefer to address this in a separate bug; I'm trying to get all the changes I need for tests to pass with bug 1308876 landed so that I can get that in.

I've made the other changes.

Comment 7

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

Attachment: Reflow absolutely positioned children when they need to be repaginated

This fixes the failure of
layout/reftests/pagination/dynamic-abspos-overflow-01-cols.xhtml with
the primary patch in bug 1308876.

Since it is an independently testable failure, I'm posting it as a
separate bug.

Without the patch, both reftests fail to rewrap in response to the
dynamic change, and the inner dark blue absolutely positioned element
remains wrapped at the wrong position when the inner light blue
relatively positioned element rewraps.  (I tested this only outside of
the reftest harness, but that should be sufficient.)

I verified manually that the height conditions were correct by modifying
both reftests to add some padding and border to #relpos and margin to
#abspos, changing the height of #abspos so that it was either exactly at
or just above the threshold where reflow was needed, and using
GECKO_DISPLAY_REFLOW_RULES_FILE debugging to verify that the reflow of
the absolutely positioned element did or didn't happen as expected.

MozReview-Commit-ID: 6ISgSEYyMiN

Comment 8

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

Comment on attachment 8869604 [details] [diff] [review]
Reflow absolutely positioned children when they need to be repaginated

Actually, I'll fix the other bit now while I have it in my head...

Comment 9

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

Attachment: Reflow absolutely positioned children when they need to be repaginated

This fixes the failure of
layout/reftests/pagination/dynamic-abspos-overflow-01-cols.xhtml with
the primary patch in bug 1308876.

Since it is an independently testable failure, I'm posting it as a
separate bug.

Without the patch, both reftests fail to rewrap in response to the
dynamic change, and the inner dark blue absolutely positioned element
remains wrapped at the wrong position when the inner light blue
relatively positioned element rewraps.  (I tested this only outside of
the reftest harness, but that should be sufficient.)

I verified manually that the height conditions were correct by modifying
both reftests to add some padding and border to #relpos and margin to
#abspos, changing the height of #abspos so that it was either exactly at
or just above the threshold where reflow was needed, and using
GECKO_DISPLAY_REFLOW_RULES_FILE debugging to verify that the reflow of
the absolutely positioned element did or didn't happen as expected.

MozReview-Commit-ID: 6ISgSEYyMiN

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8869622 [details] [diff] [review]
Reflow absolutely positioned children when they need to be repaginated

>+      // Note that this always reflows kids that have 

Looks like you forgot to finish / remove this sentence.

Comment 11

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

remove; I started to write it and then concluded it wasn't true.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/093eab57d50408eab14dcfa17f8e916e34ecd4b6
Bug 1365449 - Reflow absolutely positioned children when they need to be repaginated.  r=mats

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/093eab57d504