nsColumnSetFrame should be an absolute container

RESOLVED FIXED in mozilla22

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ wontfix, firefox13+ wontfix, firefox14+ wontfix, firefox-esr10- wontfix)

Details

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

Attachments

(4 attachments, 9 obsolete attachments)

571 bytes, text/html
Details
14.89 KB, patch
Details | Diff | Splinter Review
5.82 KB, patch
Details | Diff | Splinter Review
24.04 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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
Blocks: 736072

Comment 3

5 years ago
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.
Created attachment 609539 [details] [diff] [review]
Branch patch
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

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
Moving over the tracking flags from bug 736072.
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +

Comment 7

5 years ago
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.
Attachment #609539 - Flags: review?(roc) → review+

Comment 9

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
The try server seems to be reasonably happy with me today!

Comment 11

5 years ago
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

5 years ago
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-

Comment 13

5 years ago
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.

Comment 15

5 years ago
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.

Comment 18

5 years ago
> 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. >:[
You're right. I was confused.
So, which one of the two is the desired behavior here?  :-)

Comment 21

5 years ago
Follow comment 12.
Created attachment 611475 [details] [diff] [review]
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?
Attachment #609539 - Attachment is obsolete: true
Attachment #611475 - Flags: feedback?(fantasai.bugs)

Comment 23

5 years ago
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
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?
Whiteboard: fixes regression bug 736072

Comment 25

5 years ago
Created attachment 612052 [details]
testcase

This isn't rendering correctly with the patch. (I haven't figured out why yet.)
Created attachment 612084 [details] [diff] [review]
WIP 2

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)
Created attachment 612093 [details] [diff] [review]
WIP 2

Forgot to refresh the latest version of the patch which fixes the GetSkipSides issue.
Attachment #612084 - Attachment is obsolete: true

Comment 28

5 years ago
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

5 years ago
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 32

5 years ago
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-
Created attachment 613300 [details] [diff] [review]
Branch patch
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]

Updated

5 years ago
Whiteboard: [fixes regression bug 736072][autoland-try:613300:-p all -b do -t none -u all] → [fixes regression bug 736072][autoland-in-queue]

Comment 34

5 years ago
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.

Updated

5 years ago
Whiteboard: [fixes regression bug 736072][autoland-in-queue] → [fixes regression bug 736072]
Created attachment 613328 [details] [diff] [review]
Branch patch
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]
Created attachment 613774 [details] [diff] [review]
Branch patch

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)
Created attachment 613791 [details] [diff] [review]
Branch patch

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)
Created attachment 613792 [details] [diff] [review]
Crashtest interdiff

Comment 41

5 years ago
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?

Updated

5 years ago
tracking-firefox-esr10: ? → 13+
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+
http://hg.mozilla.org/releases/mozilla-aurora/rev/a2c30fffb583
status-firefox13: --- → fixed

Updated

5 years ago
status-firefox12: --- → wontfix
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+
http://hg.mozilla.org/releases/mozilla-esr10/rev/d4ba7f6c7081
status-firefox-esr10: affected → fixed
status-firefox14: --- → affected
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
status-firefox-esr10: fixed → affected
Attachment #613791 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/740245b70266
status-firefox14: affected → fixed
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

Comment 53

5 years ago
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.

Updated

5 years ago
status-firefox-esr10: affected → ?
tracking-firefox-esr10: 13+ → ?
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.
status-firefox-esr10: ? → wontfix
tracking-firefox-esr10: ? → -
(Assignee)

Comment 55

5 years ago
(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.
status-firefox13: fixed → wontfix
status-firefox14: fixed → wontfix
(Assignee)

Comment 56

5 years ago
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
(Assignee)

Comment 57

5 years ago
Backed out from beta and release:
https://hg.mozilla.org/releases/mozilla-release/rev/04011019b8f8
https://hg.mozilla.org/releases/mozilla-beta/rev/944aac352610
(Assignee)

Updated

5 years ago
Blocks: 698783
(Assignee)

Updated

5 years ago
Depends on: 743402

Comment 58

5 years ago
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.
(Assignee)

Comment 59

5 years 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.

Updated

5 years ago
Blocks: 812893

Updated

5 years ago
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.
(Assignee)

Comment 62

5 years ago
(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.
(Assignee)

Comment 63

5 years ago
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.
(Assignee)

Comment 64

5 years ago
(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.
(Assignee)

Comment 65

5 years ago
Actually, it looks like the patch for bug 764567 is still in Aurora. Bug 810726 tracks this hang.
Blocks: 708206

Updated

4 years ago
Blocks: 691210

Updated

4 years ago
Blocks: 836895

Updated

4 years ago
Whiteboard: [fixes regression bug 736072] → [fixes regression bug 736072][fuzzblocker]

Updated

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 762902
(Assignee)

Comment 67

4 years ago
Created attachment 719250 [details] [diff] [review]
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)?
Attachment #613791 - Attachment is obsolete: true
Attachment #719250 - Flags: review?(fantasai.bugs)
(Assignee)

Comment 68

4 years ago
Created attachment 719251 [details] [diff] [review]
actual rebased branch patch with hang fix :)

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 69

4 years ago
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+

Comment 70

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 846583
(Assignee)

Comment 71

4 years ago
Created attachment 719761 [details] [diff] [review]
b724978

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)
(Assignee)

Comment 72

4 years ago
Comment on attachment 719761 [details] [diff] [review]
b724978

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5912993f11
Attachment #719761 - Flags: review?(fantasai.bugs) → review+
(Assignee)

Updated

4 years ago
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/1f5912993f11
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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+
(Assignee)

Comment 75

4 years ago
(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)

Updated

4 years ago
Blocks: 826995
You need to log in before you can comment on or make changes to this bug.