724978 - nsColumnSetFrame should be an absolute container

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

In order to be an absolute container, two things must be true:
  1) nsColumnSetFrame needs to call FinishWithAbsoluteFrames() instead of FinishAndStoreOverflow() during reflow.
  2) nsColumnSetFrame must be an overflow container.

#1 is easy, but #2 requires some work to get nsColumnSetFrame to learn how to be an overflow container.

Comment 1

[Scott Johnson (:jwir3)]

Some additional information about this, from mats:
 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#93
93   virtual nsresult StealFrame(nsPresContext* aPresContext,
94                               nsIFrame*      aChild,
95                               bool           aForceNormal)
96   { // nsColumnSetFrame keeps overflow containers in main child list
97     return nsContainerFrame::StealFrame(aPresContext, aChild, true);
98   }

nsColumnSetFrame does not implement IsFrameOfType(eCanContainOverflowContainers).

It looks like nsOverflowContinuationTracker use Get/Set/RemovePropTableFrames
unconditionally though, so I guess that's why the overflow containers can't be
found on the principal list.

Basically, nsColumnSetFrame is storing it's overflow containers in a special way, not on the principal list. This needs to be changed and nsColumnSetFrame should stop doing it's special handling of these overflow containers.

Comment 2

[Scott Johnson (:jwir3)]

When this is completed, the following reftests will need to be re-enabled and they must pass:

  /layout/reftests/abs-pos/multicolumn-1.html
  /layout/reftests/bugs/379349-1a.xhtml
  /layout/reftests/bugs/379349-1b.xhtml
  /layout/reftests/bugs/379349-1c.xhtml
  /layout/reftests/bugs/379349-2a.xhtml
  /layout/reftests/bugs/379349-2b.xhtml

Comment 3

[fantasai]

Actually, you don't want to change how nsColumnSetFrame handles overflow containers. That'll break real overflow containers. You only want to change how it handles abspos continuations, which pretend they are overflow containers. These should be stored on the special list, not in the main child list.

Comment 4

[(no longer active)]

Attachment: Branch patch

Comment 6

[(no longer active)]

Moving over the tracking flags from bug 736072.

Comment 7

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 609539
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ead6782e3a3b
Try run started, revision ead6782e3a3b. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ead6782e3a3b

Comment 8

[(no longer active)]

Note to fantasai: the new abs-pos container APIs were landed in https://hg.mozilla.org/mozilla-central/rev/9989f9ef7b90.

Comment 9

[Mozilla RelEng Bot]

Try run for ead6782e3a3b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ead6782e3a3b
Results (out of 219 total builds):
    success: 183
    warnings: 29
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ead6782e3a3b

Comment 10

[(no longer active)]

The try server seems to be reasonably happy with me today!

Comment 11

[fantasai]

I think you need tests for the following:
  a) a multi-page multi-column element with abspos children
  b) a two-page multi-column element with 4-page abspos children
Also a testcase that does b) but manipulates the heights dynamically to cause overflow or fragmentation or not, somewhat like the pagination/dynamic-abspos-overflow-01-cols test.

Comment 12

[fantasai]

Comment on attachment 609539 [details] [diff] [review]
Branch patch

Ok, so afaict, you're not handling the pagination of absolutely-positioned children. To do that, you need to:

1. Add a call to ReflowOveflowContainerChildren() to the nsColumnSetFrame::Reflow() method and incorporate its output into the out parameters, i.e. add (this is copied from nsBlockFrame::Reflow)

  // Handle paginated overflow (see nsContainerFrame.h)
  nsOverflowAreas ocBounds;
  nsReflowStatus ocStatus = NS_FRAME_COMPLETE;
  if (GetPrevInFlow()) {
    ReflowOverflowContainerChildren(aPresContext, *reflowState, ocBounds, 0,
                                    ocStatus);
  }

near the beginning (before reflowing any children), and add

  NS_MergeReflowStatusInto(&state.mReflowStatus, ocStatus);
  NS_MergeReflowStatusInto(&state.mReflowStatus, fcStatus);

near the end. Then you also need to update StealFrames by replacing
  return nsContainerFrame::StealFrame(aPresContext, aChild, true);
with
  return nsContainerFrame::StealFrame(aPresContext, aChild,
                                      IS_TRUE_OVERFLOW_CONTAINER(aChild));

