Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Scroll range of scroll timeline should be inclusive of endScrollOffset #19

Closed
majido opened this issue Sep 26, 2018 · 24 comments
Closed

Comments

@majido
Copy link
Member

majido commented Sep 26, 2018

The spec currently makes it so that the end offset produces unresolved time. See current time algorithm.

If current scroll offset is greater than or equal to endScrollOffset, return an unresolved time value if fill is none or backwards, or the effective time range otherwise.

This seems like an non-intuitive choice to me. In particular, if the user doesn't specify start, and end values then the interval becomes the whole scroll range. With current specified behavior at the max scroll offset the effect will become in-active unless it has a fill forwards value.

Also AFAICT web-animation active interval is inclusive of both start and end. So it feels like ScrollTimeline behavior should be altered to match that.

@birtles
Copy link
Collaborator

birtles commented Sep 27, 2018

Also AFAICT web-animation active interval is inclusive of both start and end.

It only includes the either start or end (depending on the sign of the playback rate), not both.

@stephenmcgruer
Copy link
Collaborator

stephenmcgruer commented Nov 20, 2018

This issue was discussed today in the regular Animations sync. Present: @birtles @graouts @majido @stephenmcgruer . All participants agreed that this was an issue and should be fixed, but there was disagreement on the best approach. We resolved for the discussed approaches to be documented here and we will try to work through them on this Github issue.

Sequencing
It was raised during the meeting that the main reason for this behavior was to enable sequencing; e.g. having a sequence of animations for the same scroller, each with a ScrollTimeline whose startScrollOffset starts at the previous ScrollTimeline's endScrollOffset. The goal is that the transition from one animation to the next would be seamless.

Post-meeting question from @flackr - would it necessarily be a problem if the two animations specified in such a case overlapped?

Proposed Solutions

Special-case auto endScrollOffset
Brian proposed giving endScrollOffset == 'auto' special behavior, such that the currentTime algorithm would change to read something like:

If endScrollOffset is not 'auto' and current scroll offset is greater than or equal to endScrollOffset, return an unresolved time value if fill is none or backwards, or the effective time range otherwise.

The obvious pro is that it requires no change from authors and fixes the common 'scroll to the end of your scroller' behavior. However there was concern that it makes the spec more confusing and we should avoid special cases.

Author-determined inclusivity
Majid proposed allowing the web author to choose whether the endScrollOffset would be inclusive or not. Borrowing some syntax from web audio this would allow authors to write things like:

endScrollOffset: '100px'  // implicitly inclusive (or exclusive, depending on which we default to)
endScrollOffset: { inclusive: false, value: '100px' }

Here there are no special cases, but it requires web authors to make a choice (or worse - the default could surprise them).

@birtles
Copy link
Collaborator

birtles commented Nov 21, 2018

If we have any implicit behavior, it should be endpoint-exclusive.

That said I still think having 'auto' (or some other keyword) provide the inclusive behavior is simpler and we should probably prefer that in the absence of compelling use cases for using inclusive endpoints elsewhere. If we later need inclusivity for other endpoints then 'auto' simply becomes an alias for { value: '100%', inclusive: true }.

Re: whether overlap is actually a problem, I can imagine it is if you have multiple animations targeting the same element that are supposed to handover at some point.

@stephenmcgruer
Copy link
Collaborator

Majid and I were discussing this today and came up a way of doing the special 'auto' approach that I believe Majid is happy with (and hopefully others will be too!). The idea:

Do not special-case auto in the currentTime algorithm. Instead, modify the endScrollOffset definition such that auto is defined as the first scroll value past the end of "scrollSource's scroll range in orientation". Then the existing currentTime algorithm works as desired (since current scroll offset cannot be >= endScrollOffset in that case).

The downside is awkward wording in endScrollOffset (we will probably want to include a Note to highlight/explain the implication for implementors), but the upside is avoiding an additional confusing branch in the current time algorithm.

Thoughts?

@birtles
Copy link
Collaborator

birtles commented Nov 26, 2018

As far as I can tell, in practice this is the same as what I was proposing so it sounds good to me (and makes it possible to introduce an inclusive endpoint feature in future if needed).

@stephenmcgruer
Copy link
Collaborator

I was musing on this this morning and realized that there is another issue with the approach Majid and I proposed. It makes it difficult to reason about the currentTime at a given scroll offset - partially because of potential implementor differences, and partially because it is just a bit weird.

Consider two implementations:

Implementation A - has the ability to represent scroll offsets in increments of 1
Implementation B - has the ability to represent scroll offsets in increments of 0.5

