Closed
Bug 925259
Opened 11 years ago
Closed 11 years ago
<thead> with position: sticky gets stuck after scrolling down
Categories
(Core :: Layout: Positioned, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: mxn, Assigned: coyotebush)
References
Details
Attachments
(2 files, 2 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
coyotebush
:
review+
|
Details | Diff | Splinter Review |
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.
|
Reporter | |
Updated•11 years ago
|
Summary: position sticky thead → <thead> with position: sticky gets stuck after scrolling down
|
|
Reporter | |
Updated•11 years ago
|
Component: Untriaged → Layout: R & A Pos
Product: Firefox → Core
|
Assignee | |
Comment 1•11 years ago
|
||
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.)
|
|
Reporter | |
Comment 2•11 years ago
|
||
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>.
|
|
Assignee | |
Comment 3•11 years ago
|
||
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
|
|
Assignee | |
Comment 4•11 years ago
|
||
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...
|
|
Assignee | |
Comment 5•11 years ago
|
||
> 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
|
|
Assignee | |
Comment 6•11 years ago
|
||
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)
|
|
Assignee | |
Comment 7•11 years ago
|
||
|
||
Comment 8•11 years ago
|
||
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+
|
|
Assignee | |
Comment 9•11 years ago
|
||
(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.
|
|
Assignee | |
Updated•11 years ago
|
Attachment #829894 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•11 years ago
|
Attachment #831027 -
Flags: review+
|
|
Assignee | |
Updated•11 years ago
|
Keywords: checkin-needed
|
|
||
Comment 10•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
|
||
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in
before you can comment on or make changes to this bug.
Description
•