DESCRIPTION: Relative positioning of table cells doesn't work. It should be possible to relatively position table cells. In my opinion, relatively positioned cells should take their borders with them in the separated borders model, but not in the collapsed borders model. In other words, I think the borders should be attached to their respective elements in the separated borders model, and should be attached to the table in the collapsing borders model. (Keep that in mind when doing work on the collapsing borders model.) STEPS TO REPRODUCE: * load attached testcase ACTUAL RESULTS: * bold cell is in the normal position EXPECTED RESULTS: * it (the word "Data", and probably nothing else) should be displaced 30px to the left and 15px down. DOES NOT WORK CORRECTLY ON: * Linux, mozilla, 2000-04-07-08-M15

Believe it or not, this works in 4.x...

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.

As per meeting with ChrisD today, taking QA.

*** Bug 139982 has been marked as a duplicate of this bug. ***

Well I understand this bug is neither critical nor blocking, but fixing it would allow us to use CSS tables for HTML documents more easily - we could create multicolumn layouts with all columns automatically the same height as the highest column, without the restriction that the order that cells are displayed must be the same order in which HTML elements appear in the document flow.

Reconfirmed using FizzillaCFM/2002071208. Setting All/All.

Some links for your consideration: HTML: http://html.conclase.net/pruebas/3cols-en.html Opera 6.01 (expected behaviour): http://html.conclase.net/pruebas/3cols-o601.gif IE6.0: http://html.conclase.net/pruebas/3cols-ie6.gif NS6.2.2/Moz: http://html.conclase.net/pruebas/3cols-ns622.gif

mass reassign to default owner

It may be worth noting that Internet Explorer 6.0 renders this case correctly.

"Table cells may be relatively and absolutely positioned, but this is not recommended: positioning and floating remove a box from the flow, affecting table alignment." http://www.w3.org/TR/CSS2/tables.html#q7 "Positioning and floating of table cells can cause them not to be table cells anymore, according to the rules in section 9.7." http://www.w3.org/TR/2006/WD-CSS21-20060411/tables.html#q7 "The effect of 'position:relative' on table-row-group, table-header-group, table-footer-group, table-row, table-column-group, table-column, table-cell, and table-caption elements is undefined." http://www.w3.org/TR/2006/WD-CSS21-20060411/visuren.html#choose-position

*** Bug 360603 has been marked as a duplicate of this bug. ***

If the required behavior is undefined (according to the W3C), shouldn't this bug be marked as WONTFIX? I still think that position:relative should be applied to table cells at least to the point of being the reference for absolute-positioned content, but that's not what the W3C thinks. Even more, you could check if there's a consistent behavior across the rest of the major browser vendors and implement it ("undefined" being "I leave it to you, browser vendors").

This shouldn't be wontfix. We should make it work. Probably the containing block should be established by the same region in which the background is drawn. The question is what happens to borders -- I think we should treat borders in the separated border model as attached to the table cell or table (depending on which they're on), and borders in the collapsed border model as all attached to the table ... and then borders should move if they're attached to the relatively positioned element or one of its descendants. There are also some complications in background drawing, though that might be more a function of our code than the spec. I think backgrounds on the relatively positioned element and its descendants should move; I'm not sure whether that's what we'd end up with in our current code.

I have hit this problem many, many times over the years and would like to argue that this is really a bug. I have done so here, after a short example: http://wiki.orbeon.com/forms/doc/contributor-guide/browser#TOC-Firefox-doesn-t-support-position:-r

