925259 - <thead> with position: sticky gets stuck after scrolling down

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Minh Nguyễn]

Attachment: thead.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131007004003

Steps to reproduce:

1. Enable layout.css.sticky.enabled in about:config.
2. Open the attached webpage.
3. Scroll downwards, then upwards.


Actual results:

The <thead>, which has position: sticky applied to it, remains stuck somewhere in the middle of the table. If you scroll past the end of the table, the <thead> is constrained to the bottom of the table, but it stays there even after you scroll back up.


Expected results:

The <thead> should have stayed at the top of the viewport after you scrolled up.

Comment 1

[Corey Ford [:coyotebush]]

I'm actually surprised that this does anything at all, given bug 35168. (So it's possible the fix here should be to *make* it not do anything.)

Comment 2

[Minh Nguyễn]

Aw, that's unfortunate. Sticky <thead>s sounds like a good use case for position: sticky. It'd be very useful for long, complex tables, and it would seem to be the reason for having <thead> in the first place: <http://www.w3.org/TR/html401/struct/tables.html#h-11.2.3>.

Comment 3

[Corey Ford [:coyotebush]]

Absolutely. But I'm pretty sure bug 35168 is required for this to work as expected, so in the meantime it's probably better to do something more consistent.

In particular, I assume what's happening is that we never ApplyRelativePositioning on the <thead>, so the normal position is never saved and GetNormalPosition falls back to returning GetPosition every time.

Comment 4

[Corey Ford [:coyotebush]]

First I need to figure out why sticky positioning on <tr>s and <td>s doesn't do anything, given that StickyScrollContainer doesn't explicitly filter them out similarly to RecomputePosition...

Comment 5

[Corey Ford [:coyotebush]]

Attachment: test case demonstrating <thead>, <tr>, <td>

> sticky positioning on <tr>s and <td>s doesn't do anything

Er, never mind. <tr> and <td> exhibit exactly the same behavior. Not quite sure how I never noticed that.

So I'll probably want to factor out the bail-on-table-parts code from RestyleManager::RecomputePosition, then check that before adding frames to the StickyScrollContainer.

Comment 6

[Corey Ford [:coyotebush]]

Attachment: Avoid sticky positioning inner table elements.

Like so.

(I didn't think it was worth keeping the switch statement, but maybe it would still perform a bit better?)

Comment 7

[Corey Ford [:coyotebush]]

A try push: https://tbpl.mozilla.org/?tree=Try&rev=fc011f987b13

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 829894 [details] [diff] [review]
Avoid sticky positioning inner table elements.

> (I didn't think it was worth keeping the switch statement, but maybe it
> would still perform a bit better?)

I'm fine with the patch as-is (dropping the switch). It has the benefit of matching the style of the other nearby functions' coding-style (e.g. IsAbsolutelyPositionedStyle).

>diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp
>--- a/layout/base/RestyleManager.cpp
>+++ b/layout/base/RestyleManager.cpp
[...]
>   if (display->IsRelativelyPositionedStyle()) {
[...]
>+     if (display->IsInnerTableStyle()) {
>         // We don't currently support relative positioning of inner
>         // table elements.  If we apply offsets to things we haven't
>         // previously offset, we'll get confused.  So bail.
>         return true;

Nit: the new "if" check and its block is overly indented. (I think the "if" needs -1 space, and its body needs -2 spaces.)

(It's extra indented in current code because the "switch" statement added an extra layer of indentation.)

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
>+    // We don't currently support relative positioning of inner table
>+    // elements, so we exclude them from sticky positioning too.

Please mention bug 35168 in the comment here.

>+++ b/layout/reftests/position-sticky/inner-table-1.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html>
>+  <head>
>+    <title>CSS Test: Sticky Positioning - inner table elements</title>
>+    <link rel="author" title="Corey Ford" href="mailto:[e-mail]">
>+    <link rel="match" href="inner-table-1-ref.html">
>+    <meta name="assert" content="Sticky positioning on inner table elements should have no effect">

Maybe add "...until bug 35168 is fixed" to that last line there?

(Otherwise it sounds like an authoritative spec requirement sort of thing, when really it's just a limitation of our implementation.)

>+      <tr>
>+        <td class="sticky">c</tr>
                                ^
Nit: s/tr/td/--------------------^

r=me with that

Comment 9

[Corey Ford [:coyotebush]]

Attachment: Avoid sticky positioning inner table elements.

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Nit: the new "if" check and its block is overly indented. (I think the "if"
> needs -1 space, and its body needs -2 spaces.)

Good catch.

> Please mention bug 35168 in the comment here.

Added it to both of the code comments and the reftest.

Comment 10

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/083e800d6c03

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/083e800d6c03