Now consider a scroller with a scrollable size of 100 units (e.g. scrollHeight - clientHeight == 100) and a block ScrollTimeline for the scroller with timeRange = 100.

With the old approach and this setup, currentTime maps directly to scroll offset, e.g.:

scroller.scrollTop = 50;  // currentTime now 50
scroller.scrollTop = 90;  // currentTime now 90

With the new approach, currentTime no longer maps to scroll offset, and maps differently based on implementation.

scroller.scrollTop = 50;
// Implementation A: currentTime is now (50 - 0) / (101 - 0) * 100 = 49.50
// Implementation B: currentTime is now (50 - 0) / (100.5 - 0) * 100 = 49.75

Maybe this doesn't matter in practice, I'm not sure. It will make the tests a bit trickier to write (but we do already have some that set a massive timeRange to avoid rounding issues, we could just do that again). It is enough of an issue to make me lean back towards 'special behavior for auto'!

@flackr
Copy link
Collaborator

flackr commented Nov 27, 2018

I completely agree that we want to make the above case just work. When we initially talked about this I just assumed that by the first scroll value past the end you meant something like the smallest next floating point value (e.g. 100.000000001) such that in practice it would essentially produce a 100% time output. Implementers could probably even implement it as inclusive - though at this point perhaps it's better to just let auto have a special value.

@majido
Copy link
Member Author

majido commented Nov 27, 2018

@stephenmcgruer I see the potential pitfall that this can cause.

Taking a step back, I am not actually convinced that that our implicit behavior should be end-point exclusive. I would argue that there is compelling case for end-point inclusive (avoiding fill:forwards) but a weak usecase for end-point exclusive (allowing sequencing). So I suggests we make the implicit behavior of timeline to be end-point inclusive and avoid special casing 'auto'.

Note that here we are talking about sequencing timelines which is different from sequencing animations/effects.

As for the sequencing usecase, here are some considerations:

  1. AFAICT Sequencing across arbitrary animations is currently not supported in web-animations. Correct me if I am wrong but current plan is that it will only works for effects and then only within a specific context (e.g., SequentialGroupEffect). So I don't think we should consider arbitrary sequencing an important design consideration for Timelines.

  2. Sequencing multiple animations using a single scroll timeline should work as expected with other web-animations independent of ScrollTimline behavior. I believe this will be most likely usage pattern.

  3. Sequencing multiple timelines should be rare but event then there are two cases here:

    • The hand-off is seamless to avoid a jump. Seamless means that the end value of previous animation is the same as the start value of next animation. So if the animations are animating the same element/property then it should not not matter if end-point is inclusive or exclusive because the values are expected to be the same.
    • The hand-off is not seamless so there is a jump. Here since there is a jump why do we care if one wins vs the other?
    • The only exception to above that I can think of is when one is animating two different targets. Here is the ordering may be important. In this case, a reasonable workaround is to have the animation authors simply order the creation of their animations to control which animation wins (this is pretty easy to reason about because the EffectStack ordering is well-determined).

To summarize, I think arbitrary sequencing is not really a well-supported case in web-animation, further usecases that require arbitrary timeline sequencing are rare and have a reasonable workaround. Additionally, if we want to support timeline sequencing then I suggest we introduce a GroupTimeline concept which provides a context in which we can alter the end-point exclusivity behavior similar to how it is done for animations.

@flackr
Copy link
Collaborator

flackr commented Nov 27, 2018

I agree that sequencing timelines is weird, we don't have this today because we only have infinite timelines. However, once we have finite timelines I'm not sure it's all that different than sequencing animations on a given timeline.

I have a couple thoughts/clarifications on your points:

  1. While not explicit, you can sequence arbitrary animations simply by lining up the end time of one animation with the start time of the next. Here's an example: https://jsbin.com/husoxum/edit?html,js,output

  2. I agree, using a single scroll timeline this will work fine. The concern is that developers will create a scrolltimeline with a specific range for each of their animations rather than sequencing them on the same scrolltimeline since this is much easier to specify.

  3. Another tricky case where the hand-off matters:

  • With the addition of composite: add even if the hand-off is seamless there will be one scroll position where both animations are in effect adding to the composite value.

I'm not sure I understand your case about animating two different targets. If they are different targets they have separate effect stacks don't they?

I think I agree with your conclusion though, I think sequencing scrolltimelines will just work because for each animation on each of the timelines they will sequence correctly over each scrolltimeline's range. We should just not consider a scrolltimeline inactive at the end scroll offset.

@stephenmcgruer
Copy link
Collaborator

I had intended to bring this up at the sync, but we cancelled it. @graouts @birtles - flackr, majid, and I are now on the same page and believe that the default behavior for endScrollOffset (auto or otherwise) should be end-point inclusive. This is based on the previous two comments above this one, or in summary:

Sequencing timelines is not sequencing animations. Even if the timelines overlap, the animations will sequence (if they're for the same target) or can be made to sequence (if they're for different timelines)

We are of course open to debate (perhaps at the next sync), but I would really like to at worst leave the January sync with a resolution here :)

@birtles
Copy link
Collaborator

birtles commented Jan 1, 2019

I fully agree with Rob, that once we have finite timelines, sequencing effects by lining up their animations is not fundamentally different from lining up their timelines.

I'd rather avoid breaking consistency with the rest of Web Animations (there must be nearly a dozen places where we use endpoint exclusive timing there including iteration boundaries, step timing functions etc.) and introducing unintended overlap situations (leading to jumpy behavior when composite modes are involved).

Perhaps the best avenue would be to draw a parallel with filling behavior where, when an effect fills at exactly an iteration boundary, you fill using the end of the iteration, not the start of the next.

@stephenmcgruer
Copy link
Collaborator

This was discussed at the Chrome/Firefox/Safari animation sync yesterday. (From memory, so excuse mistakes).

Brian re-iterated his position that jumpy behavior will happen, for example when anim1 has an endScrollOffset == 100px and anim2 has a startScrollOffset == 100px and a composite mode of 'add'. Assuming that issue #31 is taken care of, this does seem to hold; there will be a single pixel location when scrolling where both animations are in effect and their outputs will be composited. It is arguably bad web content, but Stephen/Majid largely yielded that it is feasible.