And of course add some tests. :) And I think that should take care of it, assuming you're reflowing the abspos children correctly.

Comment 13

[fantasai]

Oh, also as Scott notes, you need to add eCanContainOverflowContainers to the frame type.

Comment 14

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

I thought the goal here was to *not* paginate abs-pos children.

Comment 15

[fantasai]

I thought the goal here was "nsColumnSetFrame should be an absolute container".

If we're not paginating abs-pos children, you need to pass aConstrainHeight = false to nsAbsoluteContainer::Reflow() so it knows not to split the kids.

But apparently nothing in the codebase is doing that anymore! I'm a little concerned about that, it doesn't seem right. Why did you think it was okay to change nsPageContentFrame to pass aConstrainHeight = True in http://hg.mozilla.org/mozilla-central/diff/0cafa2cbe386/layout/generic/nsPageContentFrame.cpp ???

Comment 16

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=736072#c8. As far as I can tell, the spec doesn't say whether abs-pos children of an element with columns should be paginated. Webkit doesn't. It's easiest for us to follow suit.

(In reply to fantasai from comment #15)
> If we're not paginating abs-pos children, you need to pass aConstrainHeight
> = false to nsAbsoluteContainer::Reflow() so it knows not to split the kids.

Why? It's OK to split the kids if the available height for the nsColumnSetFrame is constrained (e.g. if the element is itself in a paginated context).

> But apparently nothing in the codebase is doing that anymore! I'm a little
> concerned about that, it doesn't seem right. Why did you think it was okay
> to change nsPageContentFrame to pass aConstrainHeight = True in
> http://hg.mozilla.org/mozilla-central/diff/0cafa2cbe386/layout/generic/
> nsPageContentFrame.cpp ???

That looks like a mistake.

Comment 17

[(no longer active)]

I'm sort of puzzled on the desired behavior that we want to have here with regard to the pagination of the abs-pos children of nsColumnSetFrame.

Comment 18

[fantasai]

> As far as I can tell, the spec doesn't say whether abs-pos children of an element with
> columns should be paginated. Webkit doesn't. It's easiest for us to follow suit.

That's a different bug. This one is about allowing nsColumnSetFrame to be an abspos container.

> Why? It's OK to split the kids if the available height for the nsColumnSetFrame is
> constrained (e.g. if the element is itself in a paginated context).

My review comments were on how to do this properly, since right now the patch afaict doesn't handle this situation at all. Then you say the goal is to *not* enable this behavior, so I tell you how to disable it properly. To which you reply that we should allow this behavior. You are not making sense. >:[

Comment 19

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

You're right. I was confused.

Comment 20

[(no longer active)]

So, which one of the two is the desired behavior here?  :-)

Comment 21

[fantasai]

Follow comment 12.

Comment 22

[(no longer active)]

Attachment: WIP

So I gave this a shot.  There's a problem in laying out layout/reftests/abs-pos/multi-column-multi-page-2.html.  The absolute kid does not get split across multiple pages (here is the frame tree for this test: https://gist.github.com/2284574).  The h2 element only appears on the first page, but this frame tree seems to suggest that there's an h2 element on the last 4 pages (I don't know why there are 6 pages in the frame tree).

I don't know how this stuff is supposed to work, fantasai can you please take a look at the frame tree/patch and see if you can spot anything odd?

Comment 23

[fantasai]

The frametree looks right to me. The h2 element is 10in tall, and each page is only 2in tall, so it *should* show up on the first 5 pages. Right? Not sure why it's showing up on the 6th, maybe our math is off. Let me redo your testcase to use percentage heights, then we can look at print preview...

Comment 24

[Daniel Veditz [:dveditz]]

To expand on comment 6, bug 736072 was a regression in Firefox 11 and ESR 10.0.3 caused by the fix for bug 718516. We may therefore need this fix on the ESR10 branch if bug 736072 is bad/common enough.

Comment 25

[fantasai]

Attachment: testcase

This isn't rendering correctly with the patch. (I haven't figured out why yet.)

Comment 26

[(no longer active)]

Attachment: WIP 2

This updated patch fixes the reftest issue.  But the rendering of the test case in comment 25...

Comment 27

[(no longer active)]

Attachment: WIP 2

Forgot to refresh the latest version of the patch which fixes the GetSkipSides issue.

Comment 28

[fantasai]

Ehsan, so I would change the code to convert the outgoing aStatus from NS_FRAME_OVERFLOW_INCOMPLETE to NS_FRAME_OVERFLOW_COMPLETE when the last child returns NS_FRAME_OVERFLOW_INCOMPLETE. That should fix the blockquote's positioning. There's still something weird going on that I haven't figured out yet--the child's NIF is returning overflow-incomplete for no reason I can think of--but I think this is the right thing to do in any case. Make the conversion in Reflow(), right before you fold in the ocStatus. I'll still investigate the child's nsReflowStatus confusion, but that should fix it for this patch.

Comment 29

[fantasai]

Filed bug 743402 on the NS_FRAME_OVERFLOW_INCOMPLETE status.

Comment 30

[(no longer active)]

(In reply to fantasai from comment #28)
> Ehsan, so I would change the code to convert the outgoing aStatus from
> NS_FRAME_OVERFLOW_INCOMPLETE to NS_FRAME_OVERFLOW_COMPLETE when the last
> child returns NS_FRAME_OVERFLOW_INCOMPLETE. That should fix the blockquote's
> positioning. There's still something weird going on that I haven't figured
> out yet--the child's NIF is returning overflow-incomplete for no reason I
> can think of--but I think this is the right thing to do in any case. Make
> the conversion in Reflow(), right before you fold in the ocStatus. I'll
> still investigate the child's nsReflowStatus confusion, but that should fix
> it for this patch.

I tried doing that, and unfortunately it doesn't change how your test case is rendered in paginated mode.

Comment 31

[(no longer active)]

Comment on attachment 609539 [details] [diff] [review]
Branch patch

I still think this is the right patch for branches.  While it doesn't handle the overflow situation correctly, it restores the functionality in Firefox 10 (i.e., rendering _something_ for abspos children of column sets as opposed to nothing).

I'm renoming this for review as I don't think the more intensive patches I have here are going to be appropriate to take on branches (as they are too risky).

Comment 32

[fantasai]

Comment on attachment 609539 [details] [diff] [review]
Branch patch

As I said in comment 15, you need to pass aConstrainHeight = false to nsAbsoluteContainer::Reflow() so it knows not to split the kids. And you should have a dynamic testcase of splitting nsColumnSetFrames with abpsos kids so we know it doesn't crash.

Comment 33

[(no longer active)]

Attachment: Branch patch

Comment 34

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613300
	Branch: mozilla-central => try
Patch 613300 could not be applied to mozilla-central.
patching file layout/generic/crashtests/crashtests.list
Hunk #1 FAILED at 385
1 out of 1 hunks FAILED -- saving rejects to file layout/generic/crashtests/crashtests.list.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 35

[(no longer active)]

Attachment: Branch patch

Comment 36

[Alex Keybl [:akeybl]]

The deadline to get a fix into FF12 is tomorrow (Beta 5), since this issue is already present in FF11. Fixes we take in Beta 6 are inherently risky, and are meant to prevent a chemspill. If we didn't chemspill for FF11, I see no reason to do so in FF12.

tl;dr We'll be untracking for FF12 if this doesn't make it into the build tomorrow.

Comment 37

[(no longer active)]

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

Comment 38

[(no longer active)]

Attachment: Branch patch

This version of the patch changes FinishReflowWithAbsoluteFrames to actually pass aConstrainHeight down to ReflowAbsoluteFrames.

Comment 39

[(no longer active)]

Attachment: Branch patch

With the updated crashtest as per IRC conversation.  I'll also submit a crashtest interdiff for easier reviewing.

Comment 40

[(no longer active)]

Attachment: Crashtest interdiff

Comment 41

[fantasai]

Comment on attachment 613791 [details] [diff] [review]
Branch patch

r+ on the patch; for the crashtest, only the non-abspos height should be doubled -- the abspos elements don't get split into columns, right? :) But you can fix that for the trunk patch, for which this should be a proper reftest.

Comment 42

[(no longer active)]

Comment on attachment 613791 [details] [diff] [review]
Branch patch

[Approval Request Comment]
Regression caused by (bug #): bug 718516.
User impact if declined: This is a web compat layout regression which causes some elements to not be displayed on the screen in some situations.
Testing completed (on m-c, etc.): This has not landed on m-c, but this patch takes us back to the Firefox 10 behavior, and passes the tests that we have had for this.
Risk to taking this patch (and alternatives if risky): This should be fairly safe, but I would not bet my life on it.
String changes made by this patch: None.

Comment 43

[Alex Keybl [:akeybl]]

Comment on attachment 613791 [details] [diff] [review]
Branch patch

[Triage Comment]
This missed our fifth beta and carries non-zero risk, so we'll first fix this in FF13. Approved for Aurora. We'll approve for ESR once that branch's version is bumped.

Comment 44

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a2c30fffb583

Comment 45

[(no longer active)]

Unassigning since I'm not sure what needs to happen here next.  Someone else should probably take this.

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 613791 [details] [diff] [review]
Branch patch

[Triage Comment]
Now that 13 is on Beta, please go ahead and land this to ESR branch.

Comment 47

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-esr10/rev/d4ba7f6c7081

Comment 48

[(no longer active)]

Comment on attachment 613791 [details] [diff] [review]
Branch patch

Re-requesting approval for Aurora.

Comment 49

[Jonathan Kew [:jfkthame]]

Sorry, but I backed this out of ESR because it turned all the crashtests orange....
https://hg.mozilla.org/releases/mozilla-esr10/rev/b777d9ea4f95

Comment 50

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/740245b70266

Comment 51

[(no longer active)]

The test case hangs on ESR.  Great!

Jet, can you please find somebody who has some time to take a look into this?

Comment 52

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Jet so we can get someone to investigate the hanging test and drive this to landing in time for the release in just over a week.

Comment 53

[Jet Villegas (inactive)]

I'm not sure why we want this fix in an ESR release. How about we keep the code in 13 where it's green and not backport to 10 where it hangs mysteriously? AFAIK, this is not a security issue.

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

OK, I'm willing to wontfix this for ESR since it was wontfixed for 12 as well and unless this becomes a common issue for ESR users, you're right, we don't need it as it's not a security fix.

Comment 55

[Scott Johnson (:jwir3)]

(In reply to Jonathan Kew (:jfkthame) from comment #49)
> Sorry, but I backed this out of ESR because it turned all the crashtests
> orange....
> https://hg.mozilla.org/releases/mozilla-esr10/rev/b777d9ea4f95

I'm seeing this after backing out bug 695222 on release and beta (13 and 14). I'm going to have to backout csets:

https://hg.mozilla.org/releases/mozilla-beta/rev/235f5dbfe5aa
and 
https://hg.mozilla.org/releases/mozilla-release/rev/fa25fc204958

It looks like there must be something in the patch for bug 695222 that fixes the hang, and since that patch was backed out of esr 10 prior to this patch landing, it only hung there. akeybl and I discussed this on IRC, we feel that the safest route to handle this on release and beta right now is to back out this patch from FF 13 and FF 14, and set this bug as wontfix on FF 13 and FF 14.

Comment 56

[Scott Johnson (:jwir3)]

Oops, the above csets I gave were for the patches *I* pushed. Sorry. The changesets that I'm backing out that are relevant to this bug are:

https://hg.mozilla.org/releases/mozilla-beta/rev/740245b70266
and
http://hg.mozilla.org/releases/mozilla-release/rev/a2c30fffb583

Comment 57

[Scott Johnson (:jwir3)]

Backed out from beta and release:
https://hg.mozilla.org/releases/mozilla-release/rev/04011019b8f8
https://hg.mozilla.org/releases/mozilla-beta/rev/944aac352610

Comment 58

[Martin Koistinen]

OP here.  Thanks for everyone's efforts to date.  Can someone make a commitment to which one it WILL be fixed in?  This was reported 7 months and 4 full point releases of the browser ago.

Comment 59

[Scott Johnson (:jwir3)]

Martin:

We definitely appreciate your concern, but we can't commit to this bug making it into a specific release. Unfortunately, this is a highly complex bug which makes it difficult to fix without breaking something else. 

As you can see, this morning I added a new dependency for this bug, so until this dependency gets fixed, we're probably going to be unable to move forward on the current bug.

Comment 60

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan, any way forward here? (This blocks some important bugs)

Comment 61

[(no longer active)]

(In reply to comment #60)
> Ehsan, any way forward here? (This blocks some important bugs)

I'm not even working on this!  Scott was last I checked.

Comment 62

[Scott Johnson (:jwir3)]

(In reply to Ehsan Akhgari [:ehsan] from comment #61)
> (In reply to comment #60)
> > Ehsan, any way forward here? (This blocks some important bugs)
> 
> I'm not even working on this!  Scott was last I checked.

I'm not actively working on this, but I do know that it requires a fix for bug 743402, which I've been trying to resolve. Unfortunately, I'm also focused on the readability project, which is taking precedence right now.

There's a problem with bug 743402 that results in it no longer applying cleanly after the patch for column-fill landed. To add to that, the column-fill code interacts with the code from bug 743402 in a way that makes it no longer work correctly. :|

I just haven't had time to look at it in-depth for quite a while.

Comment 63

[Scott Johnson (:jwir3)]

Jet and I talked about this bug today. He was under the impression that the testcase posted in comment 25 was causing a hang on release. I have tested the following trains:

* Release (17.0)
* Beta (18.0)
* Aurora (19.0)
* Nightly (20.0)

And the testcase does not appear to be causing a hang in any of these cases.

Comment 64

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #63)
> Jet and I talked about this bug today. He was under the impression that the
> testcase posted in comment 25 was causing a hang on release. I have tested
> the following trains:
> 
> * Release (17.0)
> * Beta (18.0)
> * Aurora (19.0)
> * Nightly (20.0)
> 
> And the testcase does not appear to be causing a hang in any of these cases.

Also, it's my understanding that the patches for this bug, nor the patches for bug 695222/bug 764567 are included in the current release (17.0), beta (18.0), or aurora (19.0). A patch for bug 695222/bug 764567 IS included currently in Nightly (20.0), but may be backed out in the future, if a fix can't be found for the hang exhibited in bug 810726.

Comment 65

[Scott Johnson (:jwir3)]

Actually, it looks like the patch for bug 764567 is still in Aurora. Bug 810726 tracks this hang.

Comment 66

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

Just discussed this with fantasai and scott; we should try to get this branch patch landed on trunk rather than hold up the other things this blocks with getting abs pos children of column-sets paginating correctly.

Comment 67

[Scott Johnson (:jwir3)]

Attachment: rebased branch patch with hang fix

Ok, got the branch patch working with the crashtest. I can't determine if it fixes 836895 yet, because of another blocking issue, but I hope to get to that soon. 

Elika: Should we split out the actual changes (i.e. the non-branch patch) from this bug, so that it doesn't get lost, or do you think we should just incorporate those changes into the patch for bug 743402 when we get around to landing it (which hopefully shouldn't be too far into the future)?

Comment 68

[Scott Johnson (:jwir3)]

Attachment: actual rebased branch patch with hang fix :)

Ok, this is the real patch this time. ;)

Comment 69

[fantasai]

Comment on attachment 719251 [details] [diff] [review]
actual rebased branch patch with hang fix :)

r=fantasai, and please add p=ehsan to the checkin comment

Comment 70

[fantasai]

Also: the patch for paginating such abspos frames should probably be pulled into another bug (about paginating abspos belonging to an nsColumnSetFrame) that depends on this bug.

Comment 71

[Scott Johnson (:jwir3)]

Attachment: b724978

Another version of the patch, with (hopefully all) reftest failures corrected. Pushing to try now, as well.

Comment 72

[Scott Johnson (:jwir3)]

Comment on attachment 719761 [details] [diff] [review]
b724978

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5912993f11

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f5912993f11

Comment 74

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

Does a followup bug need to be filed on splitting the absolutely-positioned children of the columnset when the columnset splits?  Or is that problem a general problem that applies to all non-block containers of absolutely positioned frames?  Either way, it seems like there should be a bug on it.

Comment 75

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #74)
> Does a followup bug need to be filed on splitting the absolutely-positioned
> children of the columnset when the columnset splits?  Or is that problem a
> general problem that applies to all non-block containers of absolutely
> positioned frames?  Either way, it seems like there should be a bug on it.

Yes, the following are bugs tracking that (assuming I understand correctly what you mean):

bug 743402 - multicolumn frames should not run out of computed height (this is the issue described in comment 28 and comment 29).

bug 846583 - correctly paginate absolutely positioned frames that are children of a column set (this covers the issue described in comment 12)