Last Comment Bug 35168 - relative positioning of table cells doesn't work
: relative positioning of table cells doesn't work
Status: RESOLVED FIXED
[awd:tbl]
: css3, dev-doc-needed, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: P3 normal with 41 votes (vote)
: mozilla37
Assigned To: Seth Fowler [:seth] [:s2h]
:
Mentors:
: 139982 360603 427618 701746 763353 932784 1112750 (view as bug list)
Depends on: 1051147 1115999
Blocks: 975644 1055892 1055894 1055896 175590 1176337
  Show dependency treegraph
 
Reported: 2000-04-08 10:02 PDT by David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
Modified: 2015-06-19 11:49 PDT (History)
54 users (show)
dbaron: needinfo? (seth.bugzilla)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase described in bug (436 bytes, text/html)
2000-04-08 10:03 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
(Part 1) - Allow relative positioning of internal table objects. (14.24 KB, patch)
2014-02-13 18:38 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Testcase with collapsed borders. (1.19 KB, text/html)
2014-02-19 13:39 PST, Seth Fowler [:seth] [:s2h]
no flags Details
Testcase with separated borders. (1.19 KB, text/html)
2014-02-19 13:57 PST, Seth Fowler [:seth] [:s2h]
no flags Details
Combined test-case. (1.86 KB, text/html)
2014-02-25 01:06 PST, Seth Fowler [:seth] [:s2h]
no flags Details
(Part 1) - Allow relative positioning of internal table objects. (19.96 KB, patch)
2014-02-25 01:14 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Combined test-case with invalidation debugging. (2.63 KB, text/html)
2014-02-26 01:34 PST, Seth Fowler [:seth] [:s2h]
no flags Details
(Part 1) - Allow relative positioning of internal table objects. (29.66 KB, patch)
2014-02-26 01:39 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Screenshot of patch behavior with 'border-collapse: separate'. (68.72 KB, image/png)
2014-02-26 01:46 PST, Seth Fowler [:seth] [:s2h]
no flags Details
Screenshot of patch behavior with 'border-collapse: collapse'. (68.21 KB, image/png)
2014-02-26 01:46 PST, Seth Fowler [:seth] [:s2h]
no flags Details
(Part 1) - Allow relative positioning of internal table objects. (32.35 KB, patch)
2014-03-03 19:34 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 2) - Add tests for table part relative positioning. (47.42 KB, patch)
2014-03-18 03:00 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 1) - Add GetNormalRect to nsIFrame. (2.23 KB, patch)
2014-03-21 03:05 PDT, Seth Fowler [:seth] [:s2h]
dbaron: review+
Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects. (55.23 KB, patch)
2014-03-21 03:07 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 3) - Add tests for table part relative positioning. (47.42 KB, patch)
2014-03-21 03:08 PDT, Seth Fowler [:seth] [:s2h]
dbaron: review+
Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects. (56.24 KB, patch)
2014-03-26 22:58 PDT, Seth Fowler [:seth] [:s2h]
dbaron: review+
Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects (60.75 KB, patch)
2014-08-08 20:09 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects (60.72 KB, patch)
2014-08-11 15:28 PDT, Seth Fowler [:seth] [:s2h]
dbaron: review+
Details | Diff | Splinter Review
(Part 2) - Allow relative positioning of internal table objects (63.27 KB, patch)
2014-08-19 18:23 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2000-04-08 10:02:46 PDT
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
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2000-04-08 10:03:09 PDT
Created attachment 7371 [details]
testcase described in bug
Comment 2 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2000-04-08 10:03:46 PDT
Believe it or not, this works in 4.x...
Comment 3 karnaze (gone) 2000-06-06 22:03:19 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 4 Hixie (not reading bugmail) 2000-07-25 21:29:11 PDT
As per meeting with ChrisD today, taking QA.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-25 06:53:20 PDT
*** Bug 139982 has been marked as a duplicate of this bug. ***
Comment 6 Juan R. Pozo 2002-04-25 09:06:51 PDT
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.
Comment 7 Greg K. 2002-07-13 14:08:29 PDT
Reconfirmed using FizzillaCFM/2002071208. Setting All/All.
Comment 8 Juan R. Pozo 2002-08-29 16:17:56 PDT
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
Comment 9 karnaze (gone) 2003-03-31 10:42:10 PST
mass reassign to default owner
Comment 10 Idan Sofer 2003-11-17 23:27:42 PST
It may be worth noting that Internet Explorer 6.0 renders this case correctly.
Comment 11 Gérard Talbot 2006-11-14 10:51:08 PST
"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
Comment 12 Gérard Talbot 2006-11-18 01:22:12 PST
*** Bug 360603 has been marked as a duplicate of this bug. ***
Comment 13 Gérard Talbot 2008-04-07 20:15:12 PDT
*** Bug 427618 has been marked as a duplicate of this bug. ***
Comment 14 Ralph 2008-05-20 16:04:15 PDT
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").
Comment 15 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-05-20 18:14:23 PDT
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.
Comment 16 Mats Palmgren (vacation) 2011-08-22 16:04:09 PDT
*** Bug 680963 has been marked as a duplicate of this bug. ***
Comment 17 Alessandro Vernet 2012-02-16 18:31:44 PST
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
Comment 18 Pierre Saslawsky 2012-06-07 17:43:53 PDT
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.
Comment 19 Fred Truter 2012-06-15 03:51:12 PDT
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.
Comment 20 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-07-18 10:45:10 PDT
*** Bug 763353 has been marked as a duplicate of this bug. ***
Comment 21 Aldi 2012-07-18 18:03:09 PDT
For web designers, this feature is no less important.
Comment 22 Emil Ivanov 2012-10-10 15:46:34 PDT
*** Bug 701746 has been marked as a duplicate of this bug. ***
Comment 23 Michal Čaplygin [:myf] 2012-10-11 01:06:07 PDT
Seemingly more attention gets newer bug 63895
Comment 24 Jake Bellacera 2013-01-15 11:24:49 PST
I just experienced this bug by adding { position: relative } to an element that has { display: table-cell }.
Comment 25 Aleksandr Motsjonov 2013-01-21 04:25:07 PST
Just in case. here is online demo: http://jsbin.com/inomod/6/edit
First box with the bug and other boxes are possible workarounds
Comment 26 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-03-07 18:06:01 PST
This bit us in bug 845837.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-03-07 18:23:35 PST
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
Comment 28 henryfhchan 2013-08-14 06:34:27 PDT
[parity-ie][parity-chrome]
Comment 29 henryfhchan 2013-08-14 06:43:18 PDT
@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.
Comment 30 mjh563 2013-08-14 09:53:58 PDT
(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.
Comment 31 Seth Fowler [:seth] [:s2h] 2013-08-16 15:21:29 PDT
Per discussion with dbaron, it sounds like we need to check what other browsers are doing here. (Haven't read through the whole bug.)
Comment 32 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-08-16 16:39:30 PDT
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.
Comment 33 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-08-16 16:41:40 PDT
Also, if comment 18 is still correct, it may well be that bug 63895 is substantially higher priority than this.
Comment 34 Corey Ford [:coyotebush] 2013-08-16 16:55:10 PDT
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.
Comment 35 Guyllaume Doyer 2013-09-10 07:28:00 PDT
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/
Comment 36 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2013-10-30 08:40:34 PDT
*** Bug 932784 has been marked as a duplicate of this bug. ***
Comment 37 Ilya 2013-12-09 11:58:14 PST
(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!
Comment 38 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-12-11 14:30:43 PST
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.
Comment 39 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-12-13 16:39:27 PST
(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
Comment 40 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-12-13 16:40:42 PST
(Though that discussion is more directly related to bug 858333.)
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-01-31 17:21:49 PST
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.
Comment 42 Seth Fowler [:seth] [:s2h] 2014-02-13 18:38:16 PST
Created attachment 8375984 [details] [diff] [review]
(Part 1) - Allow relative positioning of internal table objects.

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.
Comment 43 Seth Fowler [:seth] [:s2h] 2014-02-14 16:50:20 PST
I've confirmed that <thead> and <tfoot> work just fine with the existing patch.
Comment 44 Seth Fowler [:seth] [:s2h] 2014-02-14 17:23:10 PST
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.
Comment 45 Seth Fowler [:seth] [:s2h] 2014-02-14 17:30:18 PST
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.
Comment 46 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-02-14 18:47:48 PST
(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.
Comment 47 Seth Fowler [:seth] [:s2h] 2014-02-19 13:39:55 PST
Created attachment 8378559 [details]
Testcase with collapsed borders.
Comment 48 Seth Fowler [:seth] [:s2h] 2014-02-19 13:53:45 PST
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.
Comment 49 Seth Fowler [:seth] [:s2h] 2014-02-19 13:55:22 PST
(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.
Comment 50 Seth Fowler [:seth] [:s2h] 2014-02-19 13:57:11 PST
Created attachment 8378571 [details]
Testcase with separated borders.
Comment 51 Seth Fowler [:seth] [:s2h] 2014-02-19 14:01:31 PST
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.
Comment 52 Corey Ford [:coyotebush] 2014-02-20 04:55:57 PST
(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.
Comment 53 Seth Fowler [:seth] [:s2h] 2014-02-20 18:54:18 PST
(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.
Comment 54 Seth Fowler [:seth] [:s2h] 2014-02-25 01:06:26 PST
Created attachment 8381235 [details]
Combined test-case.

Here's a more convenient test case that covers a wide range of scenarios.
Comment 55 Seth Fowler [:seth] [:s2h] 2014-02-25 01:14:10 PST
Created attachment 8381239 [details] [diff] [review]
(Part 1) - Allow relative positioning of internal table objects.

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.
Comment 56 Seth Fowler [:seth] [:s2h] 2014-02-25 21:57:30 PST
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.
Comment 57 Seth Fowler [:seth] [:s2h] 2014-02-26 01:34:15 PST
Created attachment 8382008 [details]
Combined test-case with invalidation debugging.

I found this version of the testcase super helpful in debugging the invalidation issues. It's strictly better than the previous combined testcase.
Comment 58 Seth Fowler [:seth] [:s2h] 2014-02-26 01:39:03 PST
Created attachment 8382012 [details] [diff] [review]
(Part 1) - Allow relative positioning of internal table objects.

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.
Comment 59 Seth Fowler [:seth] [:s2h] 2014-02-26 01:46:08 PST
Created attachment 8382015 [details]
Screenshot of patch behavior with 'border-collapse: separate'.

To reduce the burden on your imagination, I'm uploading screenshots of the patch's behavior.
Comment 60 Seth Fowler [:seth] [:s2h] 2014-02-26 01:46:44 PST
Created attachment 8382016 [details]
Screenshot of patch behavior with 'border-collapse: collapse'.
Comment 61 Seth Fowler [:seth] [:s2h] 2014-02-26 01:47:42 PST
(This is with the 'combined testcase' I just uploaded.)
Comment 62 Seth Fowler [:seth] [:s2h] 2014-03-03 19:34:36 PST
Created attachment 8385094 [details] [diff] [review]
(Part 1) - Allow relative positioning of internal table objects.

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.)
Comment 63 Seth Fowler [:seth] [:s2h] 2014-03-18 03:00:21 PDT
Created attachment 8392775 [details] [diff] [review]
(Part 2) - Add tests for table part relative positioning.

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.
Comment 64 Seth Fowler [:seth] [:s2h] 2014-03-21 03:05:07 PDT
Created attachment 8394677 [details] [diff] [review]
(Part 1) - Add GetNormalRect to nsIFrame.

It turned out that a little helper function on nsIFrame proved useful. GetNormalRect is to GetRect as GetNormalPosition is to GetPosition.
Comment 65 Seth Fowler [:seth] [:s2h] 2014-03-21 03:07:53 PDT
Created attachment 8394678 [details] [diff] [review]
(Part 2) - Allow relative positioning of internal table objects.

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.
Comment 66 Seth Fowler [:seth] [:s2h] 2014-03-21 03:08:40 PDT
Created attachment 8394680 [details] [diff] [review]
(Part 3) - Add tests for table part relative positioning.

The only thing that has changed for these tests is the commit message.
Comment 67 Seth Fowler [:seth] [:s2h] 2014-03-21 03:11:07 PDT
Here's a try job for the updated patches:

https://tbpl.mozilla.org/?tree=Try&rev=81cb32f021c6
Comment 68 Seth Fowler [:seth] [:s2h] 2014-03-21 07:18:37 PDT
Looks like there are some oranges there. Will debug on Monday.
Comment 69 Seth Fowler [:seth] [:s2h] 2014-03-26 22:58:47 PDT
Created attachment 8397620 [details] [diff] [review]
(Part 2) - Allow relative positioning of internal table objects.

This updated version of the patch fixes all of the issues uncovered with the last try job.
Comment 70 Seth Fowler [:seth] [:s2h] 2014-03-26 23:01:28 PDT
New try job here, hopefully all green:

https://tbpl.mozilla.org/?tree=Try&rev=7064d659ef6b
Comment 71 henryfhchan 2014-03-30 17:57:32 PDT
Can this land? The failures seem irrelevant to this bug :)
Comment 72 Seth Fowler [:seth] [:s2h] 2014-03-31 14:48:25 PDT
(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 73 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-04-08 21:17:18 PDT
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 74 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-04-10 18:48:56 PDT
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?
Comment 75 Seth Fowler [:seth] [:s2h] 2014-04-11 17:26:39 PDT
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 76 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-04-14 16:03:51 PDT
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.
Comment 77 Seth Fowler [:seth] [:s2h] 2014-08-08 20:09:05 PDT
Created attachment 8470398 [details] [diff] [review]
(Part 2) - Allow relative positioning of internal table objects

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.
Comment 78 Seth Fowler [:seth] [:s2h] 2014-08-11 15:28:44 PDT
Created attachment 8471148 [details] [diff] [review]
(Part 2) - Allow relative positioning of internal table objects

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 79 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-08-18 17:23:36 PDT
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?
Comment 80 Seth Fowler [:seth] [:s2h] 2014-08-19 16:02:40 PDT
(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.
Comment 81 Seth Fowler [:seth] [:s2h] 2014-08-19 18:23:24 PDT
Created attachment 8475604 [details] [diff] [review]
(Part 2) - Allow relative positioning of internal table objects

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.
Comment 83 Seth Fowler [:seth] [:s2h] 2014-08-19 18:43:35 PDT
(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.
Comment 84 Seth Fowler [:seth] [:s2h] 2014-08-19 20:43:54 PDT
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
Comment 85 Seth Fowler [:seth] [:s2h] 2014-08-19 20:52:27 PDT
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
Comment 87 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-08-22 14:57:06 PDT
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
Comment 88 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-08-22 15:13:08 PDT
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.
Comment 89 Ryan VanderMeulen [:RyanVM] 2014-08-22 15:14:48 PDT
(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.
Comment 90 mail 2014-09-10 02:30:24 PDT Comment hidden (off-topic)
Comment 91 henryfhchan 2014-11-13 07:16:25 PST
What's the current hold up for this? Looking forward to the patch getting relanded
Comment 92 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-12-17 15:54:11 PST
(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.
Comment 93 Boris Zbarsky [:bz] 2014-12-17 17:09:11 PST
*** Bug 1112750 has been marked as a duplicate of this bug. ***
Comment 94 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-12-22 15:32:20 PST
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
Comment 95 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-12-22 20:25:28 PST
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.
Comment 96 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-12-22 20:37:46 PST
I have the patches (minus the dynamic tests, which only seth has) queued up to reland, whenever the tree happens to be open...
Comment 97 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-12-23 06:50:16 PST
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.
Comment 99 cam.graham 2015-05-18 15:00:58 PDT
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.
Comment 100 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2015-05-18 19:39:57 PDT
That's not this bug.  It might be bug 688556; if it's not, it should probably be filed separately.

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