We then discussed a slight alternative to special casing auto; special casing endScrollOffset == max-scroll-position. This is slightly awkward wording, but would cover both auto as well as authors manually specifying (say) `endScrollOffset: '100%'. This seems like it may be a reasonable compromise; the next steps are for Stephen/Majid/Rob to mull this over and agree with it, or not :)

@flackr
Copy link
Collaborator

flackr commented Jan 18, 2019

I'm not a big fan of the discontinuity between say endScrollOffset: 99px (exclusive) and endScrollOffset: 100px (inclusive), however I think it would be fine.

I have one alternative to consider, which is if we go back to always exclusive, and we can even map auto to Infinity, but we also clamp the end offset in calculations to the end of the scroll range such that end value + n is in effect end value but inclusive.

For example, from above

scroller.scrollTop = 50;
// currentTime is now (Math.min(100, 50) - 0) / (Math.min(100, Infinity) - 0) * 100 = 50

As a side benefit, developers could specify Infinity whenever they want the entire scroll range without having to calculate it. :-)

@stephenmcgruer
Copy link
Collaborator

I need two confirmations/clarifications to make sure I understand the idea @flackr .

This would change the current time algorithm to say something (I'm sure wording could be better) like:

...
4. Let inclusiveEnd be True If endScrollOffset is greater than the maximum
   scroll range, False otherwise.
5. Let resolvedEndScrollOffset be min(endScrollOffset, maximum scroll range).
6. If current scroll offset is greater than endScrollOffset, return an unresolved
   time value if fill is none or backwards, or the effective time range otherwise.
7. If current scroll offset is equal to endScrollOffset and inclusiveEnd is False,
   return an unresolved time value if fill is none or backwards, or the effective
   time range otherwise.
...

Secondly, this provides the behavior where auto is inclusive, but if I set endScrollOffset to 100% I get exclusive behavior?

@flackr
Copy link
Collaborator

flackr commented Jan 23, 2019

Yes, though you don't even have to mention inclusive if you determine whether or not the time range is resolved based on endScrollOffset, and then determine the time value by capping it to the effective time range. i.e. step 4 can remain unchanged, and step 5+ change to:

  1. Let effective_start_scroll_offset = max(0, start), let effective_end_scroll_offset = min(end_scroll_offset, effective_time).
  2. Return the result (current scroll offset - effective_start_scroll_offset) / (effective_scroll_end_offset - startScrollOffset) × effective time range

@stephenmcgruer
Copy link
Collaborator

Ah yes, I see. OK that makes sense.

I am concerned about web developers doing endScrollOffset: '100%' and being surprised when it isn't inclusive. Thoughts from anyone? (On the issue that will never end ;).)

@flackr
Copy link
Collaborator

flackr commented Jan 23, 2019

Yeah that's a good question. I'm not sure whether it makes sense to special case resolving 100% to effectively Infinity or let developers be surprised that it isn't inclusive. Do we know that developers will try to do this and be surprised? My intuition would be to special case that later if it comes up as an issue if we don't have evidence to suggest it is already an issue.

@stephenmcgruer
Copy link
Collaborator

I think it is reasonable for us to leave '100%' as-is for now, and take the possible pain in the future if we have to change the behavior of '100%'.

As such, I propose we resolve this issue with: 'auto' for endScrollOffset will be considered a value of Infinity to make it endpoint-inclusive, adjusting the currentTime algorithm as explained here so that the results are still correct.

Any disagreement? @graouts @birtles @majido @flackr

@birtles
Copy link
Collaborator

birtles commented Jan 24, 2019

I am concerned about web developers doing endScrollOffset: '100%' and being surprised when it isn't inclusive. Thoughts from anyone?

I agree. I think it's more natural to have 100% be inclusive. That's what I would expect as an author. Having to specify 'auto' to get the inclusive behavior feels more magical to me.

It also has parallel with the behavior of animation iterations: if you have a filling animation and an iteration ends at exactly the end of the active interval, you fill with the end value, but if the iteration finishes just before the end of the active interval you don't.

@flackr
Copy link
Collaborator

flackr commented Jan 29, 2019

Yeah, I can definitely understand how that would be confusing. So we'll go with making the end offset inclusive if it's equal to the range? Do you think that this should include the pixel value of the scroll range? I worry a bit about precision errors if we allow floating point scrolling content lengths (i.e. an exact number of device pixels which is a fractional number of CSS pixels).

@stephenmcgruer
Copy link
Collaborator

Summarising the current proposals in advance of tomorrow's meeting:

Special case endScrollOffset == max-scroll-position to be inclusive.
Pros:
i. Works for all common cases: 'auto', '100%', etc

Cons:
i. Discontinuity between endScrollOffset: '99px' and endScrollOffset: '100px' (for a 100px max-scroll-position scroller)
ii. Spec wording a little awkward.
iii. Possible precision errors if we allow floating point scrolling content lengths (i.e. an exact number of device pixels which is a fractional number of CSS pixels).

Map auto to Infinity and clamp the end offset in calculations to the end of the scroll range.
Pros:
i. Simpler spec implementation.
ii. Developers can technically write endScrollOffset: Infinity and it fixes the output math (this may be another issue though, we can still clamp).

Cons:
i. endScrollOffset: '100%' doesn't work (nor does endScrollOffset: '100px' for a 100px max-scroll-position scroller)

Feel free to add pros/cons, but the goal is to discuss + pick one in tomorrow's meeting.

@birtles
Copy link
Collaborator

birtles commented Feb 5, 2019

Cons:
i. Discontinuity between endScrollOffset: '99px' and endScrollOffset: '100px' (for a 100px max-scroll-position scroller)

I don't think this is a problem--in fact I think it does what authors expect and mirrors what happens with iteration timing.

ii. Spec wording a little awkward.

This also seems like a non-issue.

iii. Possible precision errors if we allow floating point scrolling content lengths (i.e. an exact number of device pixels which is a fractional number of CSS pixels).

This is potentially problematic though, particularly if authors calculate lengths themselves and there are different tolerances used in different browsers, discrepancies that only show up on certain devices etc. I'm not sure how much of a problem it will be in practice, however.

@flackr
Copy link
Collaborator

flackr commented Feb 5, 2019

Can/should we differentiate '100%' from '100px' such that 100px does not get inclusive behavior but 100% does?

@stephenmcgruer
Copy link
Collaborator

This was discussed in todays Animation sync.

We resolved (!!) to go with 'special case endScrollOffset == max-scroll-position to be inclusive', including pixels and percentages. It was acknowledged that rounding errors might (in rare cases) make pixel values not operate as expected (because a value turns out to round to < max-scroll-position), but it was felt that this was the best way forward regardless. The recommendation for developers will be (as always): 'if you want endScrollOffset to be the end, use auto'.

I will start working on a PR for this.

stephenmcgruer added a commit that referenced this issue Feb 22, 2019
birtles pushed a commit that referenced this issue Feb 22, 2019
…oll-position (#37)

See the issue for more details

Fixes #19
' [ci skip]

Generated from:

commit 54383b7
Author: Stephen McGruer <stephen.mcgruer@gmail.com>
Date:   Fri Feb 22 15:26:32 2019 -0500

    Make endScrollOffset check inclusive for max-scroll-position (#37)

    See the issue for more details

    Fixes #19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants