Closed Bug 35168 Opened 24 years ago Closed 9 years ago

relative positioning of table cells doesn't work

Categories

(Core :: Layout: Tables, defect, P3)

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: dbaron, Assigned: seth, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: css3, dev-doc-needed, testcase, Whiteboard: [awd:tbl])

Attachments

(9 files, 10 obsolete files)

testcase described in bug
436 bytes, text/html
Details
Testcase with collapsed borders.
1.19 KB, text/html
Details
Testcase with separated borders.
1.19 KB, text/html
Details
Combined test-case with invalidation debugging.
2.63 KB, text/html
Details
Screenshot of patch behavior with 'border-collapse: separate'.
68.72 KB, image/png
Details
Screenshot of patch behavior with 'border-collapse: collapse'.
68.21 KB, image/png
Details
(Part 1) - Add GetNormalRect to nsIFrame.
2.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Part 3) - Add tests for table part relative positioning.
47.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects
63.27 KB, patch
Details | Diff | Splinter Review 
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...
Keywords: 4xp, css2
Keywords: testcase
Whiteboard: [TESTCASE]
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Target Milestone: M17 → M18
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.
Target Milestone: M18 → Future
As per meeting with ChrisD today, taking QA.
QA Contact: chrisd → py8ieh=bugzilla
Summary: relative positioning of table cells doesn't work → relative positioning of table cells doesn't work [REL POS]
QA Contact: ian → amar
Whiteboard: [TESTCASE] → [awd:tbl][TESTCASE]
*** 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.
OS: Linux → All
Hardware: PC → All
Blocks: 175590
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---
Target Milestone: --- → Future
Whiteboard: [awd:tbl][TESTCASE] → [awd:tbl]
It may be worth noting that Internet Explorer 6.0 renders this case correctly.
QA Contact: madhur → ian
Summary: relative positioning of table cells doesn't work [REL POS] → relative positioning of table cells doesn't work
"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.
Assignee: layout.tables → nobody
QA Contact: ian → layout.tables
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
[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.
Keywords: css2 → css3
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.
Assignee: nobody → seth
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.
Attachment #8378559 - Attachment description: Another testcase. → Testcase with collapsed borders.
(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.
Blocks: 975644
Attached file Combined test-case. (obsolete) — 
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.
Attachment #8375984 - Attachment is obsolete: true
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.
Attachment #8381235 - Attachment is obsolete: true
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.
Attachment #8382012 - Flags: review?(dbaron)
Attachment #8381239 - Attachment is obsolete: true
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.)
Attachment #8385094 - Flags: review?(dbaron)
Attachment #8382012 - Attachment is obsolete: true
Attachment #8382012 - Flags: review?(dbaron)
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.
Attachment #8392775 - Flags: review?(dbaron)
It turned out that a little helper function on nsIFrame proved useful. GetNormalRect is to GetRect as GetNormalPosition is to GetPosition.
Attachment #8394677 - Flags: review?(dbaron)
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.
Attachment #8394678 - Flags: review?(dbaron)
Attachment #8385094 - Attachment is obsolete: true
Attachment #8385094 - Flags: review?(dbaron)
The only thing that has changed for these tests is the commit message.
Attachment #8394680 - Flags: review?(dbaron)
Attachment #8392775 - Attachment is obsolete: true
Attachment #8392775 - Flags: review?(dbaron)
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.
Attachment #8397620 - Flags: review?(dbaron)
Attachment #8394678 - Attachment is obsolete: true
Attachment #8394678 - Flags: review?(dbaron)
New try job here, hopefully all green:

https://tbpl.mozilla.org/?tree=Try&rev=7064d659ef6b
Keywords: dev-doc-needed
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!
Attachment #8394677 - Flags: review?(dbaron) → review+
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
Attachment #8394680 - Flags: review?(dbaron) → review+
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.
Attachment #8397620 - Flags: review?(dbaron) → review+
Depends on: 1051147
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.
Attachment #8470398 - Flags: review?(dbaron)
Attachment #8397620 - Attachment is obsolete: true
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.
Attachment #8471148 - Flags: review?(dbaron)
Attachment #8470398 - Attachment is obsolete: true
Attachment #8470398 - Flags: review?(dbaron)
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?
Attachment #8471148 - Flags: review?(dbaron) → review+
(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.
Attachment #8471148 - Attachment is obsolete: true
Blocks: 1055892
Blocks: 1055894
Blocks: 1055896
(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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Future → ---
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.
Flags: needinfo?(seth)
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...
Depends on: 1115999
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: