Closed Bug 1312163 Opened 7 years ago Closed 5 years ago

Update 'scroll-snap-type' to the latest specification and drop support for 'scroll-snap-type-x' and 'scroll-snap-type-y'

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Version:
52 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox68 --- fixed

People

(Reporter: sebo, Assigned: hiro)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, DevAdvocacy, site-compat, Whiteboard: [DevRel:P2][wptsync upstream])

Attachments

(2 files, 1 obsolete file)

Bug 1312163 - Rename ScrollSnapType to ScrollSnapStrictness. r?emilio!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1312163 - Switch to the new scroll-snap-type syntax for the old scroll snap implementation and drop the scroll-snap-type-{x,y} longhands. r?emilio!
47 bytes, text/x-phabricator-request
Details | Review 
The specification of the 'scroll-snap-type' CSS property changed lately and its syntax looks now like this:

none | [ x | y | block | inline | both ] [ mandatory | proximity ]?

Because it integrates values for the scroll snap axis, 'scroll-snap-type-x' and 'scroll-snap-type-y' should be dropped accordingly.

Sebastian
Keywords: dev-doc-needed
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
Too late for firefox 52, mass-wontfix.
status-firefox52: affected → wontfix
Blocks: css3test
Priority: -- → P3
Could I ask why this was marked as P3? Chrome is actively working on it. Safari shipped in 11.
This has now landed in Chrome behind a flag.
This is now supported in Chrome and Safari.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED

Hi bz, I have a couple of questions about dom bindings for CSS2Properties.

The new scroll-snap-type is a longhand property whereas in our current implementation it's a shorthand. To migrate this change smoothly, I'd like to implement a machinery to switch them by a single preference value, i.e. the new longhand property is enabled by the pref and the old shorthand property is disabled by the pref.

Does this way sound reasonable?

I did actually try the way and it seems to work as expected, but still there is one thing I don't quite understand, it's jsid. Do we need to generate different jsid for them respectively? In my try I did disable an assertion to check the id conflict.

FWIW, here is a try and each entries in CSS2Properties.webidl are like this;

  [CEReactions, Throws, TreatNullAs=EmptyString, SetterNeedsSubjectPrincipal=NonSystem, Pref="layout.css.scroll-snap-v1.enabled", BindingAlias="scroll-snap-type"] attribute DOMString scrollSnapType;
  [CEReactions, Throws, TreatNullAs=EmptyString, SetterNeedsSubjectPrincipal=NonSystem, Pref="layout.css.scroll-snap-v1.enabled", DisabledByPref, BinaryName="ObsoleteScrollSnapType", BindingAlias="scroll-snap-type"] attribute DOMString scrollSnapType;
Attachment #9044031 - Flags: feedback?(bzbarsky)
Comment on attachment 9044031 [details] [diff] [review]
Introduce disabled-by-pref option

Review of attachment 9044031 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/properties/properties.mako.rs
@@ +1764,5 @@
> +            duplicated_names = []
> +            same_name_properties = {}
> +            for property in data.longhands + data.shorthands:
> +                if property.disabled_by_pref:
> +                    obsolete_properties.append(property)

I would prefer to just do the change than adding all this code.

This is very complex for very little value. Switching it to be a longhand without changing the underlying implementation should be very low risk, as long as we keep serializing it from getComputedStyle(). See the overflow shorthand for an example of that.

Ok, I think I misread that, this is going in the other direction (from shorthand to longhand)...

I think the syntax change for scroll-snap-type shouldn't be blocking the rest of implementation work, and shouldn't give us a big headache, so probably should be done in a separate bug ASAP. I discussed a bit with Hiro in:

https://mozilla.logbot.info/layout/20190215#c15970047

He wanted to know what Boris thought about it, so dropping the link there so he can hopefully take a look.

Ok, so since there was a bit more confusion about this later on I thought I'll try to summarize the options as I see it.

Do the syntax change for the property separately from the implementation of the new features.

This would mean:

  • Dropping support for scroll-snap-type-x / scroll-snap-type-y.
  • Convert scroll-snap-type to a longhand.
  • Implement the new syntax that other browsers support for scroll-snap-type (dropping the current syntax).

I think this is the least risky. It keeps working all code that right now uses both the old and new syntax (AFAICT), and I don't see any downsides per the IRC discussion.

Hiro thought this meant ensuring all his work is done on the same release, but I don't think why that's true? This change wouldn't put us in a worse situation than today, right? But maybe there is some downside I have missed?

Try to unship scroll-snap-type-x / scroll-snap-type-y, but keep the old syntax for scroll-snap-type.

This would mean:

  • Dropping support for scroll-snap-type-x / scroll-snap-type-y.
  • Convert scroll-snap-type to a longhand.
  • Keep the old syntax of scroll-snap-type.
  • Implement the new syntax behind a pref.

I think this could break sites that use stuff like:

scroll-snap-type-x: proximity;
scroll-snap-type: x proximity;

Since we'll stop understanding the first line, but we won't start understanding the second one unless the pref is toggled.

