If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla56
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [need merging to tip])

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 541823 [details]
Testcase

See attached testcase.  There should be no red, but there is.
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.
Blocks: 666606
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.
Created attachment 541896 [details] [diff] [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.
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
(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?
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.
(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.  :-)
Created attachment 542886 [details] [diff] [review]
Patch (v1)

Applied on top of my other patches.
Assignee: bzbarsky → ehsan
Attachment #541896 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #542886 - Flags: review?(dbaron)
Created attachment 542901 [details] [diff] [review]
Patch (v1)

Forgot to adjust the reftest annotations...
Attachment #542886 - Attachment is obsolete: true
Attachment #542901 - Flags: review?(dbaron)
Attachment #542886 - Flags: review?(dbaron)
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
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.
(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
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.
(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).
Created attachment 543183 [details] [diff] [review]
Patch (v2)

With a test fix
Attachment #542901 - Attachment is obsolete: true
Attachment #543183 - Flags: review?(dbaron)
Attachment #542901 - Flags: review?(dbaron)
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.
Attachment #543183 - Flags: review?(dbaron) → review+
Ehsan, do you want to merge this to tip, or should I?
(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.  :(
Boris, did you ever get to do this?
Not yet....
I'll shamelessly put this back on your plate.  :)
Assignee: ehsan → bzbarsky
Whiteboard: [need review] → [need merging to tip]
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
Blocks: 1308876
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad12b1bd44825347efc69bb52931e8ae3efeb80&group_state=expanded
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

2 months ago
Pushed by dbaron@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48f19f8bc8ba
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.