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-1] Clarify that scroll-padding and scroll-snap-type on root propagate to viewport #3740

Closed
fantasai opened this issue Mar 19, 2019 · 4 comments
Labels
Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-scroll-snap-1 Current Work

Comments

@fantasai
Copy link
Collaborator

We propagate various properties that control overflow from the root to the viewport, e.g. overflow, scrollbar-width, etc. scroll-snap-type and scroll-padding aren't expected to be any different about this point, but it probably should be stated explicitly (as it is in css-scrollbars).

@fantasai
Copy link
Collaborator Author

Agenda+ to confirm substantive change, republish CR.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Clarify that scroll-padding and scroll-snap-type on root propagate to viewport, and agreed to the following:

  • RESOLVED: Accept the edits for https://github.com/w3c/csswg-drafts/issues/3740
The full IRC log of that discussion <dael> Topic: Clarify that scroll-padding and scroll-snap-type on root propagate to viewport
<dael> github: https://github.com//issues/3740
<dael> Rossen_: fantasai are you on?
<dael> fantasai: Yeah. I think it was assumed by all of us scroll snap prop applied to viewport but we don't say so in the spec. I copied overflow draft and scrollbar draft to say prop from root but not body element
<fantasai> https://github.com/w3c/csswg-drafts/commit/b5afa1086cf344046ea87013f0c48b3800eaacd7
<dael> fantasai: Edits ^
<dael> florian: The way overflow prop from body is weird. Not doing weird is good, but isn't consistent good so can set both on body and they prop together
<dael> fantasai: Could set both on root instead. Don't prop scrollbar width or color
<dael> AmeliaBR: I haven't looked at edits, is it clear wording? Author if overflow on the body does scrollbars I'd expect that's where I put scroll snap. Maybe educate authors how that's weird and problematic but it's an extra level of lack of intuition
<dael> Rossen_: Agree. If body prop background color and overflow prop back to viewport and canvas it would make sense to have scroll prop. But as fantasai pointed out we want to stop all that legacy quirk behavior expansion of properties.
<dael> Rossen_: Couple says forward- continue adding them b/c this is what authors expect. Or not do it and teach people by making explicit test cases and educational material and that they will face that it doesn't work, ideally find and article and see body stuff is a quirk there to keep legacy web working but don't do that
<dael> Rossen_: Kinda more leaning not add to body
<dael> Rossen_: I do agree make sense to be explicitly pointed out apply to viewport
<dael> AmeliaBR: Skimming edits it looks quite sensible. There is a note about not prop from body but I don't know if we need to make that more author focused
<dael> florian: I guess that's enough. fantasai mentioned this is also case for scrollbar width. If we keep new stuff consistent and none work on body we have a fighting chance of getting people to set on root. I'm okay with edit as is if we make that the policy we follow
<dael> Rossen_: Other opinions?
<dael> Rossen_: Sounds like we're okay with the edits where scrollsnap properties prop from root. Obj to accepting edits?
<dael> RESOLVED: Accept the edits for https://github.com//issues/3740

@hiikezoe
Copy link

@fantasai, if I understand correctly the agreement on the IRC log, Example 5 should use html instead of body? I am confused about the example.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 16, 2019
…ing value. r=botond

Now the spec cleary says that we don't need to propagate body's
scroll-padding value to the document viewport since
w3c/csswg-drafts#3740, so we don't need to care about
GetViewportScrollStylesOverrideElement() at all.

This change fixes the crash test case in this commit, but it's not sufficient.
In the next patch, we will fix another crash case.

Differential Revision: https://phabricator.services.mozilla.com/D27422

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Apr 16, 2019
…ing value. r=botond

Now the spec cleary says that we don't need to propagate body's
scroll-padding value to the document viewport since
w3c/csswg-drafts#3740, so we don't need to care about
GetViewportScrollStylesOverrideElement() at all.

This change fixes the crash test case in this commit, but it's not sufficient.
In the next patch, we will fix another crash case.

Differential Revision: https://phabricator.services.mozilla.com/D27422
@fantasai
Copy link
Collaborator Author

