Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-scroll-snap] scroll-snap-align values assigned backwards #2232

Closed
fantasai opened this issue Jan 28, 2018 · 6 comments
Closed

[css-scroll-snap] scroll-snap-align values assigned backwards #2232

fantasai opened this issue Jan 28, 2018 · 6 comments

Comments

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jan 28, 2018

We decided that the first keyword should affect the block axis, the second the inline axis: and defined it correctly for the place-* shorthands of the alignment properties, but seem to have specified it backwards for css-scroll-snap. :(

@majido
Copy link
Contributor

@majido majido commented Jun 19, 2018

Filed Chromium issue to fix this before we ship. However Safari is shipping the scroll-snap-align with inline/block order so there is interop risk here.

Is there any indication that they will switch as well? otherwise we need to evaluate the interop risk here.

Loading

@majido
Copy link
Contributor

@majido majido commented Jun 25, 2018

Originally posted here.

I ran a query against httparchive database to better understand the compat risk. My exact query is below if anyone wants to replicate [1].

Only 4 were using the scroll-snap-align with two arguments. This is out of 450k pages so usage and breakage is very low AFAICT. This gives me more confidence to make the switch. We should ensure Safari is willing to make the switch to avoid interop issues.

Here is the result:

bodies_page match bodies_url
http://www.scrimba.com/ scroll-snap-align:center none; https://scrimba.com/assets/cached-client-fab8900c04b0f8f240d73f6c6a561770.css
http://www.participate.com/ scroll-snap-align: 0 50px; https://www.participate.com/webpack/style-fe837b4781471be76fc4b0b20871f197.css
http://www.zikinf.com/ scroll-snap-align:center none; https://www.zikinf.com//_css/default/wk/annonces.1513606238.css
http://www.chuzefitness.com/ scroll-snap-align: none start; https://chuzefitness.com/wp-content/themes/chuze-reflex/css/theme.css?ver=1.5.5

Note that one of the cases is actually using it incorrectly.

[1] BigQuery query:

SELECT bodies.page, REGEXP_EXTRACT(bodies.body, r'(scroll-snap-align:\s*\w+?\s+\w+?\s*;)') as match, bodies.url, 
  FROM [httparchive:har.2018_02_01_chrome_requests_bodies]
  AS bodies
JOIN (
  SELECT 
    page, url,
    JSON_EXTRACT(payload, '$._contentType') AS contentType
  FROM [httparchive:har.2018_02_01_chrome_requests]
) AS requests ON requests.url=bodies.url AND requests.page=bodies.page
WHERE (requests.contentType CONTAINS 'html' OR requests.contentType CONTAINS 'css')
AND REGEXP_MATCH(body, r'(scroll-snap-align:\s*\w+?\s+\w+?\s*;)')
LIMIT 10000

Loading

@majido
Copy link
Contributor

@majido majido commented Jun 25, 2018

@smfr do you have any opinion on this change one way or another? It seems a safe change to me so I plan to match the spec in Chromium implementation.

Loading

@fantasai
Copy link
Collaborator Author

@fantasai fantasai commented Aug 3, 2018

Loading

@majido
Copy link
Contributor

@majido majido commented Nov 20, 2018

FWIW, I filed a webkit bug since Safari is still shipping the old ordering:

Loading

@rachelandrew
Copy link
Contributor

@rachelandrew rachelandrew commented Nov 21, 2018

I've also added a note to the MDN docs for scroll-snap-align

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants