Last Comment Bug 10209 - problems with absolute or fixed position on table [ABS POS] [FIX POS]
: problems with absolute or fixed position on table [ABS POS] [FIX POS]
Status: RESOLVED FIXED
: css2, dev-doc-complete, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: x86 Linux
: P3 normal with 3 votes (vote)
: mozilla10
Assigned To: :Ehsan Akhgari
:
Mentors:
http://dbaron.org/css/test/sec1704b
: 60051 674265 (view as bug list)
Depends on: 780510 53663 655462 656130 656875 657182 659828 660588 691087 691118 691166 691210 691591 691628 691824 693107 693262 696739 708206 729114 729143 729337 729597 735579
Blocks: 203225 239310 349144 371452 63895 437722 439258 455338 512749 536692 653739 665853
  Show dependency treegraph
 
Reported: 1999-07-20 11:32 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2013-01-27 09:09 PST (History)
30 users (show)
asa: blocking1.6b-
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected
-


Attachments
Part 1: Add an API for absolute container support for all frame types (6.51 KB, patch)
2011-04-28 15:36 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
[WIP] Part 2: Implement the absolute positioning support for all frames (48.84 KB, patch)
2011-04-28 15:38 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Reflow parts [WIP -- doesn't even compile, probably doesn't make sense either] (3.35 KB, patch)
2011-04-28 15:40 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] Part 2: Implement the absolute positioning support for all frames (27.45 KB, patch)
2011-04-28 21:20 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] Part 2: Implement the absolute positioning support for all frames (80.52 KB, patch)
2011-04-29 16:15 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] Part 2: Implement the absolute positioning support for all frames (85.32 KB, patch)
2011-05-02 12:27 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (86.92 KB, patch)
2011-05-02 17:22 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: Add an API for absolute container support for all frame types (6.57 KB, patch)
2011-05-03 08:41 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (87.41 KB, patch)
2011-05-03 16:47 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (88.25 KB, patch)
2011-05-04 16:58 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (88.19 KB, patch)
2011-05-05 15:39 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (87.96 KB, patch)
2011-05-05 15:54 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
[WIP] Part 3: Make tables absolute positioning containers (6.76 KB, patch)
2011-05-06 15:37 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] Part 3: Make tables absolute positioning containers (16.55 KB, patch)
2011-05-09 17:41 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock (35.14 KB, patch)
2011-05-11 21:18 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock (34.67 KB, patch)
2011-05-17 08:49 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock (36.71 KB, patch)
2011-05-19 06:36 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Aurora backout (93.30 KB, patch)
2011-05-24 17:37 PDT, :Ehsan Akhgari
asa: approval‑mozilla‑aurora+
mounir: checkin+
Details | Diff | Splinter Review
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock (36.79 KB, patch)
2011-05-24 17:39 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
part 3.1: Replace IsContainingBlock with GetContainingBlock (38.80 KB, patch)
2011-06-10 15:53 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
part 3.1: Replace IsContainingBlock with GetContainingBlock (42.28 KB, patch)
2011-06-19 13:27 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
part 3.1: Replace IsContainingBlock with GetContainingBlock (43.11 KB, patch)
2011-06-22 17:51 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
part 3.1: Replace IsContainingBlock with GetContainingBlock (43.14 KB, patch)
2011-06-23 11:37 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
part 3.1: Replace IsContainingBlock with GetContainingBlock (43.86 KB, patch)
2011-06-24 15:38 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (84.59 KB, patch)
2011-09-22 15:34 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
Part 2: Implement the absolute positioning support for all frames (84.74 KB, patch)
2011-09-22 19:57 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2.1: Adjust the assertion count on the test case for bug 348729 (875 bytes, patch)
2011-09-26 15:37 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
Trimmed log of when the bug happens with the testcase of bug 637597 (250.01 KB, text/plain)
2011-09-28 20:16 PDT, :Ehsan Akhgari
no flags Details
Trimmed log of when the bug happens with the testcase of bug 637597 (only the layer tree) (10.10 KB, text/plain)
2011-09-28 20:18 PDT, :Ehsan Akhgari
no flags Details
Trimmed log of when the bug happens with the testcase of bug 637597 (non-patched build) (271.35 KB, text/plain)
2011-09-28 21:14 PDT, :Ehsan Akhgari
no flags Details
Part 4: Mark the reftest for bug 637597 as random (1.22 KB, patch)
2011-09-29 10:32 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 1999-07-20 11:32:44 PDT
The above URL ( http://www.fas.harvard.edu/~dbaron/css/test/sec1704b ) shouldn't
work completely until bug 2479 is fixed.  However, there may be some other
problems here, too, so I'm filing a separate bug.

The page involves fixed position on a table (same thing happens for absolute).
What should happen is:

1) fixed/absolute positioned table gets display: block
2) anonymous element with display table is put around the table (bug 2479)
3) you get a fixed positioned table

The problem in (2) is usually that the content is deleted rather than making an
anonymous box.  So this makes me think (1) isn't happening correctly, or
something else weird is happening.  You may want to wait on this bug until 2479
is fixed, though.

Right now, the table is being absolutely positioned above and to the left of its
natural position.

The above URL also gives the following assertions when I load the page:
Assertion: "bad parent" (parent == aParent) at file nsFrameList.cpp, line 342
Assertion: "no placeholder frame" (nsnull != placeholderFrame) at file
nsHTMLReflowState.cpp, line 408
Assertion: "no placeholder frame" (nsnull != placeholderFrame) at file
nsHTMLReflowState.cpp, line 408
Comment 1 troy 1999-07-20 11:38:59 PDT
Chris, it's been a while since we talked about absolute/fixed position tables.
We used to hit an assert, what's the current status?
Comment 2 karnaze (gone) 1999-10-04 19:56:59 PDT
The page doesn't assert. This bug has not gotten much attention, in favor of
html4 and css 1 issues.
Comment 3 karnaze (gone) 1999-12-01 13:21:59 PST
mass move to m14.
Comment 4 Hixie (not reading bugmail) 2000-01-13 16:08:59 PST
Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...
Comment 5 karnaze (gone) 2000-05-07 16:34:52 PDT
Moving to M17.
Comment 6 karnaze (gone) 2000-06-07 14:03:44 PDT
This bug has been marked "future" because we have determined that it is not 
critical for netscape 6.0. If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.
Comment 7 sungod 2000-07-10 16:27:26 PDT
If CSS2 positioning is indeed to be back-burnered, I must highly encourage the
complete commenting of all CSS2 positioning code that isn't also in CSS1.

IMO, we MUST NOT allow Mozilla's positioning bugs to render correctly-designed,
by-the-spec CSS2 documents brokenly. We must either support these features per
the spec, or turn them off.

Netscape 4 makes the mistake of rendering CSS1 with severe bugs, making pages
with correct CSS completely unreadable. This ruins the point of abstracting
structure from presentation. I don't think we should allow this in Mozilla.

On the other hand, if we can reach a concensus that we can get CSS2 positioning
working correctly in time for a stable public release (like netscape's?) I would
of course LOVE to have that support included.
Comment 8 Peter ``jag'' Annema 2000-07-10 17:43:38 PDT
I have to agree with sungod@atdot.org. If we can't get CSS2 specific positioning 
right per the spec, I'd rather it be turned off until such a time that it can be 
done right. A broken implementation would only be a burden.
Comment 9 Hixie (not reading bugmail) 2000-07-11 02:43:23 PDT
We should check there are no showstoppers if we are going to release it like 
this. We certainly don't want to kill absolute positioning.

I'll do this in a few weeks. No need to panic. I have a feeling IE gets in 
wrong anyway. Unfortunately David's test case is not yet complete so this will
need some thought.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2000-08-02 08:29:33 PDT
You do get a fixed position table now. But it appears to be in the wrong size 
and position. The caption spans the page and the table itself is touching the 
right edge of the page (and overlaps the scrollbar, but that's another issue).
Comment 11 R.K.Aa. 2000-11-14 11:03:04 PST
is bug 60051 a dup of this?
Comment 12 Keyser Sose 2000-11-28 13:43:44 PST
*** Bug 60051 has been marked as a duplicate of this bug. ***
Comment 13 Amarendra Hanumanula 2001-03-22 13:29:05 PST
QA contact update
Comment 14 Boris Zbarsky [:bz] 2002-04-17 21:04:49 PDT
The scrollbar overlapping problem has a patch in bug 94009
Comment 15 Greg K. 2002-07-13 11:55:16 PDT
So what specific issues is this bug about pertaining to the testcase?
Comment 16 karnaze (gone) 2003-03-31 11:17:21 PST
mass reassign to default owner
Comment 17 brewthatistrue 2004-02-29 23:45:27 PST
is bug 236077 related?
Comment 18 Robin Monks 2004-07-24 11:30:27 PDT
Is this bug still around/active?  If so, shouldn't the qawanted tag be removed
from the keywords?
Comment 19 Robin Monks 2004-10-22 11:56:24 PDT
  Reporter, is this bug still active in newer builds?

  You have 7-10 days to respond, after which time it would be OK for the bug to
be marked INVALID.  You can respond via the link you will recieve in the
bugzilla Email or via the comment blank.
Comment 20 timeless 2004-10-22 12:04:09 PDT
mozillamonks@yahoo.ca: for lack of common sense you've lost editbugs. please
take this moment to think about your actions.
Comment 21 Bernd 2004-10-22 12:06:43 PDT
What the f..., is going on here? If David files a bug it is a bug period. Please
stop immeadetely to spam bugs if you have no clue. I asked to adjust your
privilegs to your knowledge of bugzilla and mozilla in general. Broad hint:
David is the owner of the layout component. 
Comment 22 Robin Monks 2004-10-23 16:16:03 PDT
(In reply to comment #20)
> mozillamonks@yahoo.ca: for lack of common sense you've lost editbugs. please
> take this moment to think about your actions.

  I'm very sorry, I didn't know David owned that
component.  I figured when he didn't respond after a
few weeks that he was just another bug reporter who
didn't show up again.
  I followed the guidelines for message templates
saying that one could say a time for a reporter to
respond and the if s/he was using an alternate system
that one could still say it, just not carry it out.
  I ask that you reconsider you decision.  I will take
time to study the names of the component owners so I
don't offend.  Could you suggest some other farther
reading I should read up on?
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-23 16:35:56 PDT
The spec has changed significantly since this bug was filed, and someone should
probably figure out how we deviate from the current spec, file appropriate bugs,
and close this one.

However, using a template like the one you used may make sense for a bug you
can't reproduce, but doesn't make sense for a bug you simply don't understand.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-15 09:29:18 PDT
See also bug 63895.
Comment 25 :Ehsan Akhgari 2011-04-28 15:09:47 PDT
I have a patch which refactors nearly all of the abs-pos child handling stuff to nsFrame, so that all of our frame types can use this implementation.  There is one last thing which I don't know how to do with the minimum level of code change, and that is reflowing the absolute children.

The reflowing should be independent of the reflowing of the frame itself, so at first I thought that a neat trick is to reflow the abs-pos children in nsFrame::DidReflow.  But it seems like that may be too late in the reflow process.  For example, we don't have an nsReflowStatus to modify any more.

Does anybody have any suggestions on how to solve this problem?  The last thing that I want to do is to modify all of our Reflow functions to take this into account...
Comment 26 :Ehsan Akhgari 2011-04-28 15:36:13 PDT
Created attachment 528970 [details] [diff] [review]
Part 1: Add an API for absolute container support for all frame types

I think this first part is good enough (it might need minor corrections).  It adds a bunch of APIs to nsIFrame to maintain the absolute children info on every frame type.
Comment 27 :Ehsan Akhgari 2011-04-28 15:38:21 PDT
Created attachment 528971 [details] [diff] [review]
[WIP] Part 2: Implement the absolute positioning support for all frames

This part is not finished yet.  The main things which are missing from it is the reflow piece, removing all of the old code to maintain abs-pos children in nsViewportFrame, nsCanvasFrame, nsBlockFrame and removing nsPositionedInlineFrame entirely.

Oh, and of course, testing!  So far all of the testing that I've been able to do is to make sure that the changes compile...
Comment 28 :Ehsan Akhgari 2011-04-28 15:40:21 PDT
Created attachment 528973 [details] [diff] [review]
Reflow parts [WIP -- doesn't even compile, probably doesn't make sense either]

This part of the patch doesn't even compile, let alone being correct in any way.  This patch reflects what I was trying in comment 25.
Comment 29 :Ehsan Akhgari 2011-04-28 15:42:25 PDT
At this point I'm mostly stuck with comment 25.  Once the reflow stuff is done, I think I can just remove the stuff I mentioned in comment 27 and start testing the code.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 18:03:09 PDT
(In reply to comment #25)
> The reflowing should be independent of the reflowing of the frame itself, so at
> first I thought that a neat trick is to reflow the abs-pos children in
> nsFrame::DidReflow.  But it seems like that may be too late in the reflow
> process.  For example, we don't have an nsReflowStatus to modify any more.
> 
> Does anybody have any suggestions on how to solve this problem?  The last thing
> that I want to do is to modify all of our Reflow functions to take this into
> account...

Reflowing the absolute children needs to be done near the end of each Reflow implementation, because it has to affect the overflow area stored in nsHTMLReflowMetrics before Reflow returns. We do need to modify all the Reflow methods.

So we'll want a function that abstracts the code in nsPositionedInlineFrame and nsCanvasFrame (I think the code in nsPositionedInlineFrame should work). Instead of adding a new function call in every Reflow method, I think you should add a method FinishReflowWithAbsoluteFrames which does your absolute stuff and also calls nsFrame::FinishAndStoreOverflow, and change existing FinishAndStoreOverflow calls to call FinishReflowWithAbsoluteFrames. Those changes can be done incrementally. (You probably do not want to convert nsBlockFrame since that does various optimizations we don't need in general.)
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 18:03:48 PDT
Although we probably should convert the block code from using its mAbsoluteContainer to your new property-based code.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 18:04:34 PDT
 virtual nsIAtom* GetAbsoluteListName() const { return nsGkAtoms::fixedList; }

Should return nsGkAtoms::absoluteList
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 18:10:24 PDT
nsFrame::GetAdditionalChildListName needs to return null if aIndex > 0.

NS_CONTAINER_FRAME_OVERFLOW_LIST_INDEX and the other list indexes next to it need to change.

Instead of calling nsFrame::BuildDisplayListForAbsoluteChildren everywhere, you can probably just call it from nsFrame::BuildDisplayListForStackingContext and nsFrame::BuildDisplayListForChild.
Comment 34 :Ehsan Akhgari 2011-04-28 20:28:56 PDT
(In reply to comment #32)
>  virtual nsIAtom* GetAbsoluteListName() const { return nsGkAtoms::fixedList; }
> 
> Should return nsGkAtoms::absoluteList

Are you sure?  That method is defined in nsViewportFrame, as that is the only type of frame which does abs-pos child processing with postition:fixed children (that's what the current code does, and I think that's what we want it to do).  If we should return absoluteList for nsViewportFrame as well, then I can probably remove GetAbsoluteListName altogether, since this difference is the only reason I had to add it.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 20:53:29 PDT
Ah right.

We probably shouldn't make nsViewportFrame use the common mechanism you're building here. It should manage its fixed-pos frames itself.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 20:54:57 PDT
I take back comment #35. Go ahead with GetAbsoluteListName().
Comment 37 :Ehsan Akhgari 2011-04-28 21:19:37 PDT
(In reply to comment #33)
> nsFrame::GetAdditionalChildListName needs to return null if aIndex > 0.
> 
> NS_CONTAINER_FRAME_OVERFLOW_LIST_INDEX and the other list indexes next to it
> need to change.

Done.

May I pause for a moment to express my discontent at the way GetAdditionalChildListName is designed?  The way that it works now, it's impossible to figure out what needs to happen in order to change something here without looking at all of the definitions of this function.  :(  Why can't each implementation just return an array of child list names it supports?

> Instead of calling nsFrame::BuildDisplayListForAbsoluteChildren everywhere, you
> can probably just call it from nsFrame::BuildDisplayListForStackingContext and
> nsFrame::BuildDisplayListForChild.

Done.  I wish I knew this before, this makes the patch a lot smaller!  :-)
Comment 38 :Ehsan Akhgari 2011-04-28 21:20:10 PDT
(In reply to comment #36)
> I take back comment #35. Go ahead with GetAbsoluteListName().

That makes sense, since all of the other handling should be exactly the same.
Comment 39 :Ehsan Akhgari 2011-04-28 21:20:51 PDT
Created attachment 529036 [details] [diff] [review]
[WIP] Part 2: Implement the absolute positioning support for all frames

Newer version.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 21:39:48 PDT
(In reply to comment #37)
> May I pause for a moment to express my discontent at the way
> GetAdditionalChildListName is designed?  The way that it works now, it's
> impossible to figure out what needs to happen in order to change something here
> without looking at all of the definitions of this function.  :(  Why can't each
> implementation just return an array of child list names it supports?

Yes.

Really I think the whole GetAdditionalChildListName dance should be replaced with something simpler and faster. Right now traversing all the child frame lists requires two virtual calls per child list the frame supports, which is way more than necessary.

E.g. we could have
  struct ChildList {
    nsIAtom* mName;
    nsIFrame* mFirstChild;
  };
  /** Appends one ChildList element for every non-empty child list to aLists */
  virtual void GetChildLists(nsTArray<ChildList>* aLists);

then you could write
  nsAutoTArray<ChildList,4> childLists;
  frame->GetChildLists(&childLists);
  for (PRUint32 i = 0; i < childLists.Length(); ++i) {
    for (nsIFrame* f = childLists[i].mFirstChild; f = f->GetNextSibling()) {
      ...
    }
  }

The implementors could write
  void nsXYZFrame::GetChildLists(nsTArray<ChildList>* aLists)
  {
    nsSuperClassFrame::GetChildLists(aLists);
    if (!mMyExtraList.IsEmpty()) {
      aLists->AppendElement(ChildList(nsGkAtoms::myExtraList, mMyExtraList.FirstFrame());
    }
  }
we could even add a helper function nsFrameList::AppendIfNonEmpty(nsTArray<ChildList>* aLists, nsIAtom* aListName).
Comment 41 :Ehsan Akhgari 2011-04-28 21:47:09 PDT
Filed bug 653649 for comment 40.
Comment 42 :Ehsan Akhgari 2011-04-28 22:13:06 PDT
Note to myself: http://pastebin.mozilla.org/1215193
This won't make sense to anyone, it's a random irc log which I didn't have time to clean up! :)
Comment 43 :Ehsan Akhgari 2011-04-29 11:56:32 PDT
Actually, nsAbsoluteContainingBlock doesn't support reflowing on frames not derived from nsContainerFrame for now...
Comment 44 :Ehsan Akhgari 2011-04-29 16:15:40 PDT
Created attachment 529229 [details] [diff] [review]
[WIP] Part 2: Implement the absolute positioning support for all frames

This new version should be roughly complete as far as simulating the current behavior is concerned.  It needs a lot more testing though.
Comment 45 :Ehsan Akhgari 2011-05-02 12:27:55 PDT
Created attachment 529545 [details] [diff] [review]
[WIP] Part 2: Implement the absolute positioning support for all frames

This version of the patch passes the entire crashtest suite for me locally (yay!!!!!)
Comment 46 :Ehsan Akhgari 2011-05-02 17:22:43 PDT
Created attachment 529613 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

This version passes all of the reftests for me locally as well.  I've submitted it to the try server.
Comment 47 :Ehsan Akhgari 2011-05-02 17:23:08 PDT
Comment on attachment 528970 [details] [diff] [review]
Part 1: Add an API for absolute container support for all frame types

This is ready for review.
Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-02 17:47:04 PDT
Comment on attachment 528970 [details] [diff] [review]
Part 1: Add an API for absolute container support for all frame types

Review of attachment 528970 [details] [diff] [review]:

::: layout/generic/nsFrame.cpp
@@ +268,5 @@
+NS_DECLARE_FRAME_PROPERTY(AbsoluteContainingBlockProperty, DestroyAbsoluteContainingBlock)
+
+PRBool
+nsIFrame::HasAbsolutelyPositionedChildren() const {
+  return IsAbsoluteContainer() ? GetAbsoluteContainingBlock()->HasAbsoluteFrames() : PR_FALSE;

IsAbsoluteContainer() && GetAbsoluteContainingBlock()->HasAbsoluteFrames()

If PR_TRUE or PR_FALSE appears in a ?: expression, you're doing something wrong :-).
Comment 49 :Ehsan Akhgari 2011-05-03 08:38:56 PDT
(In reply to comment #48)
> +PRBool
> +nsIFrame::HasAbsolutelyPositionedChildren() const {
> +  return IsAbsoluteContainer() ?
> GetAbsoluteContainingBlock()->HasAbsoluteFrames() : PR_FALSE;
> 
> IsAbsoluteContainer() && GetAbsoluteContainingBlock()->HasAbsoluteFrames()
> 
> If PR_TRUE or PR_FALSE appears in a ?: expression, you're doing something wrong
> :-).

FWIW, I wrote this on purpose, because I think the "if"-resembling of ?: sometimes helps readability, but I have no objection to change it.  ;-)
Comment 50 :Ehsan Akhgari 2011-05-03 08:41:25 PDT
Created attachment 529728 [details] [diff] [review]
Part 1: Add an API for absolute container support for all frame types

With review comments addressed.
Comment 51 :Ehsan Akhgari 2011-05-03 08:44:00 PDT
Comment on attachment 529613 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

This should be ready for review.

These two patches shouldn't have any change on the current behavior.  I'd like to land these parts sooner than the rest of the patches in this bug (which I have yet to write).
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-03 16:06:18 PDT
Comment on attachment 529613 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Review of attachment 529613 [details] [diff] [review]:

I like this very much!

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1225,5 @@
+    // absolute containing block
+    if (aChildListName == containingBlock->GetAbsoluteListName()) {
+      if (!containingBlock->IsAbsoluteContainer()) {
+        containingBlock->MarkAsAbsoluteContainingBlock();
+      }

Why is this necessary? Seems to me that containingBlock should already have been marked as an absolute container.

@@ +1230,5 @@
+      rv = containingBlock->GetAbsoluteContainingBlock()->
+           SetInitialChildList(containingBlock, aChildListName, aFrameItems);
+    } else {
+      rv = containingBlock->SetInitialChildList(aChildListName, aFrameItems);
+    }

An alternative would be to have SetInitialChildList handle this case itself. I can see that would probably end up being more code, though.

Anyway, nsIFrame::SetInitialChildList should be documented to say that it doesn't handle GetAbsoluteListName().

@@ +1246,5 @@
+    // correctly.
+    if (aChildListName == containingBlock->GetAbsoluteListName() &&
+        !containingBlock->IsAbsoluteContainer()) {
+      containingBlock->MarkAsAbsoluteContainingBlock();
+    }

Again I would have expected containingBlock to already be one.

::: layout/base/nsFrameManager.cpp
@@ +535,5 @@
+      aListName == parentFrame->GetAbsoluteListName()) {
+    parentFrame->GetAbsoluteContainingBlock()->
+      RemoveFrame(parentFrame, aListName, aOldFrame);
+  } else {
+    rv = aOldFrame->GetParent()->RemoveFrame(aListName, aOldFrame);

use parentFrame on this line

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +426,5 @@
 
   PRBool constrainHeight = (aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE)
     && aConstrainHeight
        // Don't split if told not to (e.g. for fixed frames)
+    && !(aDelegatingFrame->GetType() == nsGkAtoms::inlineFrame && aDelegatingFrame->IsAbsoluteContainer())

We shouldn't check IsAbsoluteContainer here --- if it's not an absolute container we shouldn't be calling into nsAbsoluteContainingBlock!

If you want, you could assert aDelegatingFrame->IsAbsoluteContainer somewhere.

::: layout/generic/nsBlockFrame.cpp
@@ -304,5 @@
 
 void
 nsBlockFrame::DestroyFrom(nsIFrame* aDestructRoot)
 {
-  mAbsoluteContainer.DestroyFrames(this, aDestructRoot);

This is actually a little bit scary. We don't want to mess with frame destruction order. We should continue to tear down the absolute frames before we destroy their placeholders.

@@ +6238,5 @@
     }
   }
 
   aBuilder->MarkFramesForDisplayList(this, mFloats, aDirtyRect);
+  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);

Is this needed, given you've added the call to BuildDisplayListForStackingContext/Child?

::: layout/generic/nsCanvasFrame.cpp
@@ +276,5 @@
   if (GetPrevInFlow()) {
     DisplayOverflowContainers(aBuilder, aDirtyRect, aLists);
   }
 
+  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);

Is this needed, given you've added the call to BuildDisplayListForStackingContext/Child?

::: layout/generic/nsFrame.cpp
@@ +1854,5 @@
   return NS_OK;
 }
 
 void
+nsIFrame::BuildDisplayListForAbsoluteChildren(nsDisplayListBuilder* aBuilder,

This should be called something else since we're not actually building display items here, just ensuring that they *will* be built when placeholders are traversed.

How about just MarkAbsoluteFramesForDisplayList?

@@ +3637,5 @@
+nsFrame::FinishReflowWithAbsoluteFrames(nsPresContext*           aPresContext,
+                                        nsHTMLReflowMetrics&     aDesiredSize,
+                                        const nsHTMLReflowState& aReflowState,
+                                        nsReflowStatus&          aStatus,
+                                        PRBool                   aSkipFinish)

Instead of aSkipFinish, I think it's worth having a separate method ReflowAbsoluteFrames that does everything except the FinishAndStoreOverflow.

@@ +3646,5 @@
+    // Let the absolutely positioned container reflow any absolutely positioned
+    // child frames that need to be reflowed
+    // We want to do this under either of two conditions:
+    //  1. If we didn't do the incremental reflow above.
+    //  2. If our size changed.

But we're not checking those here, so this comment needs to be fixed.

@@ +3666,5 @@
+
+    // Factor the absolutely positioned child bounds into the overflow area
+    // Don't include this frame's bounds, nor its inline descendants' bounds,
+    // and don't store the overflow property.
+    // That will all be done by nsLineLayout::RelativePositionFrames.

This comment is inappropriate here.

::: layout/generic/nsInlineFrame.cpp
@@ +184,5 @@
 nsInlineFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
                                 const nsRect&           aDirtyRect,
                                 const nsDisplayListSet& aLists)
 {
+  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);

Shouldn't be needed, as above.

::: layout/generic/nsPageContentFrame.cpp
@@ +142,2 @@
   nsReflowStatus fixedStatus = NS_FRAME_COMPLETE;
+  FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, aReflowState, fixedStatus);

You've effectively moved FinishAndStoreOverflow up here, but that's wrong since FinishAndStoreOverflow needs to run after we've set aDesiredSize.width/height.

::: layout/generic/nsViewportFrame.cpp
@@ +92,5 @@
   // We don't need any special painting or event handling. We just need to
   // mark our visible out-of-flow frames (i.e., the fixed position frames) so
   // that display list construction is guaranteed to recurse into their
   // ancestors.
+  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);

Shouldn't be needed.

@@ +255,2 @@
 #ifdef DEBUG
+  if (IsAbsoluteContainer()) {

I think the case when this is not an absolute container is a page in print preview, right? You might want to mention that in a comment.
Comment 53 :Ehsan Akhgari 2011-05-03 16:47:40 PDT
Created attachment 529888 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

(In reply to comment #52)
> Comment on attachment 529613 [details] [diff] [review] [review]
> Part 2: Implement the absolute positioning support for all frames
> 
> Review of attachment 529613 [details] [diff] [review] [review]:
> 
> I like this very much!
> 
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +1225,5 @@
> +    // absolute containing block
> +    if (aChildListName == containingBlock->GetAbsoluteListName()) {
> +      if (!containingBlock->IsAbsoluteContainer()) {
> +        containingBlock->MarkAsAbsoluteContainingBlock();
> +      }
> 
> Why is this necessary? Seems to me that containingBlock should already have
> been marked as an absolute container.

Not if the element in question has its style changed dynamically, for example by setting elem.style.position.

> @@ +1230,5 @@
> +      rv = containingBlock->GetAbsoluteContainingBlock()->
> +           SetInitialChildList(containingBlock, aChildListName, aFrameItems);
> +    } else {
> +      rv = containingBlock->SetInitialChildList(aChildListName, aFrameItems);
> +    }
> 
> An alternative would be to have SetInitialChildList handle this case itself. I
> can see that would probably end up being more code, though.
> 
> Anyway, nsIFrame::SetInitialChildList should be documented to say that it
> doesn't handle GetAbsoluteListName().

Did the latter.

> @@ +1246,5 @@
> +    // correctly.
> +    if (aChildListName == containingBlock->GetAbsoluteListName() &&
> +        !containingBlock->IsAbsoluteContainer()) {
> +      containingBlock->MarkAsAbsoluteContainingBlock();
> +    }
> 
> Again I would have expected containingBlock to already be one.

See above.

> ::: layout/base/nsFrameManager.cpp
> @@ +535,5 @@
> +      aListName == parentFrame->GetAbsoluteListName()) {
> +    parentFrame->GetAbsoluteContainingBlock()->
> +      RemoveFrame(parentFrame, aListName, aOldFrame);
> +  } else {
> +    rv = aOldFrame->GetParent()->RemoveFrame(aListName, aOldFrame);
> 
> use parentFrame on this line

Done.

> ::: layout/generic/nsAbsoluteContainingBlock.cpp
> @@ +426,5 @@
> 
>    PRBool constrainHeight = (aReflowState.availableHeight !=
> NS_UNCONSTRAINEDSIZE)
>      && aConstrainHeight
>         // Don't split if told not to (e.g. for fixed frames)
> +    && !(aDelegatingFrame->GetType() == nsGkAtoms::inlineFrame &&
> aDelegatingFrame->IsAbsoluteContainer())
> 
> We shouldn't check IsAbsoluteContainer here --- if it's not an absolute
> container we shouldn't be calling into nsAbsoluteContainingBlock!

Hmm, right!

> If you want, you could assert aDelegatingFrame->IsAbsoluteContainer somewhere.

GetAbsoluteContainingBlock() already asserts this.  No point in asserting twice I guess.

> ::: layout/generic/nsBlockFrame.cpp
> @@ -304,5 @@
> 
>  void
>  nsBlockFrame::DestroyFrom(nsIFrame* aDestructRoot)
>  {
> -  mAbsoluteContainer.DestroyFrames(this, aDestructRoot);
> 
> This is actually a little bit scary. We don't want to mess with frame
> destruction order. We should continue to tear down the absolute frames before
> we destroy their placeholders.

Hmm, even if the current code is not depending on this order?

I'd like to avoid modifying all of the DestroyFrom implementations if possible.  If this really scares you, I guess I'll make DestroyFrom protected, make existing callers to call Destroy instead, and modify nsIFrame::Destroy to handle the anon-pos children destruction...

> @@ +6238,5 @@
>      }
>    }
> 
>    aBuilder->MarkFramesForDisplayList(this, mFloats, aDirtyRect);
> +  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);
> 
> Is this needed, given you've added the call to
> BuildDisplayListForStackingContext/Child?

Yeah, I remember some reftest somewhere failing without this, which caused me to add it back...

> ::: layout/generic/nsCanvasFrame.cpp
> @@ +276,5 @@
>    if (GetPrevInFlow()) {
>      DisplayOverflowContainers(aBuilder, aDirtyRect, aLists);
>    }
> 
> +  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);
> 
> Is this needed, given you've added the call to
> BuildDisplayListForStackingContext/Child?

Yeah, see above.

> ::: layout/generic/nsFrame.cpp
> @@ +1854,5 @@
>    return NS_OK;
>  }
> 
>  void
> +nsIFrame::BuildDisplayListForAbsoluteChildren(nsDisplayListBuilder* aBuilder,
> 
> This should be called something else since we're not actually building display
> items here, just ensuring that they *will* be built when placeholders are
> traversed.
> 
> How about just MarkAbsoluteFramesForDisplayList?

Done.

> @@ +3637,5 @@
> +nsFrame::FinishReflowWithAbsoluteFrames(nsPresContext*           aPresContext,
> +                                        nsHTMLReflowMetrics&     aDesiredSize,
> +                                        const nsHTMLReflowState& aReflowState,
> +                                        nsReflowStatus&          aStatus,
> +                                        PRBool                   aSkipFinish)
> 
> Instead of aSkipFinish, I think it's worth having a separate method
> ReflowAbsoluteFrames that does everything except the FinishAndStoreOverflow.

Done.

While addressing the rest of the comments, FinishReflowWithAbsoluteFrames is now called only once.  Is it worth keeping it around, or should I just remove it?

> @@ +3646,5 @@
> +    // Let the absolutely positioned container reflow any absolutely
> positioned
> +    // child frames that need to be reflowed
> +    // We want to do this under either of two conditions:
> +    //  1. If we didn't do the incremental reflow above.
> +    //  2. If our size changed.
> 
> But we're not checking those here, so this comment needs to be fixed.
> 
> @@ +3666,5 @@
> +
> +    // Factor the absolutely positioned child bounds into the overflow area
> +    // Don't include this frame's bounds, nor its inline descendants' bounds,
> +    // and don't store the overflow property.
> +    // That will all be done by nsLineLayout::RelativePositionFrames.
> 
> This comment is inappropriate here.

Sorry, I had just moved the comments from nsPositionedInlineFrame, and forgot to fix them.

> ::: layout/generic/nsInlineFrame.cpp
> @@ +184,5 @@
>  nsInlineFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>                                  const nsRect&           aDirtyRect,
>                                  const nsDisplayListSet& aLists)
>  {
> +  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);
> 
> Shouldn't be needed, as above.

See above.

> ::: layout/generic/nsPageContentFrame.cpp
> @@ +142,2 @@
>    nsReflowStatus fixedStatus = NS_FRAME_COMPLETE;
> +  FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, aReflowState,
> fixedStatus);
> 
> You've effectively moved FinishAndStoreOverflow up here, but that's wrong since
> FinishAndStoreOverflow needs to run after we've set aDesiredSize.width/height.