Other browsers (WebKit, Opera, IE) ignore "position:relative" for the rendering of a table cell (that's the "undefined" part of the spec regarding tables) but they continue to respect it to determine the containing block of its descendants (which is a different and non-ambiguous part of the spec). Firefox should do the same.

A simple example can be found here: http://jsfiddle.net/bLPA6/ Webkit and IE8 renders it "correctly" as intended by the designer. Except that IE8 does not understand the border-radius as it was never implemented in that browser. Opera 12 ignores the container's width (left:0 and right:0) but does respect the container's height (top:0 and bottom:0). IE9 respects the container's width (left:0 and right:0) but ignores the container's height (top:0: bottom:0) and is therefore the opposite of Opera 12 IE7 is like IE8 but adds extra padding from somewhere. Firefox 13 ignores the position:relative on the TD element and therefore the contained elements are positioned relative to the HTML element, completely covering the grey table. This is the worst implementation of the bunch. Rendering problems with the other browsers can be mitigated by adding min-height and min-width rules, or text-align: right, etc. etc. But the proposed "fix" for Firefox, which is to mess around with the display:block or block-inline is not practical.

For web designers, this feature is no less important.

Seemingly more attention gets newer bug 63895

I just experienced this bug by adding { position: relative } to an element that has { display: table-cell }.

Just in case. here is online demo: http://jsbin.com/inomod/6/edit First box with the bug and other boxes are possible workarounds

This bit us in bug 845837.

Some issues we'd need to deal with to fix this: http://lists.w3.org/Archives/Public/www-style/2013Mar/0169.html http://lists.w3.org/Archives/Public/www-style/2013Mar/0170.html

[parity-ie][parity-chrome]

@dbaron I do see the issues mentioned. Here is Chrome's behavior: For border-collapse:separate: - td's border and background and content move. - tr's background doesn't move, any border rule is ignored. For border-collapse:collapse: - td's background and content move, border doesn't. - tr's background nor border moves. However I think the most important issue troubling users is the td acting as a box for descendants to position to. This works correctly in both IE and Google Chrome. Personally I would prefer if Firefox let descendants position to the td first, and save the exact rendering for the td later.

(In reply to henryfhchan from comment #29) > However I think the most important issue troubling users is the td acting as > a box for descendants to position to. This works correctly in both IE and > Google Chrome. See bug 63895 for that.

Per discussion with dbaron, it sounds like we need to check what other browsers are doing here. (Haven't read through the whole bug.)

In particular, I'm inclined to think that: * relative positioning of cells should not move the cell's background or border (this is supported by the spec's Appendix E) + but it's possible this isn't the right approach for border-collapse:separate borders, and maybe even that it's not the right approach for backgrounds. It's definitely right for border-collapse: collapse borders, though. This is the area where I think it's most worth testing other browsers. (See also bug 858333.) * relative positioning of rows and row groups should move the cells descended from those rows or row groups (which may span out of those rows) and should not move cells that span into those rows; it should move backgrounds only exactly as much as relatively positioning cells does * relative positioning of columns and column groups should not move any table parts * once bug 63895 is fixed, relative positioning of any of the above (including columns and column groups) should move absolutely positioned descendants. (Not sure whether it's high enough priority to bother implementing the columns/column groups part, though.) There are also some interesting questions about interactions with table background painting, but we already have those questions for 'opacity'; adding this just exposes them more. I'd also note that when we fix this, we need to revert the fix for bug 845837.

Also, if comment 18 is still correct, it may well be that bug 63895 is substantially higher priority than this.

It's worth noting that table parts are a significant potential use case for sticky positioning (bug 886646), and fixing this would quite likely make that work.

Come on guys!!! This bug seems to last for 13 years. Don't you think that it would be time to fix it? Webkit, Trident and Presto all handle this correctly. Gecko is the only that does not. http://jsfiddle.net/gudoy/pcYT9/

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #33) > Also, if comment 18 is still correct, it may well be that bug 63895 is > substantially higher priority than this. It is still correct. Please fix bug 63895, because Firefox is currently the only browser that needs an extra wrapper to position something absolutely in the table cell!

It seems like we probably actually split out the position aspects and the z-order/painting aspects into 2 separate dependent bugs (and use bug 63895 as the third bug, for containing blocks). And I tend to think we should probably fix the positioning and the containing blocks first; the z-order and painting stuff will be a bit more work due to connections to other things.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #27) > Some issues we'd need to deal with to fix this: > http://lists.w3.org/Archives/Public/www-style/2013Mar/0169.html > http://lists.w3.org/Archives/Public/www-style/2013Mar/0170.html This *might* be vaguely interesting as a followup to those: http://lists.w3.org/Archives/Public/www-style/2013Jul/0081.html

(Though that discussion is more directly related to bug 858333.)

Also, bug 688556 might be an appropriate bug to use for working on the z-order and painting aspects of this. Or maybe not... see bug 688556 comment 7.

Here's an initial version of the patch, though it's not ready for review yet. Relative positioning currently works with all internal table objects, although as I write this I realize I didn't try <thead> and <tfoot>. Dynamic changes are handled as well. (This was straightforward, I just had to remove the code the deliberately disabled them for table parts.) I haven't yet verified that the behavior of backgrounds and borders meets the requirements in comment 32. Per Corey's comment 34, I'll also take a look at sticky positioning. If it's straightforward to make it work for table parts as part of this bug, I'll do that. (It might be that no work at all is required!) Also missing is Part 2, which will contain tests for this functionality.

I've confirmed that <thead> and <tfoot> work just fine with the existing patch.

So here's a report on the patch's current behavior with respect to the requirements for background and border painting in comment 32. This is basically what we get "for free" - that is, without writing any special code to modify the background and border behavior in the relative positioning case. (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #32) > * relative positioning of cells should not move the cell's background or > border (this is supported by the spec's Appendix E) > + but it's possible this isn't the right approach for > border-collapse:separate borders, and maybe even that it's not the right > approach for backgrounds. It's definitely right for border-collapse: > collapse borders, though. This is the area where I think it's most worth > testing other browsers. (See also bug 858333.) I'll test other browsers in a followup comment, as my intuition is different than yours here. For me, it's most intuitive that both backgrounds and borders should move in the separate borders case, and backgrounds (but not borders) should move in the collapsed borders case. (Though I could easily be convinced otherwise on that second part. Definitely not a strongly held belief.) It so happens that that is exactly the behavior we get 'for free'. As the patch stands right now, in the separated borders model both the borders and the backgrounds move, while in the collapsed borders model only the backgrounds move. This is true for all table parts of interest. (In particular, I intend that relative positioning of columns and column groups should do absolutely nothing. To that end I won't report on them further in this comment.) > * relative positioning of rows and row groups should move the cells > descended from those rows or row groups (which may span out of those rows) > and should not move cells that span into those rows; it should move > backgrounds only exactly as much as relatively positioning cells does We get this for free too. > * once bug 63895 is fixed, relative positioning of any of the above > (including columns and column groups) should move absolutely positioned > descendants. (Not sure whether it's high enough priority to bother > implementing the columns/column groups part, though.) I'd prefer that columns and column groups don't do this. Bug 63895 has a patch that implements the rest of this requirement. > There are also some interesting questions about interactions with table > background painting, but we already have those questions for 'opacity'; > adding this just exposes them more. Sounds like another bug. =) > I'd also note that when we fix this, we need to revert the fix for bug > 845837. As I mentioned in person, I did this as part of the patch.

Just did a quick test and our behavior with the current patch matches both Safari and Chrome, except that neither Safari nor Chrome allows relative positioning of table parts of than cells. The background and border interactions are the same, though. This is also what was found in comment 29. I'd like to check IE too, but it seems like this patch is web-compatible.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #32) > * once bug 63895 is fixed, relative positioning of any of the above > (including columns and column groups) should move absolutely positioned > descendants. (Not sure whether it's high enough priority to bother > implementing the columns/column groups part, though.) I take back the bit about relative positioning of columns and column groups moving anything.

I've now tested the separated borders model in various versions of IE. Modern IE (8+) renders identically to WebKit / Blink browsers. It works just like this patch would make Gecko work, except that the only table parts that get relatively positioned are table cells. There are some small differences between the versions. Border painting is broken by relative positioning in IE 8 and looks terribly ugly. In IE 9/10, when a cell is positioned, the space it would have occupied without positioning is filled with the table background. However, in IE 11, that space is filled with the row background. The row background is also used in WebKit / Blink and that's the behavior we should seek to emulate. IE 6 and 7 differ from the newer IE's in that relative positioning also applies to table rows. The same caveats about IE 8 apply: border painting is totally broken by relative positioning. (These IE versions also have the same background painting behavior as IE 8-10.) This test made one thing clear to me which I initially overlooked in comment 45 above: to be compatible with the other major browsers, it's important that we paint the row background and not just the table background in the original location of table cells that have been relatively positioned. The current patch doesn't do that, and it should be fixed before checkin.

(In reply to Seth Fowler [:seth] from comment #48) > I've now tested the separated borders model in various versions of IE. Ack, meant collapsed here. Relabeled the testcase to reduce confusion.

I've now tested the separated borders model (for real this time!) in various versions of IE. Essentially the same comments apply as with the collapsed borders case. It's interesting to note that older IEs handled drawing the separated borders model in combination with relative positioning much more cleanly than they handled the collapsed borders model.

(In reply to Seth Fowler [:seth] from comment #42) > Per Corey's comment 34, I'll also take a look at sticky positioning. If it's > straightforward to make it work for table parts as part of this bug, I'll do > that. (It might be that no work at all is required!) This will likely just require removing the other check of nsStyleDisplay::IsInnerTableStyle that I added in bug 925259, in nsFrame::Init.

(In reply to Corey Ford [:corey] from comment #52) > This will likely just require removing the other check of > nsStyleDisplay::IsInnerTableStyle that I added in bug 925259, in > nsFrame::Init. That makes position: sticky have some effect, (it changes the position of the table part compared to position: relative), but it doesn't actually behave stickily. There must be another problem.

Here's a more convenient test case that covers a wide range of scenarios.

This is almost ready for review. The only thing that remains is to double-check the handling of overflow areas. In particular, I noticed some overflow-related optimizations in nsTablePainter.cpp and I want to make sure that none of them make assumptions that are incorrect now that table parts support relative positioning. I have also started the process of constructing tests based upon the test cases I've posted in this bug. They'll include everything those test cases include as well as tests involving colspan and rowspan.

Wow, "double-check the handling of overflow areas" was an important thing indeed! There are severe invalidation bugs in the previous patch. I've got things working now, though, and am in the process of constructing a final, review-ready patch.

I found this version of the testcase super helpful in debugging the invalidation issues. It's strictly better than the previous combined testcase.

Alright, this is ready for review. This patch handles relative positioning of all internal table parts. (Excluding columns and column groups, of course.) Part 2 will follow with tests.

To reduce the burden on your imagination, I'm uploading screenshots of the patch's behavior.

(This is with the 'combined testcase' I just uploaded.)

This fixes all known issues with the patch. (Specifically, previous to this version relative positioning wasn't applied to table cells if they were repositioned without being reflowed, due to e.g. a resize event.)

Here are tests for table part relative positioning. There will also be a part 3 with dynamic tests, but I'll wait until this part is r+'ed, since the tests in part 3 will be variations of these tests. These tests have uncovered a bug (the 3rd test fails for both separate and collapsed borders). I'll dig into that and update part 1 once I figure it out.

It turned out that a little helper function on nsIFrame proved useful. GetNormalRect is to GetRect as GetNormalPosition is to GetPosition.

I resolved the test failures revealed by the tests. It turned out that this patch needed to grow substantially, since there are many cases where the table layout code uses functions like GetRect, GetPosition, SetRect, and SetPosition without considering relative positioning. I combed through all of the code in nsTableFrame, nsTableRowGroupFrame, nsTableRowFrame, and nsTableCellFrame with these issues in mind, and I believe I found all of the problematic code. The tests for this bug now pass, but to make sure I haven't broken anything I'll submit a new try job shortly.

The only thing that has changed for these tests is the commit message.

Here's a try job for the updated patches: https://tbpl.mozilla.org/?tree=Try&rev=81cb32f021c6

Looks like there are some oranges there. Will debug on Monday.

This updated version of the patch fixes all of the issues uncovered with the last try job.

New try job here, hopefully all green: https://tbpl.mozilla.org/?tree=Try&rev=7064d659ef6b

Can this land? The failures seem irrelevant to this bug :)

(In reply to henryfhchan from comment #71) > Can this land? The failures seem irrelevant to this bug :) They are. The patch seems to be working fine. But nothing can land until it's reviewed. Don't worry - we're almost there!

Comment on attachment 8394680 [details] [diff] [review] (Part 3) - Add tests for table part relative positioning. > skip-if(B2G) include abs-pos/reftest.list >+skip-if(B2G) include position-relative/reftest.list The skip-if(B2G) is no longer there for the neighboring lines, and you shouldn't add it for this line either. (Make sure not to drop the change entirely due to the merge conflicts, too!) It would be good if all of the tests fit within a 600x600 viewport for sharing with other browsers and potential future changes in the size of the reftest viewport; that's an issue for the top/bottom tests in particular, and should be straightforward to fix by cutting the font size and the spacing a bit in the top/bottom tests. r=dbaron with that

Comment on attachment 8397620 [details] [diff] [review] (Part 2) - Allow relative positioning of internal table objects. Here's my review so far; it's complete except for the two "TODO" comments at the end: RestyleManager.cpp ================== Maybe this removal should only be a partial removal until bug 975644 is fixed? In other words, instead of removing the test, move it inside the sticky if statement just below? nsTableFrame.cpp ================ In nsTableFrame::ReflowChildren, where you use kidRect to MovePositionBy, you may as well convert the kidRect variable to kidPosition and use the simpler GetNormalPosition, since you actually only need the y member. It might be nice to change ResizeCells to call GetSize rather than GetRect (twice). In DistributeHeightToRows, in the first loop: >- rgFrame->InvalidateFrameWithRect(oldRowRect); >+ rgFrame->InvalidateFrameWithRect(rowRect); // The row's old rect. This needs to invalidate at the with-rel-pos rect. (It might be simplest to move the invalidate call earlier, assuming that works.) Same function, in the second loop: > nsTableFrame::InvalidateTableFrame(rowFrame, rowRect, rowVisualOverflow, > false); and > nsTableFrame::InvalidateTableFrame(rgFrame, rgRect, rgVisualOverflow, > false); You need to pass a with-rel-pos rect as the second param. (You fixed up the same thing in the first loop; this is the second loop.) In GetBaseline, could you change the GetRect().height below your existing changes to GetSize().height? nsTablePainter.cpp ================== >+ // We have to draw backgrounds not only within the overflow region of this >+ // row group, but also possibly (in the case of column / column group >+ // backgrounds) at its pre-relative-positioning location. >+ nsRect rgOverflowRect(rg->GetVisualOverflowRectRelativeToSelf() + rg->GetPosition()); >+ nsRect rgNormalRect(rg->GetNormalPosition(), rg->GetSize()); >+ >+ if (rgOverflowRect.Union(rgNormalRect).Intersects(mDirtyRect - mRenderPt)) { I think you want rgNormalRect to have the same size as rgOverflowRect rather than a potentially smaller size. (I don't quite have a concrete example, but consider the intersection of overflow-from-spanning-out and overflow-from-rel-pos.) For ComputeCellBackgrounds: name the parameters with a, not o. Use "Out" in the name if you want to. nsTableRowFrame.cpp =================== There are two bugs in ReflowChildren related to what happens if ReflowChildren would be moving a frame in a way that is exactly cancelled out by its relative positioning offset. The first of these is in the doReflowChild case in the case where we didn't actually reflow it: > if (x != kidRect.x) { > kidFrame->InvalidateFrameSubtree(); > } This needs to be comparing x to kid->GetNormalPosition().x. Then the same problem is present in the !doReflowChild branch: > if (kidRect.x != x) { (You might want to put the result of GetNormalPosition() in another variable declared next to kidRect.) Instead of refactoring when we call CalcAvailWidth and when a reflow state is constructed, can't you just use MovePositionBy in the two cases where you now call ApplyRelativePositioning? You can just pass the difference between GetNormalPosition().x and the new x, no? (Those changes have a substantive performance cost.) >+ kidPosition.y = kidFrame->GetNormalPosition().y; Maybe just assert that kidFrame->GetNormalPosition().y == 0? (And the same for the other don't-reflow codepath, where you're currently just assuming it?) nsTableRowFrame::CollapseRowIfNecessary: > if (aRowOffset == 0 && cRect.TopLeft() != oldCellRect.TopLeft()) { This line, which you didn't modify, seems like it should now be checking whether the MovePositionBy() call's offset is nonzero. (Without that change it will invalidate incorrectly in many cases when there's relative positioning, and in a few cases where the math works out exactly right, fail to invalidate, just like the cases I mentioned in Reflow.) (Maybe look for other similar patterns; I didn't look that closely.) Also, maybe change nsTableRowFrame::DidResize to call GetSize() instead of GetRect() for clarity? (Just don't simplify away the nsSize constructor, which can't be simplified away since one of the params is different from the old size.) nsTableRowGroupFrame.cpp ======================== I think DisplayRows needs the same adjustment to union the rel-pos and non-rel-pos positions that TableBackgroundPainter::PaintRowGroup has, so that you don't skip display list construction when the backgrounds will be outside the rect you're checking. >+ PlaceChild(aPresContext, aReflowState, kidFrame, kidPosition, desiredSize, > oldKidRect, oldKidVisualOverflow); Please wrap desiredSize to the second line. >+ bool movedFrame = (rowNormalPosition.y != yOrigin); Maybe clearer to call this: nscoord deltaY = yOrigin - rowNormalPosition.y; and then: (1) test deltaY != 0 (2) use deltaY in the MovePositionBy call rather than redoing the subtraction TODO: I'm not sure whether you need to change nsTableRowGroupFrame::GetLine and nsTableRowGroupFrame::FindFrameAt. (They're the implementation of nsILineIterator.) TODO: GetFirstRowContaining - mostly used for painting, but what to do?

Thanks for the review! Most of these require more investigation, but a few quick responses: (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #74) > Maybe this removal should only be a partial removal until bug 975644 > is fixed? In other words, instead of removing the test, move it > inside the sticky if statement just below? Yeah, that's probably a good idea. > For ComputeCellBackgrounds: name the parameters with a, not o. Use > "Out" in the name if you want to. I'm in favor of making the "o" prefix accepted style, but I won't fight that battle in this bug. =) (Note that there is precedent, as some parts of the codebase do use it.) > TODO: GetFirstRowContaining - mostly used for painting, but what to do? As I mentioned in person, this change is probably the thing in the patch I was most unsure about. I'll be interested to hear what you have to say there.

Comment on attachment 8397620 [details] [diff] [review] (Part 2) - Allow relative positioning of internal table objects. nsILineIterator =============== * the comments for nsILineIterator::GetLine explicitly say that rel-pos may move things outside of the line bounds * The *only* user of FindFrameAt is nsFrame::GetNextPrevLineFromeBlockFrame * It looks to me like the only user of the lineBounds outparam from GetLine is nsFrame::RefreshSizeCache, which should probably be using nsBlockFrame directly instead of using the line iterator, and which can't have a row group frame. (The flags param also looks unused.) So: * we should probably file a followup on removing the rect and flags params from GetLine, which means converting nsFrame::RefreshSizeCache to access block frames directly * FindFrameAt is probably used for caret movement, so probably should consider relative positioning, which means no API change is needed. * So API-wise, keeping this as they are is probably fine. * However, it's more problematic that FindFrameAt assumes the rects of cells are monotonically increasing. However, I don't see a good alternative, so I'd say leave as-is. GetFirstRowContaining ===================== So GetFirstRowContaining is using the row cursor optimization that's set up in DisplayRows. See those methods and nsTableRowGroupFrame::FrameCursorData::AppendFrame to get some understanding of what it does. (After reading all three of those, and the comments in nsTableRowGroupFrame.h, I now understand what it's for. Let me know if you need more explanation.) This code as it exists today appears to handle overflow areas fine (though pessimistically, but it's an optimization so that's fine). But it assumes that the rows rects are monotonically increasing -- and you're changing it so that's no longer true. So I think what you ought to do is: * change nsTableRowGroupFrame::FrameCursorData::AppendFrame to treat the relative offset as an additional contribution to overflow, so that from the row cursor's perpective the "overflow" is an offset from the normal rect. (And add some comments explaining this.) * leave your change to GetFirstRowContaining to use GetNormalRect as you have * change DisplayRows to use GetNormalRect as well [this replaces my previous comment on DisplayRows] * change the place you're already fixing an existing bug in TableBackgroundPainter::PaintRowGroup to consider overflowAbove in the new way as well. (And I now understand what that code is doing; perhaps some comments might help.) FOLLOWUP: But I realize that we need a RecomputeOverflow override to clear the row cursor, and probably also more to handle RecomputePosition correctly, since we have a bunch of optimizations that don't call ClearRowCursor() when they should. But that should be in a separate patch, not this one! Do you mind filing that? So r=dbaron with comment 74 plus the comments on GetFirstRowContaining here. Let me know if you want me to spot-check your addressing of these comments.

All of the review comments above should be addressed, but given the amount of time that has passed, a second pair of eyes wouldn't hurt. As I mentioned in person, I encountered a new assertion while running the reftests after rebasing. I was able to track that down to bug 1051147, which I've uploaded a patch for. It's unrelated to the code in this patch.

It looks like bug 913586, which substantially changes the Maybe<T> API, will almost certainly land at least partially before this bug. Accordingly, I've rebased this patch to use the new Maybe<T> API. Nothing of substance has changed.

Comment on attachment 8471148 [details] [diff] [review] (Part 2) - Allow relative positioning of internal table objects Missed this comment from comment 74: >In nsTableFrame::ReflowChildren, where you use kidRect to MovePositionBy, >you may as well convert the kidRect variable to kidPosition and use >the simpler GetNormalPosition, since you actually only need the y >member. In nsTableFrame.cpp, I don't like the name "origRowNormalRect" that you introduced, since we use "Normal" for "without relative positioning", whereas you're using it here for "with relative positioning". If anything, I'd rename "rowRect" -> "rowNormalRect" and "origRowNormalRect" -> "origRowRect", or similar. Similar for rgRect and origRgNormalRect (outside of those in the loop). And the same for the first pair of names happening again in a later loop. And for one more origRgNormalRect in the last changed hunk. Your nsTableFrame::GetBaseline changes seem to have disappeared entirely. You should reinstate them, in the new nsTableFrame::GetLogicalBaseline, and then address this from comment 74: >In GetBaseline, could you change the GetRect().height below >your existing changes to GetSize().height? In nsTableBackgroundPainter::PaintTable: >+ nsRect rgNormalRect(rg->GetNormalPosition(), rgOverflowRect.Size()); This isn't quite right, since rgOverflowRect's Size might start from a different origin. I think the simplest fix is to make rgNormalRect be rg->GetVisualOverflowRectRelativeToSelf() + rg->GetNormalPosition() (and probably also cache rg->GetVisualOverflowRectRelativeToSelf() so you can use it for both rgOverflowRect and rgNormalRect). nsTableRowFrame: >+ kidPosition.y = origKidNormalPosition.y; I'm not sure why this is needed; the y in the existing code is always 0. I think this line can be dropped. >+ // We didn't reflow. To take relative positioning into account, >+ // translate the new position by the vector from the previous 'normal' >+ // position to the previous position. >+ kidPosition += kidRect.TopLeft() - origKidNormalPosition; This isn't correct for sticky; you should at the very least leave a FIXME saying so. >- if (kidRect.x != x) { >+ if (x != origKidNormalPosition.x) { > // Invalidate the old position > kidFrame->InvalidateFrameSubtree(); >- // move to the new position >- kidFrame->SetPosition(nsPoint(x, kidRect.y)); >+ // Move to the new position. As above, we need to account for relative >+ // positioning. >+ kidPosition.y = origKidNormalPosition.y; >+ kidPosition += kidRect.TopLeft() - origKidNormalPosition; >+ kidFrame->SetPosition(kidPosition); Here making sticky work correctly would actually simplify tho code; the above three lines (including incorporating here my comment above on the other chunk about setting kidPosition.y being useless) could just be: kidFrame->MovePositionBy(nsPoint(x - origKidNormalPosition.x, 0)); >+ nsRect oldCellNormalRect = cellFrame->GetNormalRect(); Just use GetNormalPosition rather than GetNormalRect; you only need the x and y here. nsTableRowGroupFrame.cpp: nsTableRowGroupFrame::FrameCursorData::AppendFrame: >+ nsRect normalOverflowRect(aFrame->GetNormalPosition(), >+ positionedOverflowRect.Size()); Again, this pattern isn't right, because you're throwing away the x/y component of the original visual overflow rect (and where it is relative to the frame). You probably want to offset positionedOverflowRect by the relative offset and add a FIXME that it isn't correct in the presence of transforms. r=dbaron with that Could you file the followup on nsILineIterator from comment 76, or should I?

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #79) > Comment on attachment 8471148 [details] [diff] [review] > (Part 2) - Allow relative positioning of internal table objects > > Missed this comment from comment 74: > >In nsTableFrame::ReflowChildren, where you use kidRect to MovePositionBy, > >you may as well convert the kidRect variable to kidPosition and use > >the simpler GetNormalPosition, since you actually only need the y > >member. I should've explicitly commented and said so, but that turns out not to be true, because we also need kidRect.height later in the function, so I couldn't make that change.

OK, thanks for that review, David. Those were all excellent points. All the issues you pointed out should be resolved in this version of the patch.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e15a7a0c8f70 https://hg.mozilla.org/integration/mozilla-inbound/rev/852b5ce53278 https://hg.mozilla.org/integration/mozilla-inbound/rev/f36adee1958f https://hg.mozilla.org/integration/mozilla-inbound/rev/479ff28f783f

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment > Could you file the followup on nsILineIterator from comment 76, or > should I? I went ahead and filed all the followups we discussed.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee51b30470 There were both B2G build failures and reftest failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=479ff28f783f

Just noticed that B2G ICS tests weren't actually run on that push, so you don't see the failures there. Here's a later changeset where you can see the B2G build failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f2d2bc1b2f4b

https://hg.mozilla.org/mozilla-central/rev/e15a7a0c8f70 https://hg.mozilla.org/mozilla-central/rev/852b5ce53278 https://hg.mozilla.org/mozilla-central/rev/f36adee1958f https://hg.mozilla.org/mozilla-central/rev/479ff28f783f https://hg.mozilla.org/mozilla-central/rev/3eee51b30470

Two things of note: (1) reftest failures were on 32-bit Linux but not 64-bit Linux (2) https://hg.mozilla.org/integration/mozilla-inbound/rev/479ff28f783f should probably have tests in it when relanding

So another useful thing to do would be to try valgrind and see if it says anything interesting. (Do we run valgrind in automation?) That said, I think I see a likely source of the problem: In TableBackgroundPainter::PaintRowGroup, overflowAbove can be uninitialized, since GetFirstRowContaining only fills it in when it returns a non-null cursor. I think that should be fixed to only do the test for bailing for later rows if a row cursor was actually returned.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #88) > (Do we run valgrind in automation?) We run Valgrind builds per-push, but only on the PGO pageset. Bug 979683 tracks doing more, but is lacking an owner.

What's the current hold up for this? Looking forward to the patch getting relanded

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #87) > (2) https://hg.mozilla.org/integration/mozilla-inbound/rev/479ff28f783f > should probably have tests in it when relanding If you happen to have the actual tests, and can attach them, then it's possible that somebody else could do the other part: (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #88) > That said, I think I see a likely source of the problem: In > TableBackgroundPainter::PaintRowGroup, overflowAbove can be uninitialized, > since GetFirstRowContaining only fills it in when it returns a non-null > cursor. > > I think that should be fixed to only do the test for bailing for later rows > if a row cursor was actually returned.

Here's a try run with the fix from comment 88, and to find out again what the B2G build failures were: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=614baab5da66 https://tbpl.mozilla.org/?tree=Try&rev=614baab5da66

The B2G ICS (gcc 4.4) bustage is easily fixed with an explicit cast to uint32_t so that it will match the constructor through the emplace() call.

I have the patches (minus the dynamic tests, which only seth has) queued up to reland, whenever the tree happens to be open...

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/bce28de2368a https://hg.mozilla.org/integration/mozilla-inbound/rev/853447a587aa https://hg.mozilla.org/integration/mozilla-inbound/rev/c8fd5af3c60c with the fixes from comment 88 (cursor &&) and comment 95 (uint32_t()). Still need to hear from Seth as to whether he actually has the tests from part 4.

https://hg.mozilla.org/mozilla-central/rev/bce28de2368a https://hg.mozilla.org/mozilla-central/rev/853447a587aa https://hg.mozilla.org/mozilla-central/rev/c8fd5af3c60c

Was this actually fixed? What was the original intent of this bug report? Absolutely positioned child elements seem to be placed correctly, but the disappearing borders issue related to it remains, ie. border-collapse on the table and a 1px border plus position relative on all table cells does bizarre things to the borders. Or is that another bug entirely? Im using Firefox 37.0.2 on Ubuntu 14.04.

That's not this bug. It might be bug 688556; if it's not, it should probably be filed separately.