667079 - Incremental reflow that changes size of a relatively positioned inline doesn't correctly reflow an absolutely positioned child of it

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

See attached testcase.  There should be no red, but there is.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is the same issue as bug 666606 at heart.

In particular, when the inline resizes it reflows the table, but this happens _before_ the inline's new size is set.  That needs to happen for correct overflow area propagation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, continuing....

At this point, the inline still has the old size.  It passes the new size explicitly as the containing block size when reflowing its absolute kids.  But that only makes it as far as the _outer_ table frame.  When the inner table frame reflows, it doesn't know what was passed in, so tries to use the frame size and this fails spectacularly.

I think the right fix is to save the containing block size in the reflow state after determining and to pass the outer table frame's containing block size explicitly to the inner table.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, perhaps

I realize this probably conflicts somewhat with the patch for bug 10209....  The hack for quirk height is sort of sad; I can get rid of it by setting 'height: inherit' on outer tables, but I'm worried about making that sort of change.

Comment 4

[(no longer active)]

(In reply to comment #3)
> Created attachment 541896 [details] [diff] [review] [review]
> Like so, perhaps
> 
> I realize this probably conflicts somewhat with the patch for bug 10209.... 
> The hack for quirk height is sort of sad; I can get rid of it by setting
> 'height: inherit' on outer tables, but I'm worried about making that sort of
> change.

Would you mind if I ported this patch on top of the rest of my patches and test it and then submit it for review?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er... I meant to ask you and dbaron for review.  :(  bzexport strikes again.  :(

I don't mind at all; it makes sense to port this on top of your patches.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Er... I meant to ask you and dbaron for review.  :(  bzexport strikes again. 
> :(
> 
> I don't mind at all; it makes sense to port this on top of your patches.

OK, I'll look at this when I'm done with bug 659828.  I will also address any review comments that I may have by changing the patch before submitting it for dbaron to review.  :-)

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Applied on top of my other patches.

Comment 8

[(no longer active)]

Attachment: Patch (v1)

Forgot to adjust the reftest annotations...

Comment 9

[(no longer active)]

This doesn't seem to have helped with the failure in 10209-1.html on Linux and Windows...

http://tbpl.mozilla.org/?tree=Try&rev=eee016e8778c

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  That test might just be incorrect... maybe.  Set the top and left of the abs pos table to 0?  That should make things work.

Comment 11

[(no longer active)]

(In reply to comment #10)
> Hmm.  That test might just be incorrect... maybe.  Set the top and left of
> the abs pos table to 0?  That should make things work.

Hmm, doing that causes the table to fall out of the screen: http://pastebin.mozilla.org/1261330

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh!  You need an actual character inside the inline; an empty inline does weird sizing stuff.  Just set the color to white on the inline, and put an X or something in there.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Oh!  You need an actual character inside the inline; an empty inline does weird
> sizing stuff.  Just set the color to white on the inline, and put an X or
> something in there.

Doing that would make the test to render the same way that it does without the changes (and hence to fail the same way too).

Comment 14

[(no longer active)]

Attachment: Patch (v2)

With a test fix

Comment 15

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

Comment on attachment 543183 [details] [diff] [review]
Patch (v2)

>-          mStylePosition->mHeight.GetUnit() == eStyleUnit_Percent) {
>+          (mStylePosition->mHeight.GetUnit() == eStyleUnit_Percent ||
>+           (frame->GetType() == nsGkAtoms::tableOuterFrame &&
>+            frame->GetFirstChild(nsnull)->GetStylePosition()->mHeight.GetUnit() == eStyleUnit_Percent))) {

Fix the 80th column violation and update to the new child list api, and r=dbaron.  Sorry for the delay getting to this.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ehsan, do you want to merge this to tip, or should I?

Comment 17

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Ehsan, do you want to merge this to tip, or should I?

I'd appreciate if you could.  I'm currently swamped with the updater stuff.  :(

Comment 18

[(no longer active)]

Boris, did you ever get to do this?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Not yet....

Comment 20

[(no longer active)]

I'll shamelessly put this back on your plate.  :)

Comment 21

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

This is needed to prevent layout/reftests/bugs/371561-1.html from failing on Android (or with 'html {overflow: hidden}') once bug 1308876 lands.  I have it merged and ready to land.

The merging and addressing of review comments consisted of these changes to the patch:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/be73eb31418b
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/673cb140d95d
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/c6dcc9754c1b

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad12b1bd44825347efc69bb52931e8ae3efeb80&group_state=expanded

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/48f19f8bc8ba126fd28b8f3e90bd25610814fc09
Bug 667079.  Make sure to set the right containing block size for inner tables no matter what.  r=dbaron

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f19f8bc8ba
Make sure to set the right containing block size for inner tables no matter what.  r=dbaron

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48f19f8bc8ba