Closed Bug 925259 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout: Positioned, defect)

26 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mxn, Assigned: coyotebush)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file thead.html (obsolete) —
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.
Summary: position sticky thead → <thead> with position: sticky gets stuck after scrolling down
Component: Untriaged → Layout: R & A Pos
Product: Firefox → Core
Blocks: 886646
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.)
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>.
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.
Assignee: nobody → corey
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
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...
> 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.
Attachment #815253 - Attachment is obsolete: true
Like so.

(I didn't think it was worth keeping the switch statement, but maybe it would still perform a bit better?)
Attachment #829894 - Flags: review?(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:corey@coreyford.name">
>+    <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
Attachment #829894 - Flags: review?(dholbert) → review+
(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.
Attachment #829894 - Attachment is obsolete: true
Attachment #831027 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/083e800d6c03
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.