Fixed.

> ::: layout/generic/nsViewportFrame.cpp
> @@ +92,5 @@
>    // We don't need any special painting or event handling. We just need to
>    // mark our visible out-of-flow frames (i.e., the fixed position frames) so
>    // that display list construction is guaranteed to recurse into their
>    // ancestors.
> +  BuildDisplayListForAbsoluteChildren(aBuilder, aDirtyRect);
> 
> Shouldn't be needed.

See above.

> @@ +255,2 @@
>  #ifdef DEBUG
> +  if (IsAbsoluteContainer()) {
> 
> I think the case when this is not an absolute container is a page in print
> preview, right? You might want to mention that in a comment.

Hmm, no, it should also happen if we don't have any fixed-pos elements, right?  I mean, the code before this tested for the fixed container list being empty, but now instead of the container list being empty, it may not exist at all.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-03 17:50:20 PDT
(In reply to comment #53)
> Not if the element in question has its style changed dynamically, for example
> by setting elem.style.position.

Changing the position style will force frame reconstruction.

> > ::: layout/generic/nsBlockFrame.cpp
> > @@ -304,5 @@
> > 
> >  void
> >  nsBlockFrame::DestroyFrom(nsIFrame* aDestructRoot)
> >  {
> > -  mAbsoluteContainer.DestroyFrames(this, aDestructRoot);
> > 
> > This is actually a little bit scary. We don't want to mess with frame
> > destruction order. We should continue to tear down the absolute frames before
> > we destroy their placeholders.
> 
> Hmm, even if the current code is not depending on this order?

I believe it is. Maybe our tests are inadequate.

> I'd like to avoid modifying all of the DestroyFrom implementations if possible.
>  If this really scares you, I guess I'll make DestroyFrom protected, make
> existing callers to call Destroy instead, and modify nsIFrame::Destroy to
> handle the anon-pos children destruction...

You can't just do that since you need to pass the aDestructRoot.

I think for now just throw in calls to a new helper function like DestroyAbsoluteFrames(). If the order really doesn't matter, then I suggest changing the order in a separate patch. That will reduce risk.

> > Is this needed, given you've added the call to
> > BuildDisplayListForStackingContext/Child?
> 
> Yeah, I remember some reftest somewhere failing without this, which caused me
> to add it back...

I would like to understand why it's needed. Maybe we're fixing that failure the wrong way.

> While addressing the rest of the comments, FinishReflowWithAbsoluteFrames is
> now called only once.  Is it worth keeping it around, or should I just remove
> it?

Probably keep it around, since it will be the right thing for nsTableFrame::Reflow to call.

> > @@ +255,2 @@
> >  #ifdef DEBUG
> > +  if (IsAbsoluteContainer()) {
> > 
> > I think the case when this is not an absolute container is a page in print
> > preview, right? You might want to mention that in a comment.
> 
> Hmm, no, it should also happen if we don't have any fixed-pos elements, right? 

Whether a ViewportFrame is an absolute container should not depend on whether it has any fixed-pos children.
Comment 55 :Ehsan Akhgari 2011-05-03 20:25:56 PDT
(In reply to comment #54)
> (In reply to comment #53)
> > Not if the element in question has its style changed dynamically, for example
> > by setting elem.style.position.
> 
> Changing the position style will force frame reconstruction.

Well, I was wrong about the reason why this is needed.  This code is necessary for fixed position stuff.  It's how we make the viewport frame as an absolute container.  (I found it by taking that code out and running the reftests, which makes us crash on <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/76331-1.html?force=1>, which has a fixed position element.)  I _could_ probably get rid of this code by making the viewport frame always be an absolute containing block, but I think the current code is cleaner (also, see below).

> > I'd like to avoid modifying all of the DestroyFrom implementations if possible.
> >  If this really scares you, I guess I'll make DestroyFrom protected, make
> > existing callers to call Destroy instead, and modify nsIFrame::Destroy to
> > handle the anon-pos children destruction...
> 
> You can't just do that since you need to pass the aDestructRoot.
> 
> I think for now just throw in calls to a new helper function like
> DestroyAbsoluteFrames(). If the order really doesn't matter, then I suggest
> changing the order in a separate patch. That will reduce risk.

OK, I'll do that for now.

> > > Is this needed, given you've added the call to
> > > BuildDisplayListForStackingContext/Child?
> > 
> > Yeah, I remember some reftest somewhere failing without this, which caused me
> > to add it back...
> 
> I would like to understand why it's needed. Maybe we're fixing that failure the
> wrong way.

Without this for nsBlockFrame, the following tests fail (possibly among others):

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/243519-9f.html?force=1
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/372037-1.html?force=1
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/387876-2.html?force=1

Basically they all have an abs-pos element which is the child of a block frame.

I think the only frame type which doesn't need this in ViewportFrame, which calls BuildDisplayListForStatckingChild unconditionally.  Do you agree?

> > > @@ +255,2 @@
> > >  #ifdef DEBUG
> > > +  if (IsAbsoluteContainer()) {
> > > 
> > > I think the case when this is not an absolute container is a page in print
> > > preview, right? You might want to mention that in a comment.
> > 
> > Hmm, no, it should also happen if we don't have any fixed-pos elements, right? 
> 
> Whether a ViewportFrame is an absolute container should not depend on whether
> it has any fixed-pos children.

Hmm, but this is not how the code currently behaves.  We mark the viewport frame as an absolute container in the code in question in the first paragraph of my comment.
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-03 21:13:22 PDT
> I think the only frame type which doesn't need this in ViewportFrame, which
> calls BuildDisplayListForStatckingChild unconditionally.  Do you agree?

Aha! The problem is in your change to BuildDisplayListForChild:
+  // Mark the display list items for absolutely positioned children
+  MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);
You actually should be calling MarkAbsoluteFramesForDisplayList on the *child*, not on the parent.

Basically, the only two callers of nsIFrame::BuildDisplayList should be BuildDisplayListForChild (which calls it on the child) and BuildDisplayListForStackingContext (which calls it on 'this'). So for something like this where we need to do something for every BuildDisplayList, you should be able to do it in just those two callers.

> Hmm, but this is not how the code currently behaves.  We mark the viewport
> frame as an absolute container in the code in question in the first paragraph
> of my comment.

OK, well, that's wrong. The viewport frame should probably be marked as an absolute container when we assign mFixedContainingBlock. Either that, or ViewportFrame::Init does it.
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-03 21:18:21 PDT
Basically I think it's a good idea to have the invariant that if a frame is pushed as some kind of absolute container during frame construction, then it has been set up as an absolute container.

Setting up the absolute container lazily might be slightly more efficient, but I think it would be more confusing. Certainly we have to be consistent and either always do it lazily or never do it lazily.
Comment 58 :Ehsan Akhgari 2011-05-04 16:06:35 PDT
(In reply to comment #56)
> > I think the only frame type which doesn't need this in ViewportFrame, which
> > calls BuildDisplayListForStatckingChild unconditionally.  Do you agree?
> 
> Aha! The problem is in your change to BuildDisplayListForChild:
> +  // Mark the display list items for absolutely positioned children
> +  MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);
> You actually should be calling MarkAbsoluteFramesForDisplayList on the *child*,
> not on the parent.
> 
> Basically, the only two callers of nsIFrame::BuildDisplayList should be
> BuildDisplayListForChild (which calls it on the child) and
> BuildDisplayListForStackingContext (which calls it on 'this'). So for something
> like this where we need to do something for every BuildDisplayList, you should
> be able to do it in just those two callers.

It all makes sense now, and I have a patch which does the above!
Comment 59 :Ehsan Akhgari 2011-05-04 16:58:37 PDT
Created attachment 530198 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

(In reply to comment #57)
> Basically I think it's a good idea to have the invariant that if a frame is
> pushed as some kind of absolute container during frame construction, then it
> has been set up as an absolute container.
> 
> Setting up the absolute container lazily might be slightly more efficient, but
> I think it would be more confusing. Certainly we have to be consistent and
> either always do it lazily or never do it lazily.

OK, makes sense.  Done.
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-04 19:32:32 PDT
Comment on attachment 530198 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Review of attachment 530198 [details] [diff] [review]:

::: layout/generic/nsBlockFrame.cpp
@@ +6239,5 @@
     }
   }
 
   aBuilder->MarkFramesForDisplayList(this, mFloats, aDirtyRect);
+  MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);

We should not need this line.

::: layout/generic/nsFrame.cpp
@@ +1682,5 @@
     pseudoStackingContext = PR_TRUE;
+  }
+
+  // Mark the display list items for absolutely positioned children
+  aChild->MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);

I think you need to pass 'dirty' instead of aDirtyRect, since the rect needs to be relative to the child.

::: layout/generic/nsIFrame.h
@@ +2717,5 @@
   virtual nsIAtom* GetAbsoluteListName() const { return nsGkAtoms::absoluteList; }
 
+  /**
+   * This should be called by BuildDisplayList implementations to support adding
+   * the frames for absolute containing blocks to the display list builder.

Fix comment. BuildDisplayList implementations should not need to call this.
Comment 61 :Ehsan Akhgari 2011-05-05 09:07:32 PDT
(In reply to comment #60)
> ::: layout/generic/nsBlockFrame.cpp
> @@ +6239,5 @@
>      }
>    }
> 
>    aBuilder->MarkFramesForDisplayList(this, mFloats, aDirtyRect);
> +  MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);
> 
> We should not need this line.
> 
> ::: layout/generic/nsFrame.cpp
> @@ +1682,5 @@
>      pseudoStackingContext = PR_TRUE;
> +  }
> +
> +  // Mark the display list items for absolutely positioned children
> +  aChild->MarkAbsoluteFramesForDisplayList(aBuilder, aDirtyRect);
> 
> I think you need to pass 'dirty' instead of aDirtyRect, since the rect needs to
> be relative to the child.

Ah, of course.  I missed these yesterday.  I fixed them, and there are now a bunch of reftests failures that I need to investigate again.  :(
Comment 62 :Ehsan Akhgari 2011-05-05 15:39:41 PDT
Created attachment 530448 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

OK, fixed the code and except for fixing the comment, just made MarkAbsoluteFramesForDisplayList private (because nobody else needs to call it!)
Comment 63 :Ehsan Akhgari 2011-05-05 15:54:29 PDT
Created attachment 530450 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Actually make MarkAbsoluteFramesForDisplayList private!
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-05 15:56:05 PDT
Comment on attachment 530450 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Review of attachment 530450 [details] [diff] [review]:
Comment 65 :Ehsan Akhgari 2011-05-06 11:21:26 PDT
Landed the first two patches in the bug:

http://hg.mozilla.org/mozilla-central/rev/b5c0b85194d6
http://hg.mozilla.org/mozilla-central/rev/14fe8a6cfd45

The rest of the work will follow.
Comment 66 :Ehsan Akhgari 2011-05-06 15:37:58 PDT
Created attachment 530762 [details] [diff] [review]
[WIP] Part 3: Make tables absolute positioning containers

This patch does most of what is needed to make tables abs-pos containing blocks.  It has some bugs though.  From the tests that I've run so far, we assert on <http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/323493-1.html?force=1> several times with a stack like this:

###!!! ASSERTION: containing block height must be constrained: 'containingBlockHeight != NS_AUTOHEIGHT', file ../../../layout/generic/nsHTMLReflowState.cpp, line 1135

#0	0x10002137f in NS_DebugBreak_P at nsDebugImpl.cpp:274
#1	0x10044b10b in nsHTMLReflowState::InitAbsoluteConstraints at nsHTMLReflowState.cpp:1134
#2	0x10044ccf7 in nsHTMLReflowState::InitConstraints at nsHTMLReflowState.cpp:1845
#3	0x10044d1e4 in nsHTMLReflowState::Init at nsHTMLReflowState.cpp:282
#4	0x1005cf43c in nsTableOuterFrame::InitChildReflowState at nsTableOuterFrame.cpp:451
#5	0x1005cf67c in nsTableOuterFrame::OuterBeginReflowChild at nsTableOuterFrame.cpp:920
#6	0x1005cf9e5 in nsTableOuterFrame::Reflow at nsTableOuterFrame.cpp:1021
#7	0x1003e351e in nsAbsoluteContainingBlock::ReflowAbsoluteFrame at nsAbsoluteContainingBlock.cpp:444
#8	0x1003e4108 in nsAbsoluteContainingBlock::Reflow at nsAbsoluteContainingBlock.cpp:158
#9	0x100416ffa in nsFrame::ReflowAbsoluteFrames at nsFrame.cpp:3681
#10	0x1005cffe9 in nsTableOuterFrame::Reflow at nsTableOuterFrame.cpp:1129
#11	0x1003e351e in nsAbsoluteContainingBlock::ReflowAbsoluteFrame at nsAbsoluteContainingBlock.cpp:444
#12	0x1003e4108 in nsAbsoluteContainingBlock::Reflow at nsAbsoluteContainingBlock.cpp:158
#13	0x100416ffa in nsFrame::ReflowAbsoluteFrames at nsFrame.cpp:3681
#14	0x100425400 in nsFrame::FinishReflowWithAbsoluteFrames at nsFrame.cpp:3642
#15	0x10044761d in nsCanvasFrame::Reflow at nsCanvasFrame.cpp:557
#16	0x100409c5c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:959
#17	0x10043b776 in nsHTMLScrollFrame::ReflowScrolledFrame at nsGfxScrollFrame.cpp:546
#18	0x10043c064 in nsHTMLScrollFrame::ReflowContents at nsGfxScrollFrame.cpp:638
#19	0x10043f66f in nsHTMLScrollFrame::Reflow at nsGfxScrollFrame.cpp:879
#20	0x100409c5c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:959
#21	0x1004bc78a in ViewportFrame::Reflow at nsViewportFrame.cpp:225

I think this is because we are either reflowing the absolute frames at the wrong time (since I'm reflowing them after the overflow dimensions is calculated -- but reflowing them before this breaks the rendering completely), or I need to change something so that the ascent member of the reflow metrics isn't NS_AUTOHEIGHT (but I have no idea how to do that yet).
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-09 04:51:31 PDT
Your reflow change looks right to me.

We shouldn't be calling InitAbsoluteConstraints for an nsTableFrame, which seems to be what's happening here. It happens on trunk too, but seems to be harmless on trunk for some reason. I think nsHTMLReflowState::InitFrameType should not return NS_CSS_FRAME_TYPE_ABSOLUTE for an nsTableFrame, I think where we test frame->GetType() == nsGkAtoms::tableFrame we should just return NS_CSS_FRAME_TYPE_UNKNOWN if so. nsHTMLReflowState::InitAbsoluteConstraints can assert frame->GetType() != nsGkAtoms::tableFrame instead of checking that.
Comment 68 :Ehsan Akhgari 2011-05-09 11:00:21 PDT
This worked perfectly, thanks!

Now, there's this reftest: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503364-1a.html?force=1 on which I'm getting an incorrect rendering.  Here's the resulting frame tree: http://pastebin.mozilla.org/1221780

Note how the frame for the div ends up under the absolute list of the viewport frame, except for the absolute list of the table frame.  I'm trying to figure out why this happens...
Comment 69 :Ehsan Akhgari 2011-05-09 11:47:48 PDT
It seems to me that because the parent table doesn't have the NODE_DESCENDANTS_NEED_FRAMES flag set, we don't encounter it while creating the frame for the div, so the canvas frame ends up being the absolute container (because it was the last thing which called PushAbsoluteContainingBlock), which makes the reftest fail.  I'm not sure how dynamic changes like this are supposed to be handled.  Should we make sure that we reframe the table too?
Comment 70 :Ehsan Akhgari 2011-05-09 13:49:51 PDT
I think I figured this out.  The code in nsCSSFrameConstructor::GetAbsoluteContainingBlock actively ignored table related frames, and I fixed that.
Comment 71 :Ehsan Akhgari 2011-05-09 17:41:08 PDT
Created attachment 531218 [details] [diff] [review]
[WIP] Part 3: Make tables absolute positioning containers

This is nearly finished.  The only remaining problem is the reftest failure with <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/371561-1.html?force=1>.  In this page, the table is rendered too wide (NS_MAXSIZE), and lots of warnings like this are printed to the console:

WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aContainingBlockWidth != NS_UNCONSTRAINEDSIZE', file ../../../layout/base/nsLayoutUtils.cpp, line 2424
WARNING: cell content 0x10643d7b8 has large width 1063004416 

I experimented with a fix like this one: http://pastebin.mozilla.org/1221886 but this patch breaks parts of this test <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/427129-table.html?force=1> (the tables with a percent width).  I've run out of ideas on how to fix this...
Comment 72 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-09 20:34:45 PDT
Analyzing the callstack from that first warning would be useful. Assuming nsTableFrame's reflow state is firing the warning, its available width should have been set in nsTableOuterFrame::OuterBeginReflowChild, to aAvailWidth, so why isn't that set to something reasonable?
Comment 73 :Ehsan Akhgari 2011-05-10 13:29:05 PDT
This is a call stack:

#0	0x100376240 in nsLayoutUtils::ComputeWidthValue at nsLayoutUtils.cpp:2425
#1	0x10044df89 in nsCSSOffsetState::ComputeWidthValue at nsHTMLReflowState.cpp:189
#2	0x10044a688 in nsCSSOffsetState::ComputeWidthValue at nsHTMLReflowState.cpp:210
#3	0x10044a6d6 in nsHTMLReflowState::ComputeMinMaxValues at nsHTMLReflowState.cpp:2284
#4	0x10044c94a in nsHTMLReflowState::InitConstraints at nsHTMLReflowState.cpp:1786
#5	0x10044d194 in nsHTMLReflowState::Init at nsHTMLReflowState.cpp:282
#6	0x1005cf3ec in nsTableOuterFrame::InitChildReflowState at nsTableOuterFrame.cpp:451
#7	0x1005cf62c in nsTableOuterFrame::OuterBeginReflowChild at nsTableOuterFrame.cpp:920
#8	0x1005cf995 in nsTableOuterFrame::Reflow at nsTableOuterFrame.cpp:1021
#9	0x1003e34ce in nsAbsoluteContainingBlock::ReflowAbsoluteFrame at nsAbsoluteContainingBlock.cpp:444
#10	0x1003e40b8 in nsAbsoluteContainingBlock::Reflow at nsAbsoluteContainingBlock.cpp:158
#11	0x100416faa in nsFrame::ReflowAbsoluteFrames at nsFrame.cpp:3681
#12	0x10045c8be in nsInlineFrame::Reflow at nsInlineFrame.cpp:417
#13	0x1004649a7 in nsLineLayout::ReflowFrame at nsLineLayout.cpp:852
#14	0x1003eddf1 in nsBlockFrame::ReflowInlineFrame at nsBlockFrame.cpp:3826
#15	0x1003f43ea in nsBlockFrame::DoReflowInlineFrames at nsBlockFrame.cpp:3622
#16	0x1003f4f1e in nsBlockFrame::ReflowInlineFrames at nsBlockFrame.cpp:3481
#17	0x1003f6b6c in nsBlockFrame::ReflowLine at nsBlockFrame.cpp:2557
#18	0x1003f767b in nsBlockFrame::ReflowDirtyLines at nsBlockFrame.cpp:1995
#19	0x1003f9526 in nsBlockFrame::Reflow at nsBlockFrame.cpp:1075
#20	0x1003fda25 in nsBlockReflowContext::ReflowBlock at nsBlockReflowContext.cpp:296
#21	0x1003f5a1c in nsBlockFrame::ReflowBlockFrame at nsBlockFrame.cpp:3199
#22	0x1003f68f5 in nsBlockFrame::ReflowLine at nsBlockFrame.cpp:2501
#23	0x1003f767b in nsBlockFrame::ReflowDirtyLines at nsBlockFrame.cpp:1995
#24	0x1003f9526 in nsBlockFrame::Reflow at nsBlockFrame.cpp:1075
#25	0x100409c0c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:959
#26	0x100446fca in nsCanvasFrame::Reflow at nsCanvasFrame.cpp:454
#27	0x100409c0c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:959
#28	0x10043b726 in nsHTMLScrollFrame::ReflowScrolledFrame at nsGfxScrollFrame.cpp:546
#29	0x10043c014 in nsHTMLScrollFrame::ReflowContents at nsGfxScrollFrame.cpp:638
#30	0x10043f61f in nsHTMLScrollFrame::Reflow at nsGfxScrollFrame.cpp:879
#31	0x100409c0c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:959
#32	0x1004bc73a in ViewportFrame::Reflow at nsViewportFrame.cpp:225
#33	0x1003a022f in PresShell::DoReflow at nsPresShell.cpp:7735

The problem is that OuterBeginReflowChild does not pass the available width to InitChildReflowState, and the reflow state ends up getting initialized with avail width being set to -1, which triggers this code: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1703>.  ComputeContainingBlockRectangle relies on mFrameType being ABSOLUTE <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1573>, which causes the incorrect available with to be calculated here.

The obvious patch <http://pastebin.mozilla.org/1222183> fixes this, but I have no idea if that's acceptable...
Comment 74 :Ehsan Akhgari 2011-05-10 17:44:34 PDT
So, this patch <http://pastebin.mozilla.org/1222364> makes that reftest pass, but it also breaks a number of table layout reftests, such as <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/370525-1-notref.html>, which is now rendered identical to <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/370525-1.html>.
Comment 75 :Ehsan Akhgari 2011-05-10 17:54:48 PDT
I think this happens because nsTableOuterFrame::IsContainingBlock now returns true, which changes the way nsHTMLReflowState::InitResizeFlags behaves (or maybe because of the change to mCBReflowState)...
Comment 76 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 20:41:17 PDT
Yeah, I think IsContainingBlock is bogus. In CSS, "containing block" is a binary relation, not a unary relation. So I think we should replace nsIFrame::IsContainingBlock with nsIFrame::GetContainingBlock() which returns the containing block for the given frame.

I don't know if that's stricly necessary to do here, but it will probably help. There aren't many users of IsContainingBlock, and some of them are just implementing GetContainingBlock() themselves.
Comment 77 :Ehsan Akhgari 2011-05-11 13:31:50 PDT
I tried to do this, and I _think_ I implemented the CSS algorithm correctly <http://pastebin.mozilla.org/1222989>, but this patch doesn't fix the reftest in comment 71, and it _also_ breaks the reftest in comment 74...
Comment 78 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-11 17:09:13 PDT
Your GetContainingBlock implementation looks right but it could be improved:

Drop GetNearestBlockContainer and just use nsLayoutUtils::FindNearestBlockAncestor.

In GetContainingBlock, don't bother checking for viewportFrame/pageContentFrame. For the STATIC case, calling FindNearestBlockAncestor(this) will return null if this has no ancestor block.

For ABSOLUTE and FIXED, you can just return GetParent(). An absolutely (for fixed) positioned frame's containing block is always its parent.
Comment 79 Boris Zbarsky [:bz] 2011-05-11 18:24:11 PDT
Shouldn't GetAdditionalChildListName only return the absolute list if the frame is an absolute container?
Comment 80 :Ehsan Akhgari 2011-05-11 20:51:37 PDT
(In reply to comment #79)
> Shouldn't GetAdditionalChildListName only return the absolute list if the
> frame is an absolute container?

No, why would it?  What prevents absolute containers to have other child list types?
Comment 81 Boris Zbarsky [:bz] 2011-05-11 21:03:05 PDT
Oh, gah.  We interpret a null as "no more lists", right.  That sucks.  :(
Comment 82 :Ehsan Akhgari 2011-05-11 21:08:32 PDT
(In reply to comment #78)
> Drop GetNearestBlockContainer and just use
> nsLayoutUtils::FindNearestBlockAncestor.

It turns out that this is not really the right thing to do, since these two functions are not equivalent.

For example, nsRootBoxFrame is not an nsBlockFrame, but it's display is set to block.  This leads to a startup crash because we would incorrectly set the reflow state's mCBReflowState to null.
Comment 83 :Ehsan Akhgari 2011-05-11 21:09:32 PDT
(In reply to comment #81)
> Oh, gah.  We interpret a null as "no more lists", right.  That sucks.  :(

Indeed!  See bug 653649.  ;-)
Comment 84 :Ehsan Akhgari 2011-05-11 21:18:04 PDT
Created attachment 531848 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock

This is what I have so far.  It doesn't crash on startup, but it probably has lots of other bugs.  I'm going to sleep now, and will continue to work on this tomorrow.
Comment 85 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-11 22:55:33 PDT
(In reply to comment #82)
> (In reply to comment #78)
> > Drop GetNearestBlockContainer and just use
> > nsLayoutUtils::FindNearestBlockAncestor.
> 
> It turns out that this is not really the right thing to do, since these two
> functions are not equivalent.
> 
> For example, nsRootBoxFrame is not an nsBlockFrame, but it's display is set
> to block.  This leads to a startup crash because we would incorrectly set
> the reflow state's mCBReflowState to null.

Hmm yeah.

Maybe the thing to do for the static/relative case is what nsCSSRendering does near the comment "// Find the containing block frame": use the parent, unless the frame is eLineParticipant, in which case use the nearest ancestor that isn't eLineParticipant.
Comment 86 :Ehsan Akhgari 2011-05-16 09:02:04 PDT
(In reply to comment #85)
> (In reply to comment #82)
> > (In reply to comment #78)
> > > Drop GetNearestBlockContainer and just use
> > > nsLayoutUtils::FindNearestBlockAncestor.
> > 
> > It turns out that this is not really the right thing to do, since these two
> > functions are not equivalent.
> > 
> > For example, nsRootBoxFrame is not an nsBlockFrame, but it's display is set
> > to block.  This leads to a startup crash because we would incorrectly set
> > the reflow state's mCBReflowState to null.
> 
> Hmm yeah.
> 
> Maybe the thing to do for the static/relative case is what nsCSSRendering
> does near the comment "// Find the containing block frame": use the parent,
> unless the frame is eLineParticipant, in which case use the nearest ancestor
> that isn't eLineParticipant.

Do you mean that should be done in nsLayoutUtils::FindNearestBlockAncestor?
Comment 87 :Ehsan Akhgari 2011-05-16 11:07:26 PDT
We need to back this out from Firefox 6 Aurora if the work in this bug has not been finished yet.
Comment 88 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-16 15:56:12 PDT
(In reply to comment #86)
> Do you mean that should be done in nsLayoutUtils::FindNearestBlockAncestor?

No. Do it in a new function.
Comment 89 :Ehsan Akhgari 2011-05-17 08:49:04 PDT
Created attachment 532973 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock
Comment 90 :Ehsan Akhgari 2011-05-19 06:36:45 PDT
Created attachment 533627 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock

Only 1 reftest fails with this patch now: layout/reftests/bugs/371561-1.html.  I'm going to fix that and submit the final version for review.
Comment 91 :Ehsan Akhgari 2011-05-23 15:19:07 PDT
Reverting the mFrameType changes here <http://hg.mozilla.org/try/rev/19113c44bbca> fixes the reftest, but causes a bunch of assertions like this at the beginning of InitAbsoluteConstraints for reftests like this <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/490176-1.html?force=1>:

###!!! ASSERTION: containing block height must be constrained: 'containingBlockHeight != NS_AUTOHEIGHT', file /Users/ehsanakhgari/moz/mozilla-central/layout/generic/nsHTMLReflowState.cpp, line 1120

This is because when computing the containing block rectangle (with the stack below), the CB RS is now the parent table outer frame, not the canvas frame, and that table outer frame has an unconstrained height:

#0	0x10061b1dd in nsHTMLReflowState::ComputeContainingBlockRectangle at nsHTMLReflowState.cpp:1558
#1	0x100617f2b in nsHTMLReflowState::InitConstraints at nsHTMLReflowState.cpp:1688
#2	0x1006164b0 in nsHTMLReflowState::Init at nsHTMLReflowState.cpp:282
#3	0x1008187b2 in nsTableOuterFrame::InitChildReflowState at nsTableOuterFrame.cpp:451
#4	0x10081a38f in nsTableOuterFrame::OuterBeginReflowChild at nsTableOuterFrame.cpp:920
#5	0x10081a9d0 in nsTableOuterFrame::Reflow at nsTableOuterFrame.cpp:1021
#6	0x1005911b2 in nsAbsoluteContainingBlock::ReflowAbsoluteFrame at nsAbsoluteContainingBlock.cpp:444
#7	0x1005903a1 in nsAbsoluteContainingBlock::Reflow at nsAbsoluteContainingBlock.cpp:155
#8	0x1005d611c in nsFrame::ReflowAbsoluteFrames at nsFrame.cpp:3677
#9	0x1005d5f7d in nsFrame::FinishReflowWithAbsoluteFrames at nsFrame.cpp:3641
#10	0x10081b2fd in nsTableOuterFrame::Reflow at nsTableOuterFrame.cpp:1130
#11	0x1005911b2 in nsAbsoluteContainingBlock::ReflowAbsoluteFrame at nsAbsoluteContainingBlock.cpp:444
#12	0x1005903a1 in nsAbsoluteContainingBlock::Reflow at nsAbsoluteContainingBlock.cpp:155
#13	0x1005d611c in nsFrame::ReflowAbsoluteFrames at nsFrame.cpp:3677
#14	0x1005d5f7d in nsFrame::FinishReflowWithAbsoluteFrames at nsFrame.cpp:3641
#15	0x10061516b in nsCanvasFrame::Reflow at nsCanvasFrame.cpp:557
#16	0x1005c1b7d in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:958
#17	0x1005fdc72 in nsHTMLScrollFrame::ReflowScrolledFrame at nsGfxScrollFrame.cpp:545
#18	0x1005fe493 in nsHTMLScrollFrame::ReflowContents at nsGfxScrollFrame.cpp:639
#19	0x1005ff5cd in nsHTMLScrollFrame::Reflow at nsGfxScrollFrame.cpp:880
#20	0x1005c1b7d in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:958
#21	0x1006accf7 in ViewportFrame::Reflow at nsViewportFrame.cpp:224

This causes ComputeContainingBlockRectangle to set aContainingBlockHeight to NS_MAXSIZE, which causes InitAbsoluteConstraints called later in InitConstraints to assert.

Boris, do you have any idea how this can be solved?
Comment 92 Boris Zbarsky [:bz] 2011-05-23 18:42:44 PDT
Why are we calling InitAbsoluteConstraints for the inner table?
Comment 93 :Ehsan Akhgari 2011-05-23 21:25:24 PDT
(In reply to comment #92)
> Why are we calling InitAbsoluteConstraints for the inner table?

Because the table frame's mFrameType is NS_CSS_FRAME_TYPE_ABSOLUTE.  <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1842>
Comment 94 Boris Zbarsky [:bz] 2011-05-23 21:34:10 PDT
Wait.  I thought we were going to leave mFrameType as BLOCK for the inner table but take out the hackery in InitCBContainingBlock so the containing block reflow state for the inner table is the outer table....
Comment 95 :Ehsan Akhgari 2011-05-24 08:10:33 PDT
(In reply to comment #94)
> Wait.  I thought we were going to leave mFrameType as BLOCK for the inner
> table but take out the hackery in InitCBContainingBlock so the containing
> block reflow state for the inner table is the outer table....

Hmm, IIRC what we discussed last week was trying to revert the changes I had made to InitFrameType, and also try removing the InitCBReflowState hack to make the CB reflow state for inner table frames the outer table on top of that.

Only doing the first thing seems to help things a lot (it fixes the rendering problems) except for these assertions: <http://tbpl.mozilla.org/?tree=Try&rev=19113c44bbca> (effectively this patch on top of my other ones: <http://hg.mozilla.org/try/rev/19113c44bbca>).  Note that with this patches, *both* the inner and outer table frames are NS_CSS_FRAME_TYPE_ABSOLUTE, and the inner table frame's CB reflow state points to the outer table frame's CB.

Removing the InitCBReflowState hackery for inner tables breaks rendering <http://tbpl.mozilla.org/?tree=Try&rev=ce676985fd3b> (effectively this patch on top of my other ones: <http://hg.mozilla.org/try/rev/ce676985fd3b>).  Note that with this patch, *both* the inner and outer table frames are NS_CSS_FRAME_TYPE_ABSOLUTE, and the inner table frame's CB reflow state points to the outer table frame.

Now, making the mFrameType for the inner table frame BLOCK will fix this assertion that I'm struggling with now, *but* it will regress the reftest in comment 91.  I'm not sure what you're suggesting...
Comment 96 Boris Zbarsky [:bz] 2011-05-24 08:40:37 PDT
I thought what I suggested was to leave your InitFrameType changes but take out the InitCBReflowState hack.  The comment 91 reftest was failing for type == BLOCK because the cb reflow state was the inline, which had the wrong computed dimensions, no?
Comment 97 JP Rosevear [:jpr] 2011-05-24 14:18:23 PDT
Is this completely landed or does it need a backout?
Comment 98 :Ehsan Akhgari 2011-05-24 17:28:30 PDT
(In reply to comment #96)
> I thought what I suggested was to leave your InitFrameType changes but take
> out the InitCBReflowState hack.  The comment 91 reftest was failing for type
> == BLOCK because the cb reflow state was the inline, which had the wrong
> computed dimensions, no?

Leaving the InitFrameType changes, and removing the InitCBReflowState hacks for dynamically positioned table frames seems to fix everything!  Bug I wouldn't believe it until I get try results.  ;-)
Comment 99 :Ehsan Akhgari 2011-05-24 17:33:03 PDT
(In reply to comment #97)
> Is this completely landed or does it need a backout?

It does need a backout, I will attach a patch soon.
Comment 100 :Ehsan Akhgari 2011-05-24 17:37:52 PDT
Created attachment 534949 [details] [diff] [review]
Aurora backout
Comment 101 :Ehsan Akhgari 2011-05-24 17:39:46 PDT
Created attachment 534950 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock
Comment 102 :Ehsan Akhgari 2011-05-25 13:17:24 PDT
Comment on attachment 534950 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock

roc, do you want to review this too?
Comment 103 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-25 14:59:18 PDT
Comment on attachment 534950 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock

bz can do this.
Comment 104 :Ehsan Akhgari 2011-05-25 18:24:48 PDT
So, I was looking at why the test case in bug 63895 still doesn't render correctly, and I figured out that it's because the outer table frame (which is what we position abs-pos kids relative to) is getting a width equal to that of its containing block (the body element in this case).

After a while of poking through the specs, and talking to dbaron in IRC, I came to the conclusion that this is the right behavior, and it's Opera and WebKit which have got this wrong.

We can still make something like the rendering that the author of that test case intended work by making the row groups also an abs-pos container, so that the author can make the tbody element positioned, but that can happen in another bug.

So, the set of patches attached here, with the addition of the patch in bug 656130 complete the work of making tables abs-pos containers.  :-)
Comment 105 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-25 19:29:10 PDT
(In reply to comment #104)
> After a while of poking through the specs, and talking to dbaron in IRC, I
> came to the conclusion that this is the right behavior, and it's Opera and
> WebKit which have got this wrong.

I disagree. See discussion in bug 659828.
Comment 106 Asa Dotzler [:asa] 2011-06-01 11:33:54 PDT
Comment on attachment 534949 [details] [diff] [review]
Aurora backout

approved to back out from 5 Aurora
Comment 108 Boris Zbarsky [:bz] 2011-06-09 17:58:09 PDT
Comment on attachment 534950 [details] [diff] [review]
[WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock

>+++ b/layout/generic/nsFrame.cpp
>+PRBool
>+nsIFrame::IsBlockWrapper()
>+{
>+  if (nsLayoutUtils::GetAsBlock(this)) {

I don't think we need this check; just check the pseudo type directly.  Unless this is meant to be an optimization?  I'm not sure which is faster...

>+GetNearestBlockContainer(nsIFrame* frame)

Can you make this iterative instead of recursing?  Should be simple, and better to not recurse when we can avoid it.

>+nsIFrame::GetContainingBlock()

1) Would it make sense to test IsAbsolutelyPositioned() instead of using the switch?
2) Even if IsAbsolutelyPositioned(), we probably need to check for the out-of-flow bit before deciding the containing block is our parent.  I hate MathML's messing with positioning.  :(

>+++ b/layout/generic/nsHTMLReflowState.cpp
>+  // Absolutely positioned frames should always be kids of the frames that
>+  // determine their containing block, except for inner table frames.

I'm not sure I follow this comment, given the code below.  In particular, for positioned inner table frames you're setting mCBReflowState = parentReflowState!  I think this comment should just go away.

>+  if (parentReflowState->frame == frame->GetContainingBlock() ||
>       (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {

Isn't that second test redundant?  That is, can mFrameType be ABSOLUTE when parentReflowState->frame != frame->GetContainingBlock()?  Used to be that we needed the mFrameType test to handle the case when parentReflowState->frame was a rel pos inline, but with your changes that will test as frame->GetContainingBlock(), right?

>     // an absolutely positioned inner table needs to use the parent of
>+    // the outer table.

This comment doesn't seem to match reality.

>+    if (frame->GetType() == nsGkAtoms::tableFrame &&
>+        !parentReflowState->frame->GetStyleDisplay()->IsAbsolutelyPositioned()) {
>+      mCBReflowState = parentReflowState->mCBReflowState;

As I said on IRC, this doesn't quite make sense to me.  Why do we use the containing block of the _outer_ table for non-positioned inner tables only?  Shouldn't we just use the outer table as the inner table's containing block in all cases?

And yes, I realize that we had a whole comment about this situation that you deleted that claimed that we need to use the mCBReflowState of the outer table as the CBReflowState for the inner.  Maybe that's all that's going on here, but if so we should documet that.  And I'd love to understand why that's so....

>+  } else if (frame->GetType() == nsGkAtoms::tableColFrame ||
>+             frame->GetType() == nsGkAtoms::tableRowFrame ||
>+             frame->IsBlockWrapper()) {
>+    mCBReflowState = parentReflowState;

Why is this clause needed?  That is, if frame->IsBlockWrapper(), shouldn't we have fallen into the first clause of the |if|?  And similar for colframe and rowframe?

>+    // Make sure that we don't set mCBReflowState to null for edge cases
>+    // such as root frames.

Hmm.  What used to happen for those before?

>@@ -368,17 +365,18 @@ nsHTMLReflowState::InitResizeFlags(nsPre
>+  } else if (mCBReflowState &&
>+             !(nsLayoutUtils::GetAsBlock(frame) && !frame->IsBlockWrapper())) {

I _think_ this is OK, but I think that would be more readable as:

   } else if (mCBReflowState &&
              (!nsLayoutUtils::GetAsBlock(frame) || frame->IsBlockWrapper())) {

(less in the way of double-negatives).

>@@ -412,17 +410,18 @@ nsHTMLReflowState::InitResizeFlags(nsPre
>     // but only on containing blocks if this frame is not a suitable block
>-    dependsOnCBHeight |= !frame->IsContainingBlock();
>+    dependsOnCBHeight |= !(mCBReflowState &&
>+                           frame->GetContainingBlock() == mCBReflowState->frame);

This I'm pretty sure is wrong.  The test here needs to match the test in nsHTMLReflowState::CalcLineHeight(void), which I think is actually correct.

>+nsHTMLReflowState::GetContainingBlockFor(nsIFrame* aFrame)

This whole method seems redundant.  Indeed, if aFrame->GetStyleDisplay()->IsAbsolutelyPositioned(), then aFrame->GetContainingBlock() == aFrame->GetParent(), right?  So this method always returns aFrame->GetContainingBlock(), as far as I can tell.

If I'm right, then we should just make it return aFrame->GetContainingBlock() and file a bug to nix this method altogether.

>+++ b/layout/generic/nsIFrame.h
>+  PRBool IsBlockWrapper();

This can be a const method, right?

>+  nsIFrame* GetContainingBlock();

Likewise.

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+    if (nsLayoutUtils::GetAsBlock(mInnerFrame) && !mInnerFrame->IsBlockWrapper()) {

This pattern pops up a lot in this patch.  Can we just factor it out into a helper method on nsLayoutUtils or something?
Comment 109 :Ehsan Akhgari 2011-06-10 15:53:53 PDT
Created attachment 538623 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

(In reply to comment #108)
> Comment on attachment 534950 [details] [diff] [review] [review]
> [WIP] part 3.1: Replace IsContainingBlock with GetContainingBlock
> 
> >+++ b/layout/generic/nsFrame.cpp
> >+PRBool
> >+nsIFrame::IsBlockWrapper()
> >+{
> >+  if (nsLayoutUtils::GetAsBlock(this)) {
> 
> I don't think we need this check; just check the pseudo type directly. 
> Unless this is meant to be an optimization?  I'm not sure which is faster...

No, this wasn't meant to be an optimization (and if it were, it would've been a premature one!).  I'll fix this.

> >+GetNearestBlockContainer(nsIFrame* frame)
> 
> Can you make this iterative instead of recursing?  Should be simple, and
> better to not recurse when we can avoid it.

Sure.

> >+nsIFrame::GetContainingBlock()
> 
> 1) Would it make sense to test IsAbsolutelyPositioned() instead of using the
> switch?

I guess so.

> 2) Even if IsAbsolutelyPositioned(), we probably need to check for the
> out-of-flow bit before deciding the containing block is our parent.  I hate
> MathML's messing with positioning.  :(

I don't really know about how MathML messes with positioning, so I'm not really sure what you're suggesting here...

> >+++ b/layout/generic/nsHTMLReflowState.cpp
> >+  // Absolutely positioned frames should always be kids of the frames that
> >+  // determine their containing block, except for inner table frames.
> 
> I'm not sure I follow this comment, given the code below.  In particular,
> for positioned inner table frames you're setting mCBReflowState =
> parentReflowState!  I think this comment should just go away.

Agreed.  This was stupid.  :-)

> >+  if (parentReflowState->frame == frame->GetContainingBlock() ||
> >       (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
> 
> Isn't that second test redundant?  That is, can mFrameType be ABSOLUTE when
> parentReflowState->frame != frame->GetContainingBlock()?  Used to be that we
> needed the mFrameType test to handle the case when parentReflowState->frame
> was a rel pos inline, but with your changes that will test as
> frame->GetContainingBlock(), right?

You're right, I've removed it.

> >     // an absolutely positioned inner table needs to use the parent of
> >+    // the outer table.
> 
> This comment doesn't seem to match reality.

Right, fixed.

> >+    if (frame->GetType() == nsGkAtoms::tableFrame &&
> >+        !parentReflowState->frame->GetStyleDisplay()->IsAbsolutelyPositioned()) {
> >+      mCBReflowState = parentReflowState->mCBReflowState;
> 
> As I said on IRC, this doesn't quite make sense to me.  Why do we use the
> containing block of the _outer_ table for non-positioned inner tables only? 
> Shouldn't we just use the outer table as the inner table's containing block
> in all cases?

OK, so I tried really hard to remember why I've done this, to no avail.  So I removed it and ran the reftests, and not including this check breaks this test: <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/371561-1.html?force=1>.  In this test, we have an abs-pos table as a child of a positioned inline.  I guess in that case, the parent's CB is not suitable as the CB for the inner table frame.  Maybe we need to change this to check for the positioned inline case?  I really don't know what's a better way to deal with this situation.

> >+  } else if (frame->GetType() == nsGkAtoms::tableColFrame ||
> >+             frame->GetType() == nsGkAtoms::tableRowFrame ||
> >+             frame->IsBlockWrapper()) {
> >+    mCBReflowState = parentReflowState;
> 
> Why is this clause needed?  That is, if frame->IsBlockWrapper(), shouldn't
> we have fallen into the first clause of the |if|?  And similar for colframe
> and rowframe?

Hmm, right.  I had probably added this while I was in the middle of working on this, and forgot to take it out.

> >+    // Make sure that we don't set mCBReflowState to null for edge cases
> >+    // such as root frames.
> 
> Hmm.  What used to happen for those before?

Oops, s/root/canvas/.  nsCanvasFrame::IsContainingBlock returns true, so for canvas, we used to end up in the first if branch.  Basically, this preserves that behavior.

> >@@ -368,17 +365,18 @@ nsHTMLReflowState::InitResizeFlags(nsPre
> >+  } else if (mCBReflowState &&
> >+             !(nsLayoutUtils::GetAsBlock(frame) && !frame->IsBlockWrapper())) {
> 
> I _think_ this is OK, but I think that would be more readable as:
> 
>    } else if (mCBReflowState &&
>               (!nsLayoutUtils::GetAsBlock(frame) ||
> frame->IsBlockWrapper())) {
> 
> (less in the way of double-negatives).

Fixed.

> >@@ -412,17 +410,18 @@ nsHTMLReflowState::InitResizeFlags(nsPre
> >     // but only on containing blocks if this frame is not a suitable block
> >-    dependsOnCBHeight |= !frame->IsContainingBlock();
> >+    dependsOnCBHeight |= !(mCBReflowState &&
> >+                           frame->GetContainingBlock() == mCBReflowState->frame);
> 
> This I'm pretty sure is wrong.  The test here needs to match the test in
> nsHTMLReflowState::CalcLineHeight(void), which I think is actually correct.

Done.

> >+nsHTMLReflowState::GetContainingBlockFor(nsIFrame* aFrame)
> 
> This whole method seems redundant.  Indeed, if
> aFrame->GetStyleDisplay()->IsAbsolutelyPositioned(), then
> aFrame->GetContainingBlock() == aFrame->GetParent(), right?  So this method
> always returns aFrame->GetContainingBlock(), as far as I can tell.
> 
> If I'm right, then we should just make it return
> aFrame->GetContainingBlock() and file a bug to nix this method altogether.

Correct.  But not worth filing another bug, I just went ahead and did it.  :-)

> >+++ b/layout/generic/nsIFrame.h
> >+  PRBool IsBlockWrapper();
> 
> This can be a const method, right?
> 
> >+  nsIFrame* GetContainingBlock();
> 
> Likewise.

Done.

> >+++ b/layout/style/nsComputedDOMStyle.cpp
> >+    if (nsLayoutUtils::GetAsBlock(mInnerFrame) && !mInnerFrame->IsBlockWrapper()) {
> 
> This pattern pops up a lot in this patch.  Can we just factor it out into a
> helper method on nsLayoutUtils or something?

Done: nsLayoutUtils::IsNonWrapperBlock.
Comment 110 Boris Zbarsky [:bz] 2011-06-16 14:09:04 PDT
Comment on attachment 538623 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

> I don't really know about how MathML messes with positioning

It disallows positioning inside it, so the style can be NS_STYLE_POSITION_ABSOLUTE but the frame is still in-flow and behaves like a statically-positioned frame...  So I really think we need to test IsAbsolutelyPositioned() && out-of-flow.

>+++ b/layout/generic/nsHTMLReflowState.cpp
>+    // Inner table frames need to use the containing block of the outer
>+    // table frame, unless if they're absolutely positioned.

s/unless if/unless/

You can just check GetStyleDisplay() on frame, instead of checking it on parentReflowState->frame, right?

> I guess in that case, the parent's CB is not suitable as the CB for
> the inner table frame.

Well, sure.  But the question is why it _is_ for non-positioned cases.  I guess percentage heights or something?

We should revisit this code after bug 659828, perhaps.  In my ideal world the comment here would describe _why_ the containing block is what it is.  In my even more ideal world, GetContainingBlock would return the right thing on inner tables to start with!

Can we file a followup bug to sort this out?

> Oops, s/root/canvas/.  nsCanvasFrame::IsContainingBlock returns true,
> so for canvas, we used to end up in the first if branch.  Basically,
> this preserves that behavior.

But frame->GetContainingBlock() should be the canvas for kids of the canvas, no?  So shouldn't we still end up in the first if branch?  Or is this trying to deal with the case when |frame| is the canvas or something?  But what's parentReflowState->frame in that case?  I'd really like the comments to clearly explain which case we're dealing with, at least.

The rest looks fine.
Comment 111 :Ehsan Akhgari 2011-06-19 13:27:47 PDT
Created attachment 540345 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

OK, this should address all of the review comments.  I finally figured out that special casing non-abspos inner table frames is a mistake; special casing should happen in ComputeContainingBlockRectangle.  This fails the reftest we talked about on IRC.  This is the fix: <http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/file/b879492c5fc7/trial>, and it should be the only significant change in this version of the patch (aside from the reftests added).
Comment 112 :Ehsan Akhgari 2011-06-21 06:56:52 PDT
We need to back this out from Aurora if the rest of the pieces don't land by the next Aurora migration (which is pretty unlikely I think!)
Comment 113 Boris Zbarsky [:bz] 2011-06-21 13:47:34 PDT
Comment on attachment 540345 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

r=me with two nits that don't need another patch:

1)  Please file that followup bug about sorting out GetContainingBlock() behavior for inner tables.

2)  The new check added to ComputeContainingBlockRectangle should probably also check that frame->GetParent() is out of flow.
Comment 114 :Ehsan Akhgari 2011-06-22 17:51:02 PDT
Created attachment 541248 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

(In reply to comment #113)
> Comment on attachment 540345 [details] [diff] [review] [review]
> part 3.1: Replace IsContainingBlock with GetContainingBlock
> 
> r=me with two nits that don't need another patch:
> 
> 1)  Please file that followup bug about sorting out GetContainingBlock()
> behavior for inner tables.

Filed bug 666447.

> 2)  The new check added to ComputeContainingBlockRectangle should probably
> also check that frame->GetParent() is out of flow.

Done.

Also, added a new reftest for the case you asked me to test.  But there's one problem with it: the test asserts here:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1584

The assertion happens because aContainingBlockRS->mComputedBorderPadding.right is 60 (because of the right-border style, while aContainingBlockRS->frame->GetRect() is empty (because the inline frame's reflow is not finished yet.)  It seems like the comment there about nsInlineFrame::Reflow passing the CB dimensions is just lies, and the same assertions happen without my patches too.

Should I file a bug about that and annotate the reftest file?
Comment 115 Boris Zbarsky [:bz] 2011-06-22 18:40:00 PDT
The comment about nsInlineFrame::Reflow passing the CB dimensions is true for the kids of that inline frame; the _outer_ table frame gets the right CB size passed to it.  ;)

Please do file a bug and annotate the reftest, yes.
Comment 116 :Ehsan Akhgari 2011-06-23 11:37:49 PDT
Created attachment 541446 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

Filed bug 666606 for the assertion.
Comment 117 :Ehsan Akhgari 2011-06-24 15:38:46 PDT
Created attachment 541837 [details] [diff] [review]
part 3.1: Replace IsContainingBlock with GetContainingBlock

Switched the reftests to use the Ahem font for precise measurements, and skipped them because of bug 667079.
Comment 118 Jesse Ruderman 2011-06-26 13:18:34 PDT
>We need to back this out from Aurora if the rest of the pieces don't land by the >next Aurora migration (which is pretty unlikely I think!)

I'd prefer if you backed it out of mozilla-central. This divergence between what we're testing on mozilla-central and shipping in 5,6,7 is increasingly problematic. So is not being able to fuzz properly (see bug 656130).

Did this ever get backed out from Aurora-6? The bug is marked as "status-firefox6:fixed" :(
Comment 119 :Ehsan Akhgari 2011-06-28 15:04:19 PDT
(In reply to comment #118)
> >We need to back this out from Aurora if the rest of the pieces don't land by the >next Aurora migration (which is pretty unlikely I think!)
> 
> I'd prefer if you backed it out of mozilla-central. This divergence between
> what we're testing on mozilla-central and shipping in 5,6,7 is increasingly
> problematic. So is not being able to fuzz properly (see bug 656130).

OK, fair enough.  Backed out the patch from mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/a10dd1a539db>

> Did this ever get backed out from Aurora-6? The bug is marked as
> "status-firefox6:fixed" :(

It has been backed out (comment 107), which is why I set the status to fixed.  Is that the wrong value?
Comment 120 Mats Palmgren (:mats) 2011-07-26 11:01:21 PDT
*** Bug 674265 has been marked as a duplicate of this bug. ***
Comment 121 christian 2011-08-09 17:48:56 PDT
Probably more appropriate as unaffected, as the backout made Firefox 6 unaffected by this problem.
Comment 122 :Ehsan Akhgari 2011-09-22 15:34:56 PDT
Created attachment 561910 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

This part is rewritten on top of the new frame childlist APIs.
Comment 123 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-22 16:20:09 PDT
Comment on attachment 561910 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Review of attachment 561910 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableOuterFrame.cpp
@@ +220,5 @@
>        return mFrames;
>      case kCaptionList:
>        return mCaptionFrames;
> +    case kAbsoluteList:
> +      return nsHTMLContainerFrame::GetChildList(aListID);

Why don't we just make this the default case?

In fact, can't nsTableOuterFrame::GetChildList() just check for kCaptionList and pass all other cases to nsHTMLContainerFrame::GetChildList?
Comment 124 :Ehsan Akhgari 2011-09-22 19:26:29 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #123)
> Comment on attachment 561910 [details] [diff] [review]
> Part 2: Implement the absolute positioning support for all frames
> 
> Review of attachment 561910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/tables/nsTableOuterFrame.cpp
> @@ +220,5 @@
> >        return mFrames;
> >      case kCaptionList:
> >        return mCaptionFrames;
> > +    case kAbsoluteList:
> > +      return nsHTMLContainerFrame::GetChildList(aListID);
> 
> Why don't we just make this the default case?

The default case is returning an empty list right now.  I assumed that's for a good reason?

> In fact, can't nsTableOuterFrame::GetChildList() just check for kCaptionList
> and pass all other cases to nsHTMLContainerFrame::GetChildList?

If that modification to the default case is acceptable, then we can make this change.
Comment 125 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-22 19:37:14 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #124)
> The default case is returning an empty list right now.  I assumed that's for
> a good reason?

I can't think what that reason might be.
Comment 126 :Ehsan Akhgari 2011-09-22 19:57:31 PDT
Created attachment 561959 [details] [diff] [review]
Part 2: Implement the absolute positioning support for all frames

Addressed the review comment.
Comment 127 :Ehsan Akhgari 2011-09-26 15:37:19 PDT
Created attachment 562556 [details] [diff] [review]
Part 2.1: Adjust the assertion count on the test case for bug 348729

These two additional assertions happening here are the same as bug 548836, only now they happen twice for the placeholder frames of the abs-pos frame.
Comment 128 Mozilla RelEng Bot 2011-09-28 00:31:24 PDT
Try run for 3d1d60d61b12 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3d1d60d61b12
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-3d1d60d61b12
Comment 129 Mozilla RelEng Bot 2011-09-28 00:31:41 PDT
Try run for 5de952be2d5a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5de952be2d5a
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-5de952be2d5a
Comment 130 Mozilla RelEng Bot 2011-09-28 00:32:23 PDT
Try run for a0d0a3161bcf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a0d0a3161bcf
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-a0d0a3161bcf
Comment 131 Mozilla RelEng Bot 2011-09-28 00:41:00 PDT
Try run for ccf050fdea2f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ccf050fdea2f
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-ccf050fdea2f
Comment 132 Mozilla RelEng Bot 2011-09-28 00:41:07 PDT
Try run for 0266bcb23550 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0266bcb23550
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-0266bcb23550
Comment 133 Mozilla RelEng Bot 2011-09-28 00:41:29 PDT
Try run for 682e26fccd71 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=682e26fccd71
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-682e26fccd71
Comment 134 Mozilla RelEng Bot 2011-09-28 00:41:46 PDT
Try run for 9abe603e8e15 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9abe603e8e15
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-9abe603e8e15
Comment 135 Mozilla RelEng Bot 2011-09-28 00:42:08 PDT
Try run for 36ab7e228dd2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=36ab7e228dd2
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-36ab7e228dd2
Comment 136 Mozilla RelEng Bot 2011-09-28 11:50:56 PDT
Try run for 8533d2324679 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8533d2324679
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8533d2324679
Comment 137 :Ehsan Akhgari 2011-09-28 16:05:01 PDT
So, I'm facing one last reftest failure that I'm not sure how to debug.  This is the failure: https://tbpl.mozilla.org/?tree=Try&rev=8533d2324679.  The failure only happens on Windows 7 (and not Windows XP).  The failure is basically the #d1 div appearing below the #d2 div as a horizontal 1-pixel tall line at the bottom of #d2.

It happens with the first two parts of the patch applied.  The reftest in question <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/637597-1.html?force=1> is using transforms, but I'm not sure how they're implemented, and how they would interact with absolute positioning.

I tried reproducing the reftest locally.  When I run the entire reftest suite, I can reproduce it.  When I run part of it (such as all of the tests in layout/reftests/bugs), I can't reproduce.  I tried adding some debug code which would print the frame tree before and after the MozReftestInvalidate event, but with that code in place, the failure doesn't happen any more (and the frame trees look identical).

I'm not sure how to proceed with debugging this.  Do you guys have any ideas?
Comment 138 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-28 16:32:08 PDT
You mean #b1 and #b2 right?

I think you're misinterpreting the results. The difference is entirely in the rendering of the #b1 div, the #b2 div is white and nowhere near the #b1 div.

That is a little weird since clearly parts 1 and 2 should not affect behavior of this testcase.

Dumping the layer trees and comparing them might be helpful.
Comment 139 :Ehsan Akhgari 2011-09-28 17:38:04 PDT
Roc was right about me misinterpreting the results.  I'm too confused!

Timothy suggested that I should increase the timer in <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4153>.  I increased it to 500ms, and then I reran the reftest suite with MOZ_DUMP_PAINT_LIST, but now the failure doesn't happen any more.  Any other ideas?
Comment 140 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-28 18:48:05 PDT
Getting a layer tree seems like a good idea.

One question is whether you're actually regressing bug 637597. You might want to run the testcase(s) in that bug.

Breaking up patch 2 into as many independent patches as possible might help narrow down the problem.
Comment 141 :Ehsan Akhgari 2011-09-28 19:32:32 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #140)
> One question is whether you're actually regressing bug 637597. You might
> want to run the testcase(s) in that bug.

Interesting!  Seems like that bug was never quite fixed!  I loaded https://bug637597.bugzilla.mozilla.org/attachment.cgi?id=516245 on Mac.  On a trunk build, when the green box moves, its width is increased by 1 pixel on the right side.  With a patched build with my patches, the green box's width is increased in the same way, but its height shrinks up by 1 pixel on the down side too.

On Windows, I don't see this on either a patched or non-patched build.

Is it possible that bug 637597 is not completely fixed, and my patch is triggering it on Windows 7 somehow?
Comment 142 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-28 19:49:06 PDT
Yes it is.

The thing is, I don't know why your patch would change anything here. It's supposed to be just a refactoring. If you can reproduce the different on Mac at will, maybe you can track down what's happening differently?
Comment 143 :Ehsan Akhgari 2011-09-28 20:06:33 PDT
OK, I'll see if I can track this down on Mac.

Another interesting data point is that back when I was working on this bug in April and May, I used to push this to the try server all the time, and I never got that reftest failure even though the test was living in the tree back then.
Comment 144 :Ehsan Akhgari 2011-09-28 20:16:42 PDT
Created attachment 563282 [details]
Trimmed log of when the bug happens with the testcase of bug 637597

This is the log representing when the test case is loaded on Mac with a patched build, until the move happens and the bug shows up.
Comment 145 :Ehsan Akhgari 2011-09-28 20:18:24 PDT
Created attachment 563283 [details]
Trimmed log of when the bug happens with the testcase of bug 637597 (only the layer tree)

This is the same log, only the layer tree parts, for easier viewing.
Comment 146 :Ehsan Akhgari 2011-09-28 21:14:48 PDT
Created attachment 563292 [details]
Trimmed log of when the bug happens with the testcase of bug 637597 (non-patched build)

I noticed another weird thing.  How many pixels the green box changes in dimensions seems to depend on the Window size.  I was testing the trunk build in a maximized window and my patched build in a non-maximized window.  When I built a non-patched version and tried this, it shows the same behavior as trunk in a maximized window, and it also shows the same behavior as my patched build in the non-maximized window.

I'm not sure how to make sense of this log, or if the current direction in which I'm headed is going to help at all.
Comment 147 :Ehsan Akhgari 2011-09-28 21:22:05 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #143)
> Another interesting data point is that back when I was working on this bug
> in April and May, I used to push this to the try server all the time, and I
> never got that reftest failure even though the test was living in the tree
> back then.

Hmm, I was actually wrong here.  Bug 637597 was marked as fixed by bug 637852, which was landed on 2011-06-22.  So this test probably did not exist in the tree back then.
Comment 148 :Ehsan Akhgari 2011-09-28 21:30:14 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #147)
> (In reply to Ehsan Akhgari [:ehsan] from comment #143)
> > Another interesting data point is that back when I was working on this bug
> > in April and May, I used to push this to the try server all the time, and I
> > never got that reftest failure even though the test was living in the tree
> > back then.
> 
> Hmm, I was actually wrong here.  Bug 637597 was marked as fixed by bug
> 637852, which was landed on 2011-06-22.  So this test probably did not exist
> in the tree back then.

I just downloaded the nightly from 2011-06-23, and I can confirm that the same bug exists in that nightly too!
Comment 149 Mozilla RelEng Bot 2011-09-29 00:51:15 PDT
Try run for af10abe5e706 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=af10abe5e706
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-af10abe5e706
Comment 150 Mozilla RelEng Bot 2011-09-29 01:03:07 PDT
Try run for 3df652952e6c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3df652952e6c
Results (out of 3 total builds):
    success: 1
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-3df652952e6c
Comment 151 Timothy Nikkel (:tnikkel) 2011-09-29 10:22:35 PDT
It's pretty easy for me to reproduce the problem in bug 637597 on a plain nightly on Linux (with basic layers). Seems like Ehsan is just getting unlucky here.
Comment 152 :Ehsan Akhgari 2011-09-29 10:32:24 PDT
Created attachment 563455 [details] [diff] [review]
Part 4: Mark the reftest for bug 637597 as random

I think the best option for us at this point is to mark that reftest as random (I've already reopened that bug), and add it back to our reftest arsenal when we actually fix that bug.
Comment 154 Mozilla RelEng Bot 2011-09-29 15:11:27 PDT
Try run for f17b29c28af5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f17b29c28af5
Results (out of 230 total builds):
    exception: 1
    success: 216
    warnings: 12
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f17b29c28af5
Comment 155 Eric Shepherd [:sheppy] 2011-12-19 13:49:28 PST
Change documented:

https://developer.mozilla.org/en/CSS/position#Gecko_notes

And linked to from Firefox 10 for developers.
Comment 156 T. Brains 2012-01-12 17:01:32 PST
The positioning issue seems to be fixed, but offsetParent on the element still returns the outer block element, rather than the table element. Isn't this wrong behavior?
Comment 157 Boris Zbarsky [:bz] 2012-01-12 17:06:43 PST
Yep.  Please file a separate bug?  The behavior of offsetParent is actually completely independent of positioning behavior, and it has a special-case for tables that might need removing now...
Comment 158 T. Brains 2012-01-12 17:42:16 PST
Added under bug 717819.

Note You need to log in before you can comment on or make changes to this bug.