Adding machinery to get scroll-snap-type to be both a shorthand and a longhand at the same time (based on a pref).

I'd rather don't, there are going to be bugs for sure, and it seems really complex for a hopefully very-short-term state of affairs, given we want to remove support for the old syntax ASAP.

Hiro, did I miss something? I might, it's pretty late here.

Thanks Emilio for the summary. The latter looks less risky to me because we can keep the current syntax for a while even if it's considered as a longhand. The only thing that will break something (what I can think of) is devtools uses the information whether the given property name is shorthand or longhand. But I think it's not so often used there.

Can we keep scroll-snap-type-x and scroll-snap-type-y along with the old scroll-snap-type as a longhand? It seems to me it's the least risky way.

Anyway, I started with this scroll-snap-type change, but it would be nice to work other bugs (scroll-snap-align, scroll-padding, scroll-margin) prior to this (scroll-snap-stop could be deferred since it's marked at risk) even if they don't work without the new scroll-snap-type as visually, but they can be parsed and serialized.

Thanks Emilio for the summary. The latter looks less risky to me because we can keep the current syntax for a while even if it's considered as a longhand. The only thing that will break something (what I can think of) is devtools uses the information whether the given property name is shorthand or longhand. But I think it's not so often used there.

Well, you drop support for the existing longhands, that doesn't strike me as less risky.

Can we keep scroll-snap-type-x and scroll-snap-type-y along with the old scroll-snap-type as a longhand? It seems to me it's the least risky way.

No, not without breaking how the cascade works for these properties.

Comment on attachment 9044031 [details] [diff] [review]
Introduce disabled-by-pref option

I would _really_ raather not introduce all this codegen complexity.

Especially since the actual web-observable behavior is that depending on the pref ... nothing changes in terms of the binding bits.  There's still an attribute named "scrollSnapType" and an attribute named "scroll-snap-type".  You can get them and set them.  The actual parsing differs, and we're implementing that under the hood via the difference between SetObsoleteScrollSnapType and SetScrollSnapType, but we could just as easily have SetScrollSnapType check the pref, no?  Or is that hard to do with the macro bits we use to implement things like SetScrollSnapType?
Attachment #9044031 - Flags: feedback?(bzbarsky) → feedback-

Another possible approach on top of the ones in comment 10 is keep it as a shorthand, but with the new longhand syntax instead if the pref is set. But that's observable as well from the spec, and I don't think it's feasible to implement the logical values.

Oh, and the first option from comment 9 seems sanest to me...

Thank you, bz. Yes we can check another new preference in SetScrollSnapType. But, yeah, both of you doesn't prefer the way, I will not go with it.

What I am concerned about the first option in comment 9 is I found some which using scroll-snap-type-y (or x) solely.

I will probably go with a way to switch it on build time.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

What I am concerned about the first option in comment 9 is I found some which using scroll-snap-type-y (or x) solely.

That doesn't work in Chrome or any other browser already, right? And will break with your patches anyhow. If so I think we should just adapt to the new spec.

Blocks: 1373835
Blocks: 1373832
Depends on: 1529420

I've decided to go with the first approach what Emilio suggested in comment 9. Also, I'd like to avoid the intermediate state (the new scroll-snap-type syntax applies to the old implementation) as much as possible, so I'd like to land all relevant stuff (bug 1373835, bug 1373832, bug 1373833 and bug 1531228) at the same time which means we switch the new scroll snap v1 at that time.

Anyway, I think now it's time to review all relevant stuff. FWIW, here is a try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=39ea56261d15c9d3a039bc3d52b64488b5e6c821 . There are oranges in wpt, but it's a fault in the test itself. I've already sent a PR for that but it hasn't got merged yet. (Probably I will ask Botond to review it in bug 1373832).

Note that I am going to send an 'intent to implement/ship and unship' mail when all reviews get closer to finish.

Attachment #9044031 - Attachment is obsolete: true

The scroll snap strictness is defined in the new spec [1], and the structure
is the exactly same as the old scroll snap type structure.

[1] https://drafts.csswg.org/css-scroll-snap-1/#snap-strictness

I am hold landing these patches until bug 1373835, bug 1373832 and bug 1373833 get ready to be landed.

Keywords: site-compat
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23d3b9b92a77
Rename ScrollSnapType to ScrollSnapStrictness. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1703fb877b13
Switch to the new scroll-snap-type syntax for the old scroll snap implementation and drop the scroll-snap-type-{x,y} longhands. r=emilio

https://hg.mozilla.org/mozilla-central/rev/23d3b9b92a77
https://hg.mozilla.org/mozilla-central/rev/1703fb877b13

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16506 for changes under testing/web-platform/tests
Whiteboard: [DevRel:P2] → [DevRel:P2][wptsync upstream]

Scroll Snap is documented https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Scroll_Snap and a note added to the 68 release notes.

I have also been through and made sure anything regarding the old implementation is flagged up as being deprecated or removed as appropriate.

You need to log in before you can comment on or make changes to this bug.