@hiikezoe Yes. Thanks. Fixed. :)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 19, 2019
Previously we propagated scroll snap properties [1] from viewport
defining element's style to the viewport. However this is not spec
compliant [1] and more importantly our behavior is different from
Gecko.

This patch fixes this issue by having these properties propagate
from document.documentElement instead.

TODO: better understand the backward compat impact.
Note that this can cause previously snapping viewport to no longer snap
if the web developer has declared the snap type on body as opposed to
document and body is viewport defining element

[1] scroll-snap-type, scroll-padding
[2] w3c/csswg-drafts#3740

BUG= 952711
TEST= wpt/css/css-scroll-snap/scroll-snap-type-on-root-element.html

Change-Id: I2d84095decb3af52f6a99c52a5a1a8d5c92fcf62
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 30, 2019
Previously we propagated scroll snap properties [1] from viewport
defining element's style to the viewport. However this is not spec
compliant [1] and more importantly our behavior is different from
Gecko.

This patch fixes this issue by having these properties propagate
from document.documentElement instead.


TODO: better understand the backward compat impact.
Note that this can cause previously snapping viewport to no longer snap
if the web developer has declared the snap type on body as opposed to
document and body is viewport defining element

[1] scroll-snap-type, scroll-padding
[2] w3c/csswg-drafts#3740

BUG= 952711
TEST= wpt/css/css-scroll-snap/scroll-snap-type-on-root-element.html


Change-Id: I2d84095decb3af52f6a99c52a5a1a8d5c92fcf62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1704859
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682363}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…ing value. r=botond

Now the spec cleary says that we don't need to propagate body's
scroll-padding value to the document viewport since
w3c/csswg-drafts#3740, so we don't need to care about
GetViewportScrollStylesOverrideElement() at all.

This change fixes the crash test case in this commit, but it's not sufficient.
In the next patch, we will fix another crash case.

Differential Revision: https://phabricator.services.mozilla.com/D27422

UltraBlame original commit: 9bf188c86f9c422d4f09b35268eed50d22495679
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…ing value. r=botond

Now the spec cleary says that we don't need to propagate body's
scroll-padding value to the document viewport since
w3c/csswg-drafts#3740, so we don't need to care about
GetViewportScrollStylesOverrideElement() at all.

This change fixes the crash test case in this commit, but it's not sufficient.
In the next patch, we will fix another crash case.

Differential Revision: https://phabricator.services.mozilla.com/D27422

UltraBlame original commit: 9bf188c86f9c422d4f09b35268eed50d22495679
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…ing value. r=botond

Now the spec cleary says that we don't need to propagate body's
scroll-padding value to the document viewport since
w3c/csswg-drafts#3740, so we don't need to care about
GetViewportScrollStylesOverrideElement() at all.

This change fixes the crash test case in this commit, but it's not sufficient.
In the next patch, we will fix another crash case.

Differential Revision: https://phabricator.services.mozilla.com/D27422

UltraBlame original commit: 9bf188c86f9c422d4f09b35268eed50d22495679
@fantasai fantasai added the Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. label Nov 12, 2020
fantasai added a commit to web-platform-tests/wpt that referenced this issue Jan 16, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 21, 2021
…viewport., a=testonly

Automatic update from web-platform-tests
[css-scroll-snap-1] Test propagation to viewport. w3c/csswg-drafts#3740

--

wpt-commits: 7b1a019883bdf1f6bf549cd205d9e6ce7f3e5e5d
wpt-pr: 27163
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 25, 2021
…viewport., a=testonly

Automatic update from web-platform-tests
[css-scroll-snap-1] Test propagation to viewport. w3c/csswg-drafts#3740

--

wpt-commits: 7b1a019883bdf1f6bf549cd205d9e6ce7f3e5e5d
wpt-pr: 27163

UltraBlame original commit: 359eb0d961345f405af354a3c14c5a69497eac07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-scroll-snap-1 Current Work
Projects
None yet
Development

No branches or pull requests

3 participants