Remove CSS pref "layout.css.sticky.enabled"

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla48
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
(I'll have patches ready for this shortly; taking.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8732363 [details] [diff] [review]
part 1: Adjust automated tests to assume position:sticky is unconditionally supported

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

2 years ago
Created 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.

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

2 years ago
Created attachment 8732367 [details] [diff] [review]
part 3: Remove support for the "layout.css.sticky.enabled" pref (so we'll unconditionally support "position: sticky")

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

2 years ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b89ad7a5316
This all seems fine to me, and Try was okay, but I'll take a closer look after the weekend.
(Assignee)

Comment 7

2 years ago
Sounds good, thanks!
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 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 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

2 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

2 years ago
Flags: in-testsuite-
Keywords: dev-doc-complete
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

2 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

2 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
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(jypenator)
You need to log in before you can comment on or make changes to this bug.