Closed
Bug 1257938
Opened 9 years ago
Closed 9 years ago
Remove CSS pref "layout.css.sticky.enabled"
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(3 files)
(deleted),
patch
|
coyotebush
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
coyotebush
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
coyotebush
:
review+
|
Details | Diff | Splinter Review |
We've been shipping position:sticky support enabled by default since mid-2014 (Firefox 32), as of bug 916315. Seems unlikely that we'd opt to roll back support at this point, so we should remove the pref and the code that reacts to it.
Assignee | ||
Comment 1•9 years ago
|
||
(I'll have patches ready for this shortly; taking.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Corey, since you CC'd yourself, maybe you'd be up for reviewing? :) If not, I'm happy to tag heycam or someone else. This first patch just makes our automated tests assume that position:sticky is unconditionally supported (dropping pref-enabling annotations). I've also dropped the reftest that checks whether the pref works. (Note: As hinted by an XXXdholbert comment in this patch, my next patch will merge test_position_sticky.html together with its iframe helper-file, since the separation there is no longer needed.)
Attachment #8732363 -
Flags: review?(corey)
Assignee | ||
Comment 3•9 years ago
|
||
This is basically just cut-n-paste from file_position_sticky.html into test_position_sticky.html. I've removed the "SimpleTest.waitForExplicitFinish()", too -- that was only there because the pref-setting & the iframe load were asynchronous. Now, everything happens synchronously, so we don't need waitForExplicitFinish() anymore.
Attachment #8732365 -
Flags: review?(corey)
Assignee | ||
Comment 4•9 years ago
|
||
This last patch actually removes the pref from the code. After this patch, grep tells me there are no remaining mentions of "layout.css.sticky.enabled" in the codebase.
Attachment #8732367 -
Flags: review?(corey)
Assignee | ||
Comment 5•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b89ad7a5316
Comment 6•9 years ago
|
||
This all seems fine to me, and Try was okay, but I'll take a closer look after the weekend.
Comment 8•9 years ago
|
||
Comment on attachment 8732363 [details] [diff] [review] part 1: Adjust automated tests to assume position:sticky is unconditionally supported Review of attachment 8732363 [details] [diff] [review]: ----------------------------------------------------------------- r=corey
Attachment #8732363 -
Flags: review?(corey) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8732365 [details] [diff] [review] part 2: Remove separation between test_position_sticky.html & its helper-file, now that it doesn't need to tweak a pref. It took me a bit to remember what this test is doing and why it's a mochitest. Maybe update the comment as long as we're here to clarify this, something roughly like /** Test for Bug 886646 - Offsets for sticky positioning, when accessed through getComputedStyle(), should be accurately computed. In particular, percentage offsets should be computed in terms of the scroll container's content box. */ r=corey anyway.
Attachment #8732365 -
Flags: review?(corey) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8732367 [details] [diff] [review] part 3: Remove support for the "layout.css.sticky.enabled" pref (so we'll unconditionally support "position: sticky") r=corey
Attachment #8732367 -
Flags: review?(corey) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Thanks! I made that comment-tweak in part 2, and pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad57442dddb6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a76f6237641f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8af3fc0676
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite-
Updated•9 years ago
|
Keywords: dev-doc-complete
Comment 12•9 years ago
|
||
Oups wrong keyword, sorry for the spam. This consists mostly in checking we don't speak about this pref on MDN.
Keywords: dev-doc-complete → dev-doc-needed
Assignee | ||
Comment 13•9 years ago
|
||
I don't think there's any documentation needed here; it'd just add noise to MDN. Our behavior isn't changing; we're just removing the ability to disable this feature. The page where this feature (sticky positioning) is documented... https://developer.mozilla.org/en-US/docs/Web/CSS/position ...*does* mention the pref ("layout.css.sticky.enabled is"), but only in a footnote, regarding the period of time where it was enabled in Nightly/Aurora but not in release. --> Removing dev-doc-needed. (If you disagree, please elaborate a bit about what you think should be documented & where.) Thanks!
Flags: needinfo?(jypenator)
Keywords: dev-doc-needed
Comment 14•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad57442dddb6 https://hg.mozilla.org/mozilla-central/rev/a76f6237641f https://hg.mozilla.org/mozilla-central/rev/ba8af3fc0676
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Flags: needinfo?(jypenator)
You need to log in
before you can comment on or make changes to this bug.
Description
•