1081683 - Relative position on TD or TH gives misleading warning (TablePartRelPosWarning)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Stefan Weiss]

Attachment: ff-TablePartRelPosWarning.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/34.0.1847.116 Chrome/34.0.1847.116 Safari/537.36

Steps to reproduce:

Load attached test case, which contains a minimal HTML document with a <table> and a CSS rule giving TD elements `position:relative`.


Actual results:

In the JS console (or Firebug, if enabled), the following text appears as warning: "Relative positioning of table rows and row groups is now supported. This site may need to be updated because it may depend on this feature having no effect."


Expected results:

No warning (see rationale below).

Comment 1

[Stefan Weiss]

The code emitting this warning is nsTableFrame::RegisterPositionedTablePart() in layout/tables/nsTableFrame.cpp, line 269:
http://code.metager.de/source/xref/mozilla/firefox/layout/tables/nsTableFrame.cpp#269
(line number current as of today)

A few things to note about the current behavior:

1) The warning is given on documents where everything is working fine, basically amounting to: Warning, what you are doing actually works!

2) There is no (apparent) way to get rid of the warning.

3) The text of the warning and the comment in the source do not match the situation where the warning is seen: "table parts other than table cells" should not include TD elements (see attached test case).

4) It's not necessary to actually change the position of a TD to get the warning; just `position:relative` without top/bottom/left/right is enough.

5) The original intention may have been to help broken sites track down the source of their error. Since the error was specifying "position:relative" and expecting this to have "no effect", conforming sites should not be punished by getting the warning.

6) The warning is NOT given if any of the following are true:

a) the TD's CSS position is "absolute" instead of "relative"
b) the CSS does not contain a rule to collapse cell borders in a table
c) the HTML does not contain actual TD elements

I have no idea what border collapsing has to do with this, but it certainly made it a lot harder to track down the reason for the warning.

7) Everything else aside, a warning about a changed feature should have an expiry date or release version set.

-

For what it's worth, the situation where I encountered the warning was an absolutely positioned SPAN in a table header. As far as I can see, there's currently no way to position a child element relative to its containing TD or TH without triggering the "TablePartRelPosWarning" warning.

Comment 2

[Nickolay_Ponomarev]

The warning http://hg.mozilla.org/mozilla-central/annotate/199fb29c3467/layout/tables/nsTableFrame.cpp#l270 was added in bug 63895.

Seth, could you comment on this bug?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> "table parts other than table cells" should not include TD elements 

Yes, this needs to be fixed.  The issue was that other UAs also did not support relpos on rows and such, so there may well have been sites depending on it not working.  Hence the warning for sites that tried to use it...

> It's not necessary to actually change the position of a TD to get the warning

It's not necessary to change the position of a relpos element to affect layout, since just having it be relpos it affects the positioning of abspos descendants.

> a) the TD's CSS position is "absolute" instead of "relative"

That makes it not be a table cell anymore, so there is no problem.

> b) the CSS does not contain a rule to collapse cell borders in a table

This indicates some sort of bug, indeed.  The bug is that the check in nsTableFrame::RegisterPositionedTablePart is only for the nsGkAtoms::tableCellFrame frame type, but when border collapse is enabled the cells have type nsGkAtoms::bcTableCellFrame.

> c) the HTML does not contain actual TD elements

It doesn't matter what the elements are, just what their display styling ends up being.  The warning should be there for rows/rowgroups, but not for cells.

> 7) Everything else aside, a warning about a changed feature should have an expiry date
> or release version set.

Possibly.  How about "when other UAs also support it"?

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The test before issuing the warning should be using IS_TABLE_CELL() rather than checking for only the separated-borders half of it.

(In reply to Boris Zbarsky [:bz] from comment #3)
> > It's not necessary to actually change the position of a TD to get the warning
> 
> It's not necessary to change the position of a relpos element to affect
> layout, since just having it be relpos it affects the positioning of abspos
> descendants.

... though it might have been that other browsers support this aspect (containing block for abs pos) even when they're not moving it, in which case it might make sense to condition the warning on that.

> > 7) Everything else aside, a warning about a changed feature should have an expiry date
> > or release version set.
> 
> Possibly.  How about "when other UAs also support it"?

Perhaps, although I'd use the term "browsers" rather than "UAs".  Not sure it's really needed, though.  I think the main thing is getting the right conditions.

Comment 5

[Seth Fowler [:seth] [:s2h]]

So I agree with Boris's comments above, but I'll add what I can.

In general the bottom line is that we're showing this warning when we shouldn't. I'll upload a patch that makes us use IS_TABLE_CELL(), as David recommends.

(In reply to Boris Zbarsky [:bz] from comment #3)
> > 7) Everything else aside, a warning about a changed feature should have an expiry date
> > or release version set.
> 
> Possibly.  How about "when other UAs also support it"?

Yeah, that's the problem with a fixed expiration for this warning. I don't think we necessarily need to wait for other UAs to implement this feature, but we probably do need to wait until it becomes reasonably well-known that this is something developers need to pay attention to. I think we should leave it in for a year at a minimum, and then reevaluate.

It would make me feel more comfortable removing it if I knew that there was good documentation of the change that was easy for developers to find. I just checked MDN and I see that it's out of date on this issue; as of this writing, this page still explicitly says that bug 63895 has not been fixed:

https://developer.mozilla.org/en-US/docs/Web/CSS/position#Relative_positioning

I'll update MDN, which moves us in the right direction as far as getting the word out.

I've thought about rewording the warning, but I haven't been able to come up with a different wording that I think adds much value. (And I'm hesitant to explicitly state in the warning that other UAs don't support this, because it seems like the sort of thing that could easily become out-of-date without anyone noticing.) I'm open to suggestions, though.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #4)
> ... though it might have been that other browsers support this aspect
> (containing block for abs pos) even when they're not moving it, in which
> case it might make sense to condition the warning on that.

Yeah, I couldn't remember whether this was true, so I checked WebKit and Blink. They don't support treating table parts other than table cells as abs pos containing blocks. In other words, they don't support the behavior we're warning about here.

Given that, it seems that we should display the warning unconditionally when a non-cell table part is relatively positioned.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Check for table cells correctly when warning about positioned table parts

This patch makes us correctly not warn for the testcase in this bug.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8504310 [details] [diff] [review]
Check for table cells correctly when warning about positioned table parts

r=me

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be7879798a2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3be7879798a2

Comment 11

[xiaofuse]

reporting that i encountered this warning when using position sticky on thead, is this intended?

Comment 12

[Tobia]

I just encountered this warning when applying position: relative to a TR (which works) in order to offset absolute children wrt. the TR (which works.)
So Firefox (80.0.1 Linux) is basically telling me: "Heads up! What you are doing actually works! Are you sure you want it to work?"
I don't understand why this warning exists.

Comment 13

[chrisboringlife]

(In reply to fusekai from comment #11)

reporting that i encountered this warning when using position sticky on thead, is this intended?

I can confirm with fusekai
thead { position: -webkit-sticky; position: sticky; }
The above CSS is great to have the thead stay on top of the div when scrolling down a table.
But it still returns the 'alert' in the console, and I think it really should be removed, as it holds no merit.