Closed Bug 724978 Opened 8 years ago Closed 7 years ago

nsColumnSetFrame should be an absolute container

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox12 + wontfix
firefox13 + wontfix
firefox14 + wontfix
firefox-esr10 - wontfix

People

(Reporter: jwir3, Assigned: jwir3)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fixes regression bug 736072][fuzzblocker])

Attachments

(4 files, 9 obsolete files)

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.
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.
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
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.
Attached patch Branch patch (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #609539 - Flags: review?(roc)
Attachment #609539 - Flags: review?(fantasai.bugs)
Whiteboard: [autoland-try:-b do -p all -u all -t none]
No longer blocks: 736072
Duplicate of this bug: 736072
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
Moving over the tracking flags from bug 736072.
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
Note to fantasai: the new abs-pos container APIs were landed in https://hg.mozilla.org/mozilla-central/rev/9989f9ef7b90.
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/autolanduser@mozilla.com-ead6782e3a3b
Whiteboard: [autoland-in-queue]
The try server seems to be reasonably happy with me today!
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 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.
Attachment #609539 - Flags: review?(fantasai.bugs) → review-
Oh, also as Scott notes, you need to add eCanContainOverflowContainers to the frame type.
I thought the goal here was to *not* paginate abs-pos children.
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 ???
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.
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.
> 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. >:[
So, which one of the two is the desired behavior here?  :-)
Follow comment 12.
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #609539 - Attachment is obsolete: true
Attachment #611475 - Flags: feedback?(fantasai.bugs)
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...
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.
Blocks: 718516
Whiteboard: fixes regression bug 736072
Attached file testcase
This isn't rendering correctly with the patch. (I haven't figured out why yet.)
Attached patch WIP 2 (obsolete) — Splinter Review
This updated patch fixes the reftest issue.  But the rendering of the test case in comment 25...
Attachment #611475 - Attachment is obsolete: true
Attachment #611475 - Flags: feedback?(fantasai.bugs)
Attached patch WIP 2Splinter Review
Forgot to refresh the latest version of the patch which fixes the GetSkipSides issue.
Attachment #612084 - Attachment is obsolete: true
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.
Filed bug 743402 on the NS_FRAME_OVERFLOW_INCOMPLETE status.
(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 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).
Attachment #609539 - Attachment description: Patch (v1) → Branch patch
Attachment #609539 - Attachment is obsolete: false
Attachment #609539 - Flags: review- → review?(fantasai.bugs)
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.
Attachment #609539 - Flags: review?(fantasai.bugs) → review-
Attached patch Branch patch (obsolete) — Splinter Review
Attachment #609539 - Attachment is obsolete: true
Attachment #613300 - Flags: review?(fantasai.bugs)
Whiteboard: fixes regression bug 736072 → [fixes regression bug 736072][autoland-try:613300:-p all -b do -t none -u all]
Whiteboard: [fixes regression bug 736072][autoland-try:613300:-p all -b do -t none -u all] → [fixes regression bug 736072][autoland-in-queue]
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.
Whiteboard: [fixes regression bug 736072][autoland-in-queue] → [fixes regression bug 736072]
Attached patch Branch patch (obsolete) — Splinter Review
Attachment #613300 - Attachment is obsolete: true
Attachment #613300 - Flags: review?(fantasai.bugs)
Attachment #613328 - Flags: review?(fantasai.bugs)
Whiteboard: [fixes regression bug 736072] → [fixes regression bug 736072][autoland-try:613328:-p all -b do -t none -u all]
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.
http://tbpl.mozilla.org/?tree=Try&rev=2cf1e3d07880
Whiteboard: [fixes regression bug 736072][autoland-try:613328:-p all -b do -t none -u all] → [fixes regression bug 736072]
Attached patch Branch patch (obsolete) — Splinter Review
This version of the patch changes FinishReflowWithAbsoluteFrames to actually pass aConstrainHeight down to ReflowAbsoluteFrames.
Attachment #613328 - Attachment is obsolete: true
Attachment #613328 - Flags: review?(fantasai.bugs)
Attachment #613774 - Flags: review?(fantasai.bugs)
Attached patch Branch patch (obsolete) — Splinter Review
With the updated crashtest as per IRC conversation.  I'll also submit a crashtest interdiff for easier reviewing.
Attachment #613774 - Attachment is obsolete: true
Attachment #613774 - Flags: review?(fantasai.bugs)
Attachment #613791 - Flags: review?(fantasai.bugs)
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.
Attachment #613791 - Flags: review?(fantasai.bugs) → review+
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.
Attachment #613791 - Flags: approval-mozilla-esr10?
Attachment #613791 - Flags: approval-mozilla-beta?
Attachment #613791 - Flags: approval-mozilla-aurora?
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.
Attachment #613791 - Flags: approval-mozilla-beta?
Attachment #613791 - Flags: approval-mozilla-beta-
Attachment #613791 - Flags: approval-mozilla-aurora?
Attachment #613791 - Flags: approval-mozilla-aurora+
Unassigning since I'm not sure what needs to happen here next.  Someone else should probably take this.
Assignee: ehsan → nobody
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.
Attachment #613791 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment on attachment 613791 [details] [diff] [review]
Branch patch

Re-requesting approval for Aurora.
Attachment #613791 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Sorry, but I backed this out of ESR because it turned all the crashtests orange....
https://hg.mozilla.org/releases/mozilla-esr10/rev/b777d9ea4f95
Attachment #613791 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The test case hangs on ESR.  Great!

Jet, can you please find somebody who has some time to take a look into this?
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.
Assignee: nobody → jet
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.
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.
(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.
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
Blocks: 698783
Depends on: 743402
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.
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.
Blocks: 641724
Ehsan, any way forward here? (This blocks some important bugs)
(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.
(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.
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.
(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.
Actually, it looks like the patch for bug 764567 is still in Aurora. Bug 810726 tracks this hang.
Blocks: 691210
Blocks: 836895
Whiteboard: [fixes regression bug 736072] → [fixes regression bug 736072][fuzzblocker]
Blocks: 839871
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.
Assignee: bugs → sjohnson
Depends on: 762902
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)?
Attachment #613791 - Attachment is obsolete: true
Attachment #719250 - Flags: review?(fantasai.bugs)
Ok, this is the real patch this time. ;)
Attachment #719250 - Attachment is obsolete: true
Attachment #719250 - Flags: review?(fantasai.bugs)
Attachment #719251 - Flags: review?(fantasai.bugs)
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
Attachment #719251 - Flags: review?(fantasai.bugs) → review+
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.
Blocks: 846583
Attached patch b724978Splinter Review
Another version of the patch, with (hopefully all) reftest failures corrected. Pushing to try now, as well.
Attachment #719251 - Attachment is obsolete: true
Attachment #719761 - Flags: review?(fantasai.bugs)
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/1f5912993f11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Attachment #613791 - Flags: approval-mozilla-esr10+
Attachment #613791 - Flags: approval-mozilla-aurora+
(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)
Blocks: 826995
You need to log in before you can comment on or make changes to this bug.