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

[scroll-animations-1] Something is odd with the handling of source in the ScrollTimeline constructor #5202

Closed
birtles opened this issue Jun 12, 2020 · 5 comments
Assignees
Labels

Comments

@birtles
Copy link
Contributor

birtles commented Jun 12, 2020

The ScrollTimeline constructor text has the following:

Creates a new ScrollTimeline object using the following procedure:

  1. Let timeline be a new ScrollTimeline object.

  2. Let source be the result corresponding to the first matching condition from below.
    If the source value of options is non-null,

    • Let source be source

    Otherwise (source is null):

    • Let source be the scrollingElement of the Document associated with the Window that is the current global object.

    Note: source may still be null after this step, e.g. if the Document has no scrollingElement.

  3. Set the source of timeline to source.

Which is odd because source is not nullable in the ScrollTimeline spec. So how can we assign source if it is null?

Also, "Let source be source" is odd. Clearly it means the source member of options but it needs to say that.

@birtles birtles added the scroll-animations-1 Current Work label Jun 12, 2020
@birtles
Copy link
Contributor Author

birtles commented Jun 12, 2020

If we decide that a source can be null then procedures like the one to resolve a container-based offset will need to be updated to say what to do in that case.

@birtles
Copy link
Contributor Author

birtles commented Jun 12, 2020

I notice that the procedure to resolve an element-based offset handles a null source so I suspect the IDL and "resolve a container-based offset" need to be updated (we should also check the other algorithms too, however).

@majido
Copy link
Contributor

majido commented Jun 14, 2020

I guess the idea is that if the ScrollTimelineOptions may have a null source (which is the default value). And if that is given as input we resolve it to root scroller. So constructed timeline will never have a null source.

However thinking about this more, I wonder if we should allow a three state in our handling:

  • Unspecified: Use document scroller e.g., new ScrollTimeline({}); /*or*/ new ScrollTimeline()
  • Explicitly set to null: set source to to null e.g., new ScrollTimeline({source: null})
  • Explicitly set to element: set source to element e.g., new ScrollTimeline({source: $div})

This is similar to how timeline is handler on Animation constructor.
I can make this change and clear up the wording mistake as well.

@majido majido self-assigned this Jun 14, 2020
@birtles
Copy link
Contributor Author

birtles commented Jun 16, 2020

I guess the idea is that if the ScrollTimelineOptions may have a null source (which is the default value). And if that is given as input we resolve it to root scroller. So constructed timeline will never have a null source.

But the spec explicitly says that the Document can have no scrollingElement and hence source can be null?

However thinking about this more, I wonder if we should allow a three state in our handling:

  • Unspecified: Use document scroller e.g., new ScrollTimeline({}); /*or*/ new ScrollTimeline()
  • Explicitly set to null: set source to to null e.g., new ScrollTimeline({source: null})
  • Explicitly set to element: set source to element e.g., new ScrollTimeline({source: $div})

That would solve this issue (once we also define the behavior when source is null, e.g. in the "resolve a container-based offset" algorithm). I'm not sure if it's useful to be able to do that, but maybe it's fine.

majido added a commit that referenced this issue Jun 26, 2020
…5263)

Fixing #5202 and #5211

Changes:
- Fix IDL to make source nullable.
- In CSS source will accept 'auto' and 'none' with none mapping to null.
- In JS, we now map missing 'source' to document scrolling element (i.e., auto behavior) and explicitly null 'source' to null.
- Correctly check null source when calculating offsets.

Other minor cleanups.
@majido
Copy link
Contributor

majido commented Jun 30, 2020

Spec is fixed. I have filed a bug to fix Chromium implementation.

@majido majido closed this as completed Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants