Open Bug 1321428 Opened 8 years ago Updated 2 years ago

Land the ScrollTimeline API behind a pref

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox53 --- affected

People

(Reporter: botond, Unassigned)

References

(Blocks 4 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 1 obsolete file)

Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for scroll-driven animations.
58 bytes, text/x-review-board-request
bzbarsky
: review-
Details
Bug 1321428 - Represent scroll timeline information in the Layers API.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321428 - Populate scroll timeline information in FrameLayerBuilder.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321428 - Add a couple of helper functions to AsyncCompositionManager.
58 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1321428 - Handle scroll timelines in AsyncCompositionManager.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321428 - Add an async scrolling reftest for a scroll-driven animation.
58 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1321428 - Put scroll-driven animations behind a pref, and restrict to nightly builds.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1321428 - Allow restricting reftests to nightly builds only.
58 bytes, text/x-review-board-request
Details
Bug 1321428 - Support timelines that do not track wallclock time.
59 bytes, text/x-review-board-request
Details 
I've been working on scroll-driven animations (tracking bug [1], spec draft [2]) in a user repo [3].

I'd like to land part of this work, enabled on nightly branches only, and behind a pref.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1281348
[2] https://wicg.github.io/scroll-animations/
[3] https://hg.mozilla.org/users/bballo_mozilla.com/scroll-driven-animations/
Depends on: 1321885
These are WIP versions of the patches I intend to land here. They're not quite ready for review yet, they need a bit more work:

  - The conditioning on a pref and on nightly builds needs to be added

  - Some more tests wouldn't hurt
Now behind a pref and nightly-only.
Rebased across bug 1305325.
(In reply to Botond Ballo [:botond] from comment #13)
> Now behind a pref and nightly-only.

Having the feature behind a pref and nightly-only means the corresponding reftest needs to be behind a pref and nightly-only as well.

I put the reftest behind a pref, but making it nightly-only requires adding support to the reftest framework to check for that.
Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed14a437954d

I think we can get this reviewed and landed. I'll write some more tests as one of several pieces of follow-up work I have in mind.
Notes for reviewers:

  - On the first patch, I'm asking Boris for review of the interface/WebIDL
    parts, and Brian for the implementation.

  - Brian: these patches do not yet reflect the "use infinity rather than an
    unresolved time value outside the scroll range" approach that we discussed. 
    I do plan to explore that, but I'd like to not block the initial landing 
    on that exploration. Let me know if that seems reasonable.
Comment on attachment 8816602 [details]
Bug 1321428 - Add an async scrolling reftest for a scroll-driven animation.

https://reviewboard.mozilla.org/r/97296/#review99584

::: layout/reftests/async-scrolling/animation/reftest.list:3
(Diff revision 6)
> +default-preferences pref(dom.animations-api.scroll-driven.enabled,true) 
> +
> +skip-if(!isNightlyBuild) skip-if(!asyncPan) == transform-1.html transform-1-ref.html

This is somewhat dangerous: when we enable the API in non-nightly builds, what will make sure we enable the reftest too?

In any case, we're enabling the pref for this test anyway, right?  So why can't the test be enabled on all channels?
Comment on attachment 8816596 [details]
Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for scroll-driven animations.

https://reviewboard.mozilla.org/r/97284/#review99580

You're going to have an orange tree unless you adjust dom/tests/mochitest/general/test_interfaces.html accordingly, right?

::: dom/animation/ScrollTimeline.h:19
(Diff revision 4)
> +#include "mozilla/dom/ScrollTimelineBinding.h"
> +#include "AnimationTimeline.h"
> +#include "nsCSSValue.h"
> +#include "nsIDocument.h"
> +#include "nsIScrollPositionListener.h"
> +#include "mozilla/dom/Element.h"

Usually the mozilla/dom/ includes would be grouped together, and ideally in alphabetical order.

Same for the mozilla/ includes.

::: dom/animation/ScrollTimeline.h:21
(Diff revision 4)
> +#include "nsCSSValue.h"
> +#include "nsIDocument.h"
> +#include "nsIScrollPositionListener.h"
> +#include "mozilla/dom/Element.h"
> +#include "mozilla/dom/Animation.h"
> +#include "mozilla/LinkedList.h"

This is included twice.

::: dom/animation/ScrollTimeline.h:60
(Diff revision 4)
> +                 const nsString& aStartScrollOffset,
> +                 const nsString& aEndScrollOffset,
> +                 const OwningDoubleOrScrollTimelineAutoKeyword& aTimeRange,
> +                 FillMode aFillMode)
> +    : AnimationTimeline(aDocument->GetParentObject())
> +    , mDocument(aDocument)

What's supposed to happen if aTarget's node document changes?  If aTarget is removed from the document?  If aTarget stops being a scroll container?

At first glance this last is handled by just treating it as a scroll offset of 0, maybe, but it's hard to tell because the spec doesn't define what it means by "scroll offset".  Please raise spec issues about all these bits.  Also, need web platform tests.

::: dom/animation/ScrollTimeline.h:80
(Diff revision 4)
> +  {
> +    if (mDocument) {
> +      mDocument->ScrollTimelines().insertBack(this);
> +    }
> +    if (nsIScrollableFrame* scrollFrame = GetScrollFrame()) {
> +      scrollFrame->AddScrollPositionListener(mScrollPositionListener.get());

What happens if the element is reframed later (e.g. due to a display change on an ancestor, or due to its overflow styles changing or whatever)?  Looks to me like we'll stop getting notifications from it, because we will not be registered on the new scrollframe.  Again, this needs a test.

::: dom/animation/ScrollTimeline.h:90
(Diff revision 4)
> +  }
> +
> +protected:
> +  virtual ~ScrollTimeline()
> +  {
> +    // Note: cleanup happens in Teardown() which is called by

How do you know Unlink() will get called?  It'll only get called if you're part of a cycle, and I see nothing that guarantees that for these objects.

You want to call Teardown() from both places, and make sure it's idempotent.

::: dom/animation/ScrollTimeline.h:122
(Diff revision 4)
> +  Constructor(const GlobalObject& aGlobal,
> +              const ScrollTimelineOptions& aOptions,
> +              ErrorResult& aRv);
> +
> +  // WebIDL properties
> +  Element* SourceElement() const { return mElement.get(); }

You don't need the .get() here.

::: dom/animation/ScrollTimeline.h:122
(Diff revision 4)
> +  Constructor(const GlobalObject& aGlobal,
> +              const ScrollTimelineOptions& aOptions,
> +              ErrorResult& aRv);
> +
> +  // WebIDL properties
> +  Element* SourceElement() const { return mElement.get(); }

Spec says:

> If this is not specified, the document element is used.

Please raise a spec issue about it being unclear _which_ document's document element.  Also whether that determination is made at getter call time or constructor time or when.

::: dom/animation/ScrollTimeline.h:123
(Diff revision 4)
> +              const ScrollTimelineOptions& aOptions,
> +              ErrorResult& aRv);
> +
> +  // WebIDL properties
> +  Element* SourceElement() const { return mElement.get(); }
> +  ScrollDirection Orientation() const { return mComputedOrientation; }

Spec doesn't actually define what this getter should return.  Presumably the thing passed in the init dictionary?  If so, the spec needs to actually say that.  Please raise a spec issue.

Similar for the other properties on this interface, actually.

::: dom/animation/ScrollTimeline.cpp:51
(Diff revision 4)
> +void
> +ScrollTimeline::Teardown()
> +{
> +  if (nsIScrollableFrame* scrollFrame = GetScrollFrame()) {
> +    scrollFrame->RemoveScrollPositionListener(mScrollPositionListener.get());
> +  } else {

Please remove the empty else block.

::: dom/animation/ScrollTimeline.cpp:69
(Diff revision 4)
> +/* static */ already_AddRefed<ScrollTimeline>
> +ScrollTimeline::Constructor(const GlobalObject& aGlobal,
> +                            const ScrollTimelineOptions& aOptions,
> +                            ErrorResult& aRv)
> +{
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());

Why is this the right document?

::: dom/animation/ScrollTimeline.cpp:71
(Diff revision 4)
> +                            const ScrollTimelineOptions& aOptions,
> +                            ErrorResult& aRv)
> +{
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> +  if (!doc) {
> +    aRv.Throw(NS_ERROR_FAILURE);

This is clearly not a per-spec thing, right, since the spec has no concept of NS_ERROR_FAILURE?  This needs at least a comment, eg. about why we can end up in this situation.

::: dom/animation/ScrollTimeline.cpp:76
(Diff revision 4)
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +
> +  if (!aOptions.mScrollSource.WasPassed()) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Again, this can't be what a spec requires.  What should actually be happening here?

::: dom/animation/ScrollTimeline.cpp:183
(Diff revision 4)
> +}
> +
> +void
> +ScrollTimeline::QueryScrollValues()
> +{
> +  if (nsIScrollableFrame* frame = GetScrollFrame()) {

Might be nicer to just early-return if !frame and then go on with more outdenting.

::: dom/animation/ScrollTimeline.cpp:202
(Diff revision 4)
> +    mMaxScroll = InterpretValue(mComputedEndScrollOffset, min, max, max);
> +  }
> +}
> +
> +void
> +ScrollTimeline::NotifyScroll() {

Curly on next line.

::: dom/animation/ScrollTimeline.cpp:207
(Diff revision 4)
> +ScrollTimeline::NotifyScroll() {
> +  mNeedsTick = true;
> +}
> +
> +void
> +ScrollTimeline::Tick() {

Curly on next line.

::: dom/webidl/ScrollTimeline.webidl:7
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * https://w3c.github.io/web-animations/#documenttimeline

That doesn't look like the origin of this file, in that the linked-to spec doesn't define ScrollTimeline at all.  I assume this should be linking to https://wicg.github.io/scroll-animations/#scrolltimeline-interface instead.  Please fix accordingly.

Is there a plan for moving this out of WICG and into an official spec draft?

::: dom/webidl/ScrollTimeline.webidl:13
(Diff revision 4)
> + *
> + * Copyright © 2016 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +enum ScrollDirection { "vertical", "horizontal" };

This doesn't match the spec draft, right?  In fact, it only includes the values that ISSUE 1 for the spec talks about removing.

This is probably ok for an initial experiment, but please make sure there is a bug filed on aligning with the spec here that blocks the "ship web animations on release" bug.

::: dom/webidl/ScrollTimeline.webidl:18
(Diff revision 4)
> +enum ScrollDirection { "vertical", "horizontal" };
> +
> +enum ScrollTimelineAutoKeyword { "auto" };
> +
> +dictionary ScrollTimelineOptions {
> +  Element scrollSource;

If this is required to be present in the dictionary, why not say so in the IDL instead of writing explicit checks in the constructor?

I can't tell what behavior the spec expects here, because the spec doesn't actually define _anything_ about the behavior of the ScrollTimeline constructor.  Please raise a spec issue to actually define its behavior.

::: dom/webidl/ScrollTimeline.webidl:19
(Diff revision 4)
> +
> +enum ScrollTimelineAutoKeyword { "auto" };
> +
> +dictionary ScrollTimelineOptions {
> +  Element scrollSource;
> +  ScrollDirection orientation;

This doesn't match the spec draft either.  The spec has:

    ScrollDirection orientation = "auto";
    
Again, please make sure a bug is filed, etc.

::: dom/webidl/ScrollTimeline.webidl:20
(Diff revision 4)
> +enum ScrollTimelineAutoKeyword { "auto" };
> +
> +dictionary ScrollTimelineOptions {
> +  Element scrollSource;
> +  ScrollDirection orientation;
> +  DOMString startScrollOffset = "0";

This doesn't match the spec draft either...  The default value in the spec is "auto".

::: dom/webidl/ScrollTimeline.webidl:23
(Diff revision 4)
> +  Element scrollSource;
> +  ScrollDirection orientation;
> +  DOMString startScrollOffset = "0";
> +  DOMString endScrollOffset = "auto";
> +  (double or ScrollTimelineAutoKeyword) timeRange = "auto";
> +  FillMode fillMode = "none";

This doesn't match the spec either: the dictionary member is named "fill" in the spec.

::: dom/webidl/ScrollTimeline.webidl:27
(Diff revision 4)
> +  (double or ScrollTimelineAutoKeyword) timeRange = "auto";
> +  FillMode fillMode = "none";
> +};
> +
> +[Func="nsDocument::IsWebAnimationsEnabled",
> + Constructor(ScrollTimelineOptions options)]

This isn't valid IDL and doesn't match the spec (which does have valid IDL).  The dictionary needs to be optional, since it's in trailing position and doesn't have required members.  Again, unless scrollSource ends up required.

The fact that our codegen didn't catch this is very sad.  I filed bug 1324178 on that.
Attachment #8816596 - Flags: review?(bzbarsky) → review-
Boris, thanks a lot for your detailed feedback!

As you've noticed, and as I probably should have clarified, this is very much a WIP implementation of the spec (and the spec itself is very much a WIP at this stage). There is a fair amount of follow-up work I plan to do before enabling this, including waiting for the spec to mature, making sure the implementation is to spec, writing web platform tests as you mentioned, etc. The rationale for landing this work at this stage is to 1) make it easier for people to play around / experiment with, and 2) to avoid the patches sitting unreviewed and bitrotting for too long.

There are a number of implementation issues that you pointed out that I hadn't considered, such as an element being reframed or changing document. I'll do my best to address these in an updated version of the patch.
OK, it wasn't clear that we're still not at a "follow the current draft" stage yet.

If that's the case, then IDL/DOM review is not terribly useful, obviously, since the whole point of that review is to check that the spec makes sense and we're following the spec.  ;)  OK to land things in the current state in terms of the IDL, then; we'll just have to make sure we do that carefully at the point when we think we're close to the spec and the spec is more stable.

It might be OK to land without fixing the reframing thing too, if it's this early.  We just need to make sure that we don't turn things on until all that is fixed.

One other thing I forgot.  You should probably send an intent to implement per https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement which describes the current status of things.
Comment on attachment 8816598 [details]
Bug 1321428 - Populate scroll timeline information in FrameLayerBuilder.

https://reviewboard.mozilla.org/r/97288/#review99618

::: layout/painting/nsDisplayList.cpp:528
(Diff revision 4)
>      animation->baseStyle() = null_t();
>    }
> +  if (ScrollTimeline* scrollTimeline = aAnimation->GetTimeline()->AsScrollTimeline()) {
> +    // The scroll frame may have been reflowed since the ScrollTimeline was
> +    // constructed. Call QueryScrollValues() to pick up its updated metrics.
> +    scrollTimeline->QueryScrollValues();

It seems a bit failure prone to require the caller to know if a reflow might have happened since the object was last used, and then to know to call QueryScrollValues.

Is there any way to make this implicit?

Another alternative would be to make the getters private and only allow access from an RAII helper that calls QueryScrollValues for you.
Attachment #8816598 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8816597 [details]
Bug 1321428 - Represent scroll timeline information in the Layers API.

https://reviewboard.mozilla.org/r/97286/#review99620
Attachment #8816597 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8816600 [details]
Bug 1321428 - Handle scroll timelines in AsyncCompositionManager.

https://reviewboard.mozilla.org/r/97292/#review99626

::: gfx/layers/composite/AsyncCompositionManager.cpp:764
(Diff revision 4)
>            AnimData& animData = animationData[i];
>  
> +          TimeStamp timelineTime = aPoint;
> +          if (animation.scrollTimelineInfo().type() == MaybeScrollTimelineInfo::TScrollTimelineInfo) {
> +            const ScrollTimelineInfo& info = animation.scrollTimelineInfo();
> +            if (AsyncPanZoomController* apzc = FindAPZC(LayerMetricsWrapper(aLayer),

It might be worth moving this code into a helper that returns a bool, and call continue if it return false.

The multiple branches calling continue made the control flow a little hard to understand.

::: gfx/layers/composite/AsyncCompositionManager.cpp:781
(Diff revision 4)
> +              }
> +              Nullable<TimeDuration> currentTime =
> +                  ScrollTimelineUtils::CalculateCurrentTime(currentScroll,
> +                      minScroll, maxScroll, info.timeRange(), info.fillMode());
> +              if (!currentTime.IsNull()) {
> +                // TODO: Is this right?

It seems like it is :)

As Boris said, we'll need some thorough tests before this can be enabled.
Attachment #8816600 - Flags: review?(matt.woodrow) → review+
Attachment #8818710 - Flags: review?(matt.woodrow)
Comment on attachment 8818710 [details]
Bug 1321428 - Allow restricting reftests to nightly builds only.

https://reviewboard.mozilla.org/r/98666/#review99630

I'm probably not the right person to review changes to nsDOMWindowUtils, but the reftest.jsm change looks good!
Hi Botond, I'm working through the first patch and one thing I don't understand is why we call TickScrollTimelines() from nsRefreshDriver::Tick. Is it not possible to make ScrollTimelines observe the refresh driver like DocumentTimelines do?

I'd prefer that these different types of timelines get called at the same point in the refresh driver lifecycle. As it is, I think these will happen at more-or-less the same point, but I'm concerned that if, say, someone adds another call before TickScrollTimelines() (e.g. some sort of event dispatch) then it will be observable that some animations were updated and others were not.

If we can't just observe the refresh driver (e.g. we have strict ordering requirements) then we should probably take DocumentTimelines and tick them in the same way.
Comment on attachment 8816596 [details]
Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for scroll-driven animations.

https://reviewboard.mozilla.org/r/97284/#review99714

I'm still working my way through this but I have the following questions about TracksWallclockTime().

::: dom/animation/ScrollTimeline.h:104
(Diff revision 4)
> +  bool TracksWallclockTime() const override {
> +    return true;
> +  }

This should be false, right? We can't convert TimeStamps to and from scroll-driven timeline "times"?

::: dom/animation/ScrollTimeline.cpp:97
(Diff revision 4)
> +Nullable<TimeDuration>
> +ScrollTimeline::ToTimelineTime(const TimeStamp& aTimeStamp) const
> +{
> +  // Memo: We don't concern about clock time and zero time.
> +  Nullable<TimeDuration> result; // Initializes to null
> +  if (aTimeStamp.IsNull()) {
> +    return result;
> +  }
> +
> +  RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
> +  if (MOZ_UNLIKELY(!timing)) {
> +    return result;
> +  }
> +
> +  result.SetValue(aTimeStamp
> +                  - timing->GetNavigationStartTimeStamp());
> +  return result;
> +}

See my comments above about TracksWallclockTime(), but I think this should just return null.

::: dom/animation/ScrollTimeline.cpp:132
(Diff revision 4)
> +TimeStamp
> +ScrollTimeline::ToTimeStamp(const TimeDuration& aTimeDuration) const
> +{
> +  TimeStamp result;
> +  RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
> +  if (MOZ_UNLIKELY(!timing)) {
> +    return result;
> +  }
> +
> +  result =
> +    timing->GetNavigationStartTimeStamp() + aTimeDuration;
> +  return result;
> +}

Likewise this should just return a null TimeStamp, I believe.
Comment on attachment 8816599 [details]
Bug 1321428 - Add a couple of helper functions to AsyncCompositionManager.

https://reviewboard.mozilla.org/r/97290/#review99932

::: gfx/layers/composite/AsyncCompositionManager.cpp:715
(Diff revision 4)
> +  }
> +  return 0;
> +}
> +
> +static AsyncPanZoomController*
> +FindAPZC(LayerMetricsWrapper aRoot, uint64_t aLayersId, FrameMetrics::ViewID aViewId)

This is basically a DFS, right? Can we use ForEachNode or one of those treewalker helpers instead?
Attachment #8816599 - Flags: review?(bugmail) → review+
Comment on attachment 8818710 [details]
Bug 1321428 - Allow restricting reftests to nightly builds only.

https://reviewboard.mozilla.org/r/98666/#review99936

::: layout/tools/reftest/reftest.jsm:686
(Diff revision 3)
>        gWindowUtils.layerManagerType == "OpenGL";
>      sandbox.layersOMTC =
>        gWindowUtils.layerManagerRemote == true;
> +      
> +    // This allows restricting certain reftests to nightly builds.
> +    sandbox.isNightlyBuild = gWindowUtils.isNightlyBuild;

I don't think you need the DOMWindowUTils functions here. You can just #ifdef inside reftest.jsm. There are similar ifdefs for MOZ_STYLO, MOZ_WEBRTC, etc.
Comment on attachment 8816602 [details]
Bug 1321428 - Add an async scrolling reftest for a scroll-driven animation.

https://reviewboard.mozilla.org/r/97296/#review99938
Attachment #8816602 - Flags: review?(bugmail) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> ::: layout/reftests/async-scrolling/animation/reftest.list:3
> (Diff revision 6)
> > +default-preferences pref(dom.animations-api.scroll-driven.enabled,true) 
> > +
> > +skip-if(!isNightlyBuild) skip-if(!asyncPan) == transform-1.html transform-1-ref.html
> 
> This is somewhat dangerous: when we enable the API in non-nightly builds,
> what will make sure we enable the reftest too?
> 
> In any case, we're enabling the pref for this test anyway, right?  So why
> can't the test be enabled on all channels?

Due to the early stage of the spec and the implementation, I'm enabling the feature only if you're on the Nightly channel _and_ you've set the pref. 

The "Nightly" part of the condition is implemented with an #ifdef in nsDocument::IsScrollDrivenAnimationsEnabled().

I can add a comment there to remind myself to adjust the reftests as well when eventually removing that #ifdef.
> I can add a comment there to remind myself to adjust the reftests as well when eventually removing that #ifdef.

Sounds great, thanks!
Blocks: 1324602
Blocks: 1324605
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> You're going to have an orange tree unless you adjust
> dom/tests/mochitest/general/test_interfaces.html accordingly, right?

The test wasn't failing in my Try push (comment 35), I guess because the pref isn't enabled when that test runs? Should I add the new interface to test_interfaces.html anyways, or wait until we enable the pref on some channel?

> ::: dom/animation/ScrollTimeline.h:60
> What's supposed to happen if aTarget's node document changes?  If aTarget is
> removed from the document?  If aTarget stops being a scroll container?
> 
> At first glance this last is handled by just treating it as a scroll offset
> of 0, maybe, but it's hard to tell because the spec doesn't define what it
> means by "scroll offset".  Please raise spec issues about all these bits. 
> Also, need web platform tests.

Excellent questions!

Spec issues filed:
  https://github.com/WICG/scroll-animations/issues/2
  https://github.com/WICG/scroll-animations/issues/3
  https://github.com/WICG/scroll-animations/issues/4

I'll think about what the behaviour should be in each of these cases, and follow up about that.

Web platform tests will forthcome in bug 1324605.

> ::: dom/animation/ScrollTimeline.h:80
> (Diff revision 4)
> > +  {
> > +    if (mDocument) {
> > +      mDocument->ScrollTimelines().insertBack(this);
> > +    }
> > +    if (nsIScrollableFrame* scrollFrame = GetScrollFrame()) {
> > +      scrollFrame->AddScrollPositionListener(mScrollPositionListener.get());
> 
> What happens if the element is reframed later (e.g. due to a display change
> on an ancestor, or due to its overflow styles changing or whatever)?  Looks
> to me like we'll stop getting notifications from it, because we will not be
> registered on the new scrollframe.  Again, this needs a test.

Hmm, the initial prototype by Mantaroh had two levels of listeners, one on the element and one on the scroll frame. I replaced that with a listener directly on the scroll frame, thinking I was simplifying things, but perhaps the two levels is necessary to handle reframing, and I just failed to appreciate that. 

I'll review that, and assuming it does handle reframing, bring back the two levels of listeners.

> ::: dom/animation/ScrollTimeline.h:122
> (Diff revision 4)
> > +  Constructor(const GlobalObject& aGlobal,
> > +              const ScrollTimelineOptions& aOptions,
> > +              ErrorResult& aRv);
> > +
> > +  // WebIDL properties
> > +  Element* SourceElement() const { return mElement.get(); }
> 
> Spec says:
> 
> > If this is not specified, the document element is used.
> 
> Please raise a spec issue about it being unclear _which_ document's document
> element.  Also whether that determination is made at getter call time or
> constructor time or when.

Filed https://github.com/WICG/scroll-animations/issues/5.

> ::: dom/animation/ScrollTimeline.h:123
> (Diff revision 4)
> > +              const ScrollTimelineOptions& aOptions,
> > +              ErrorResult& aRv);
> > +
> > +  // WebIDL properties
> > +  Element* SourceElement() const { return mElement.get(); }
> > +  ScrollDirection Orientation() const { return mComputedOrientation; }
> 
> Spec doesn't actually define what this getter should return.  Presumably the
> thing passed in the init dictionary?  If so, the spec needs to actually say
> that.  Please raise a spec issue.
> 
> Similar for the other properties on this interface, actually.

Filed https://github.com/WICG/scroll-animations/issues/6.

> ::: dom/animation/ScrollTimeline.cpp:69
> (Diff revision 4)
> > +/* static */ already_AddRefed<ScrollTimeline>
> > +ScrollTimeline::Constructor(const GlobalObject& aGlobal,
> > +                            const ScrollTimelineOptions& aOptions,
> > +                            ErrorResult& aRv)
> > +{
> > +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> 
> Why is this the right document?

I guess that depends on the outcome of some of the above spec issues. Will follow up on this.

> ::: dom/animation/ScrollTimeline.cpp:71
> (Diff revision 4)
> > +                            const ScrollTimelineOptions& aOptions,
> > +                            ErrorResult& aRv)
> > +{
> > +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> > +  if (!doc) {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> 
> This is clearly not a per-spec thing, right, since the spec has no concept
> of NS_ERROR_FAILURE?  This needs at least a comment, eg. about why we can
> end up in this situation.

DocumentTimeline::Constructor() does the same thing. Should I file a spec issue against Web Animations for this?

> ::: dom/animation/ScrollTimeline.cpp:76
> (Diff revision 4)
> > +    aRv.Throw(NS_ERROR_FAILURE);
> > +    return nullptr;
> > +  }
> > +
> > +  if (!aOptions.mScrollSource.WasPassed()) {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> 
> Again, this can't be what a spec requires.  What should actually be
> happening here?

We should be using the document element (with the question of "which document" to be nailed down, as you pointed out). This is just not implemented yet. I'll file a follow-up for this.

> Is there a plan for moving this out of WICG and into an official spec draft?

It was just accepted into WICG not that long ago.

The plan is to move this into W3C in due course, but I'm not too familiar with the process in terms of what milestones need to be reached before that can happen.

> ::: dom/webidl/ScrollTimeline.webidl:13
> (Diff revision 4)
> > + *
> > + * Copyright © 2016 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> > + * liability, trademark and document use rules apply.
> > + */
> > +
> > +enum ScrollDirection { "vertical", "horizontal" };
> 
> This doesn't match the spec draft, right?  In fact, it only includes the
> values that ISSUE 1 for the spec talks about removing.
> 
> This is probably ok for an initial experiment, but please make sure there is
> a bug filed on aligning with the spec here that blocks the "ship web
> animations on release" bug.

The are several things related to orientation that are not implemented yet:

  - logical directions
  - auto (to choose the scrollable direction if there's only one)
  - changing the orientation after constructing a timeline

I'll file follow-ups.

> ::: dom/webidl/ScrollTimeline.webidl:18
> (Diff revision 4)
> > +enum ScrollDirection { "vertical", "horizontal" };
> > +
> > +enum ScrollTimelineAutoKeyword { "auto" };
> > +
> > +dictionary ScrollTimelineOptions {
> > +  Element scrollSource;
> 
> If this is required to be present in the dictionary, why not say so in the
> IDL instead of writing explicit checks in the constructor?
> 
> I can't tell what behavior the spec expects here, because the spec doesn't
> actually define _anything_ about the behavior of the ScrollTimeline
> constructor.  Please raise a spec issue to actually define its behavior.

scrollSource is optional. The spec does say "If this is not specified, the document element is used".

That said, I did file a spec issue, https://github.com/WICG/scroll-animations/issues/7.
> I guess because the pref isn't enabled when that test runs?

Yeah, I had misunderstood the setup and thought this was default-enabled on nightly.  If it's not, then no need to mess with test_interfaces.html.  Once we flip the pref on some channel, we'll want to add it there to reflect that.  The idea is that this file lists what we _expect_ to be visible and on which channel, and right now we don't expect this API to be visible.

> DocumentTimeline::Constructor() does the same thing. Should I file a spec issue against Web Animations for this?

Probably, yes.

That said, in practice maybe this can only return failure if we're calling the constructor from a torn-down window for which GetDoc() returns null, which is a situation the spec thinks can't exist.  So it's possible that returning a Gecko-specific error code is the right thing here; needs a comment.

Thanks for filing the various followups!
Comment on attachment 8816619 [details]
Bug 1321428 - Put scroll-driven animations behind a pref, and restrict to nightly builds.

https://reviewboard.mozilla.org/r/97298/#review100518
Attachment #8816619 - Flags: review?(bbirtles) → review+
Comment on attachment 8816596 [details]
Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for scroll-driven animations.

https://reviewboard.mozilla.org/r/97284/#review100520

This looks mostly good to me but I'm clearing review request for now since I'd like to know the answer to comment 54 and comment 55.

::: dom/animation/ScrollTimeline.h:170
(Diff revision 4)
> +  nsString mSpecifiedStartScrollOffset;
> +  nsString mSpecifiedEndScrollOffset;

Do we need to store these? It seems like we could parse them in ScrollTimeline::Constructor then synthesize the string values as-needed from mComputedStart/EndScrollOffset?

::: dom/animation/ScrollTimeline.cpp:149
(Diff revision 4)
> +}
> +
> +void
> +ScrollTimeline::CalculateEffectiveTimeRange()
> +{
> +  if (mSpecifiedTimeRangeCooked.isSome()) {

Nit: Maybe has "operator bool()" defined to mean isSome() so we don't need '.isSome()' here.

::: dom/animation/ScrollTimeline.cpp:166
(Diff revision 4)
> +  switch (aValue.GetUnit())
> +  {
> +  case eCSSUnit_Auto:
> +    return aDefault;
> +  case eCSSUnit_Pixel:
> +    return Clamp(aValue.GetPixelLength(), aMin, aMax);
> +  case eCSSUnit_Percent:
> +    return aMin + ((aMax - aMin) * aValue.GetPercentValue());
> +  default:
> +    NS_WARNING("Unhandled CSS value type");
> +    return aDefault;
> +  }

Nit: I think our coding style wants:

  switch (aValue.GetUnit()) {
    case eCSSUnit_Auto:
      return aDefault;
    ...
  }

here

::: dom/animation/ScrollTimelineUtils.cpp:21
(Diff revision 4)
> +    if (aFillMode == FillMode::Both || aFillMode == FillMode::Backwards) {
> +      result.SetValue(TimeDuration(0));
> +    }

As discussed recently, I think we will eventually drop fill modes from the timeline, but for now this is fine.

One question, however, we have a constexpr constructor for TimeDuration that is equivalent to TimeDuration(0)--do you know what difference using that is likely to make to the generated code in a case like this? Is it worth using TimeDuration() here instead?

::: dom/animation/ScrollTimelineUtils.cpp:34
(Diff revision 4)
> +  if (scrollRange != 0 && !aEffectiveTimeRange.IsZero() &&
> +      aEffectiveTimeRange != TimeDuration::Forever()) {
> +    result.SetValue(
> +        TimeDuration(aEffectiveTimeRange.MultDouble(scrollPosition / scrollRange)));

Should the result be zero if the effective time range is infinity?
Attachment #8816596 - Flags: review?(bbirtles)
Comment on attachment 8816601 [details]
Bug 1321428 - Have ScrollTimelines be kept alive by their scrollSource while there are animations associated with them.

https://reviewboard.mozilla.org/r/97294/#review100550

::: dom/animation/ScrollTimeline.cpp:295
(Diff revision 4)
> +  if (ScrollTimelineSet* scrollTimelineSet =
> +      ScrollTimelineSet::GetScrollTimelineSet(mElement)) {

Nit: As with bz's comment on an earlier patch, I think it's more common in our code to just add an early return if !scrollTimelineSet (although it's less important in this case since the indented code block is very short).
Attachment #8816601 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #54)
> Hi Botond, I'm working through the first patch and one thing I don't
> understand is why we call TickScrollTimelines() from nsRefreshDriver::Tick.
> Is it not possible to make ScrollTimelines observe the refresh driver like
> DocumentTimelines do?
> 
> I'd prefer that these different types of timelines get called at the same
> point in the refresh driver lifecycle. As it is, I think these will happen
> at more-or-less the same point, but I'm concerned that if, say, someone adds
> another call before TickScrollTimelines() (e.g. some sort of event dispatch)
> then it will be observable that some animations were updated and others were
> not.

The reasoning for this can be found here [1]. See in particular [2] and [3].

I can add some code comments to record the reasoning for people looking at the code in the future.

> If we can't just observe the refresh driver (e.g. we have strict ordering
> requirements) then we should probably take DocumentTimelines and tick them
> in the same way.

I could replace TickScrollTimelines() with TickTimelines() which ticks both kinds of timelines, if you feel that's a more future-proof approach.

[1] https://github.com/theres-waldo/scroll-driven-animations/issues/3
[2] https://github.com/theres-waldo/scroll-driven-animations/issues/3#issuecomment-246531260
[3] https://github.com/theres-waldo/scroll-driven-animations/issues/3#issuecomment-248103375
(In reply to Brian Birtles (:birtles) from comment #55)
> Comment on attachment 8816596 [details]
> Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for
> scroll-driven animations.
> 
> https://reviewboard.mozilla.org/r/97284/#review99714
> 
> I'm still working my way through this but I have the following questions
> about TracksWallclockTime().
> 
> ::: dom/animation/ScrollTimeline.h:104
> (Diff revision 4)
> > +  bool TracksWallclockTime() const override {
> > +    return true;
> > +  }
> 
> This should be false, right? We can't convert TimeStamps to and from
> scroll-driven timeline "times"?
> 
> ::: dom/animation/ScrollTimeline.cpp:97
> (Diff revision 4)
> > +Nullable<TimeDuration>
> > +ScrollTimeline::ToTimelineTime(const TimeStamp& aTimeStamp) const
> > +{
> > +  // Memo: We don't concern about clock time and zero time.
> > +  Nullable<TimeDuration> result; // Initializes to null
> > +  if (aTimeStamp.IsNull()) {
> > +    return result;
> > +  }
> > +
> > +  RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
> > +  if (MOZ_UNLIKELY(!timing)) {
> > +    return result;
> > +  }
> > +
> > +  result.SetValue(aTimeStamp
> > +                  - timing->GetNavigationStartTimeStamp());
> > +  return result;
> > +}
> 
> See my comments above about TracksWallclockTime(), but I think this should
> just return null.
> 
> ::: dom/animation/ScrollTimeline.cpp:132
> (Diff revision 4)
> > +TimeStamp
> > +ScrollTimeline::ToTimeStamp(const TimeDuration& aTimeDuration) const
> > +{
> > +  TimeStamp result;
> > +  RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
> > +  if (MOZ_UNLIKELY(!timing)) {
> > +    return result;
> > +  }
> > +
> > +  result =
> > +    timing->GetNavigationStartTimeStamp() + aTimeDuration;
> > +  return result;
> > +}
> 
> Likewise this should just return a null TimeStamp, I believe.

Honestly, these functions were like this in Mantaroh's initial prototype, and I didn't look at them too closely.

I agree that TracksWallclockTime() sounds like something that should be false for a ScrollTimeline :) I'll change it and see if everything still works.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:715
> (Diff revision 4)
> > +  }
> > +  return 0;
> > +}
> > +
> > +static AsyncPanZoomController*
> > +FindAPZC(LayerMetricsWrapper aRoot, uint64_t aLayersId, FrameMetrics::ViewID aViewId)
> 
> This is basically a DFS, right? Can we use ForEachNode or one of those
> treewalker helpers instead?

Yep - good idea!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #57)
> Comment on attachment 8818710 [details]
> Bug 1321428 - Allow restricting reftests to nightly builds only.
> 
> https://reviewboard.mozilla.org/r/98666/#review99936
> 
> ::: layout/tools/reftest/reftest.jsm:686
> (Diff revision 3)
> >        gWindowUtils.layerManagerType == "OpenGL";
> >      sandbox.layersOMTC =
> >        gWindowUtils.layerManagerRemote == true;
> > +      
> > +    // This allows restricting certain reftests to nightly builds.
> > +    sandbox.isNightlyBuild = gWindowUtils.isNightlyBuild;
> 
> I don't think you need the DOMWindowUTils functions here. You can just
> #ifdef inside reftest.jsm. There are similar ifdefs for MOZ_STYLO,
> MOZ_WEBRTC, etc.

Oh, indeed, good catch! That simplifies this patch considerably.
Hi Botond. I've turned off review requests since I'm away / moving office for the next few days but if re-review of the first patch in this series is the only thing blocking you from landing this, by all means needinfo me and I'll try to get to it during the break.
(In reply to Brian Birtles (:birtles, away or busy until 6 Jan) from comment #64)
> ::: dom/animation/ScrollTimeline.h:170
> (Diff revision 4)
> > +  nsString mSpecifiedStartScrollOffset;
> > +  nsString mSpecifiedEndScrollOffset;
> 
> Do we need to store these? It seems like we could parse them in
> ScrollTimeline::Constructor then synthesize the string values as-needed from
> mComputedStart/EndScrollOffset?

We could - it's a time/space tradeoff. Do you have a particular reason to prefer one over the other?

> ::: dom/animation/ScrollTimelineUtils.cpp:21
> (Diff revision 4)
> > +    if (aFillMode == FillMode::Both || aFillMode == FillMode::Backwards) {
> > +      result.SetValue(TimeDuration(0));
> > +    }
> 
> One question, however, we have a constexpr constructor for TimeDuration that
> is equivalent to TimeDuration(0)--do you know what difference using that is
> likely to make to the generated code in a case like this? Is it worth using
> TimeDuration() here instead?

I don't expect it will make a difference in the generated code, but I'm not an authority on compiler optimization. We can use TimeDuration() for good measure.

> ::: dom/animation/ScrollTimelineUtils.cpp:34
> (Diff revision 4)
> > +  if (scrollRange != 0 && !aEffectiveTimeRange.IsZero() &&
> > +      aEffectiveTimeRange != TimeDuration::Forever()) {
> > +    result.SetValue(
> > +        TimeDuration(aEffectiveTimeRange.MultDouble(scrollPosition / scrollRange)));
> 
> Should the result be zero if the effective time range is infinity?

The spec says:

  "If any animation directly associated with the timeline has a 
   target effect end of infinity, the behavior is unspecified."

So it's conforming. Do you have a suggestion for a different behaviour here?
(In reply to Botond Ballo [:botond] from comment #70)
> (In reply to Brian Birtles (:birtles, away or busy until 6 Jan) from comment
> #64)
> > ::: dom/animation/ScrollTimeline.h:170
> > (Diff revision 4)
> > > +  nsString mSpecifiedStartScrollOffset;
> > > +  nsString mSpecifiedEndScrollOffset;
> > 
> > Do we need to store these? It seems like we could parse them in
> > ScrollTimeline::Constructor then synthesize the string values as-needed from
> > mComputedStart/EndScrollOffset?
> 
> We could - it's a time/space tradeoff. Do you have a particular reason to
> prefer one over the other?

Not a strong preference, I just generally prefer to store less state to avoid it getting out of sync. As for time vs space, I think the serializers are very rarely used so it probably is better to save a bit of memory in the 99% case for a tiny increase in time (in generally non-performance critical code) in the 1% case. But like I said, I don't particularly mind.

> > ::: dom/animation/ScrollTimelineUtils.cpp:34
> > (Diff revision 4)
> > > +  if (scrollRange != 0 && !aEffectiveTimeRange.IsZero() &&
> > > +      aEffectiveTimeRange != TimeDuration::Forever()) {
> > > +    result.SetValue(
> > > +        TimeDuration(aEffectiveTimeRange.MultDouble(scrollPosition / scrollRange)));
> > 
> > Should the result be zero if the effective time range is infinity?
> 
> The spec says:
> 
>   "If any animation directly associated with the timeline has a 
>    target effect end of infinity, the behavior is unspecified."
> 
> So it's conforming. Do you have a suggestion for a different behaviour here?

Yes, I think we discussed this once, but I have a strong aversion to undefined behaviour in Web-facing specs. I think the most consistent thing here is that the time should be zero.

(I *think* this will be correct even if scrollPosition >= scrollRange. I was wondering if the time should be infinity in that case, like what was proposed in [1] but I'm fairly sure that we should just treat the animation as this infinitely long thing that you never make any progress in.)

[1] https://github.com/w3c/web-animations/issues/174#issuecomment-262854357
Just wanted to give an update here. I've been working on addressing the review comments for these patches, it's just been slow-going as I've also been busy with other APZ bugs.

The latest rebased WIP patch series can be found here:

https://hg.mozilla.org/users/bballo_mozilla.com/scroll-driven-animations/
Attachment #8816601 - Attachment is obsolete: true
Boris, Bugzilla says you're not accepting review requests. I assume you'd still like to review this, since you reviewed it initially? If not, should I ask another DOM peer for review on the WebIDL changes?
Flags: needinfo?(bzbarsky)
(In reply to Botond Ballo [:botond] from comment #82)
> I assume you'd still like to review this

(By "this" I mean the first patch, "Introduce ScrollTimeline...").
(In reply to Botond Ballo [:botond] from comment #61)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> > ::: dom/animation/ScrollTimeline.h:60
> > What's supposed to happen if aTarget's node document changes?  If aTarget is
> > removed from the document?  If aTarget stops being a scroll container?
> > 
> > At first glance this last is handled by just treating it as a scroll offset
> > of 0, maybe, but it's hard to tell because the spec doesn't define what it
> > means by "scroll offset".  Please raise spec issues about all these bits. 
> > Also, need web platform tests.
> 
> Excellent questions!
> 
> Spec issues filed:
>   https://github.com/WICG/scroll-animations/issues/2
>   https://github.com/WICG/scroll-animations/issues/3
>   https://github.com/WICG/scroll-animations/issues/4
> 
> I'll think about what the behaviour should be in each of these cases, and
> follow up about that.
> 
> Web platform tests will forthcome in bug 1324605.

These issues have now been resolved in the spec, and the updated patches implement their resolutions.

> > ::: dom/animation/ScrollTimeline.h:80
> > (Diff revision 4)
> > > +  {
> > > +    if (mDocument) {
> > > +      mDocument->ScrollTimelines().insertBack(this);
> > > +    }
> > > +    if (nsIScrollableFrame* scrollFrame = GetScrollFrame()) {
> > > +      scrollFrame->AddScrollPositionListener(mScrollPositionListener.get());
> > 
> > What happens if the element is reframed later (e.g. due to a display change
> > on an ancestor, or due to its overflow styles changing or whatever)?  Looks
> > to me like we'll stop getting notifications from it, because we will not be
> > registered on the new scrollframe.  Again, this needs a test.
> 
> Hmm, the initial prototype by Mantaroh had two levels of listeners, one on
> the element and one on the scroll frame. I replaced that with a listener
> directly on the scroll frame, thinking I was simplifying things, but perhaps
> the two levels is necessary to handle reframing, and I just failed to
> appreciate that. 
> 
> I'll review that, and assuming it does handle reframing, bring back the two
> levels of listeners.

The updated patches handle reframing. There aren't two levels of listeners, but the scrollSource knows about its ScrollTimelines, and when we construct an nsIScrollableFrame, the relevant listeners are re-added.

> > ::: dom/animation/ScrollTimeline.h:122
> > (Diff revision 4)
> > > +  Constructor(const GlobalObject& aGlobal,
> > > +              const ScrollTimelineOptions& aOptions,
> > > +              ErrorResult& aRv);
> > > +
> > > +  // WebIDL properties
> > > +  Element* SourceElement() const { return mElement.get(); }
> > 
> > Spec says:
> > 
> > > If this is not specified, the document element is used.
> > 
> > Please raise a spec issue about it being unclear _which_ document's document
> > element.  Also whether that determination is made at getter call time or
> > constructor time or when.
> 
> Filed https://github.com/WICG/scroll-animations/issues/5.

Also resolved and the resolution implemented.

> > ::: dom/animation/ScrollTimeline.h:123
> > (Diff revision 4)
> > > +              const ScrollTimelineOptions& aOptions,
> > > +              ErrorResult& aRv);
> > > +
> > > +  // WebIDL properties
> > > +  Element* SourceElement() const { return mElement.get(); }
> > > +  ScrollDirection Orientation() const { return mComputedOrientation; }
> > 
> > Spec doesn't actually define what this getter should return.  Presumably the
> > thing passed in the init dictionary?  If so, the spec needs to actually say
> > that.  Please raise a spec issue.
> > 
> > Similar for the other properties on this interface, actually.
> 
> Filed https://github.com/WICG/scroll-animations/issues/6.

Likewise.

> > ::: dom/animation/ScrollTimeline.cpp:69
> > (Diff revision 4)
> > > +/* static */ already_AddRefed<ScrollTimeline>
> > > +ScrollTimeline::Constructor(const GlobalObject& aGlobal,
> > > +                            const ScrollTimelineOptions& aOptions,
> > > +                            ErrorResult& aRv)
> > > +{
> > > +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> > 
> > Why is this the right document?
> 
> I guess that depends on the outcome of some of the above spec issues. Will
> follow up on this.

The spec now says that the document in question is "the active document in the current browsing context". The Web Animations spec uses the same wording, and uses AnimationUtils::GetCurrentRealmDocument() to fetch that document, so I assume that's correct.

> > ::: dom/animation/ScrollTimeline.cpp:76
> > (Diff revision 4)
> > > +    aRv.Throw(NS_ERROR_FAILURE);
> > > +    return nullptr;
> > > +  }
> > > +
> > > +  if (!aOptions.mScrollSource.WasPassed()) {
> > > +    aRv.Throw(NS_ERROR_FAILURE);
> > 
> > Again, this can't be what a spec requires.  What should actually be
> > happening here?
> 
> We should be using the document element (with the question of "which
> document" to be nailed down, as you pointed out). This is just not
> implemented yet. I'll file a follow-up for this.

I just implemented this in the updated patch.
(In reply to Brian Birtles (:birtles) from comment #71)
> (In reply to Botond Ballo [:botond] from comment #70)
> > (In reply to Brian Birtles (:birtles, away or busy until 6 Jan) from comment
> > #64)
> > > ::: dom/animation/ScrollTimeline.h:170
> > > (Diff revision 4)
> > > > +  nsString mSpecifiedStartScrollOffset;
> > > > +  nsString mSpecifiedEndScrollOffset;
> > > 
> > > Do we need to store these? It seems like we could parse them in
> > > ScrollTimeline::Constructor then synthesize the string values as-needed from
> > > mComputedStart/EndScrollOffset?
> > 
> > We could - it's a time/space tradeoff. Do you have a particular reason to
> > prefer one over the other?
> 
> Not a strong preference, I just generally prefer to store less state to
> avoid it getting out of sync. As for time vs space, I think the serializers
> are very rarely used so it probably is better to save a bit of memory in the
> 99% case for a tiny increase in time (in generally non-performance critical
> code) in the 1% case. But like I said, I don't particularly mind.

I implemented your suggestion in the updated patch.

> > > ::: dom/animation/ScrollTimelineUtils.cpp:34
> > > (Diff revision 4)
> > > > +  if (scrollRange != 0 && !aEffectiveTimeRange.IsZero() &&
> > > > +      aEffectiveTimeRange != TimeDuration::Forever()) {
> > > > +    result.SetValue(
> > > > +        TimeDuration(aEffectiveTimeRange.MultDouble(scrollPosition / scrollRange)));
> > > 
> > > Should the result be zero if the effective time range is infinity?
> > 
> > The spec says:
> > 
> >   "If any animation directly associated with the timeline has a 
> >    target effect end of infinity, the behavior is unspecified."
> > 
> > So it's conforming. Do you have a suggestion for a different behaviour here?
> 
> Yes, I think we discussed this once, but I have a strong aversion to
> undefined behaviour in Web-facing specs. I think the most consistent thing
> here is that the time should be zero.

I filed a spec issue for this [1], resolved it as you suggested, and implemented its resolution in the updated patch.

[1] https://github.com/WICG/scroll-animations/issues/10
See Also: → 1350456
Blocks: 1350457
(In reply to Botond Ballo [:botond] from comment #61)
> The are several things related to orientation that are not implemented yet:
> 
>   - logical directions
>   - auto (to choose the scrollable direction if there's only one)
>   - changing the orientation after constructing a timeline
> 
> I'll file follow-ups.

Filed bug 1350457 for this.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #62)
> > DocumentTimeline::Constructor() does the same thing. Should I file a spec issue against Web Animations for this?
> 
> Probably, yes.
> 
> That said, in practice maybe this can only return failure if we're calling
> the constructor from a torn-down window for which GetDoc() returns null,
> which is a situation the spec thinks can't exist.  So it's possible that
> returning a Gecko-specific error code is the right thing here; needs a
> comment.

Filed bug 1350456 for this.
Blocks: 1350461
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #49)
> One other thing I forgot.  You should probably send an intent to implement
> per https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement
> which describes the current status of things.

Intent to implement email sent: https://groups.google.com/forum/#!topic/mozilla.dev.platform/4YST7JQysiU
I believe all comments now have either been addressed in the updated patch series, or have a follow-up bug filed for them.
> The spec now says that the document in question is "the active document in the current browsing context".

That's a security bug _and_ underdefined (because there is no defined concept of "current browsing context").  Filed spec issues at https://github.com/WICG/scroll-animations/issues/11 and <https://github.com/WICG/scroll-animations/issues/12> but I expect the former is more important than the latter, in that fixing it properly will likely make the latter moot.

> The Web Animations spec uses the same wording, and uses
> AnimationUtils::GetCurrentRealmDocument() to fetch that document, so I assume that's correct.

Filed https://github.com/w3c/web-animations/issues/183 on the security bug in the spec there.  The good news is that GetCurrentRealmDocument() doesn't have that security bug (and in fact implements a behavior similar to the one I'm suggesting be implemented in <https://github.com/WICG/scroll-animations/issues/11>).  The bad news is that this means our code and the spec are at best third cousins here, or some similar distant relations.
Comment on attachment 8816596 [details]
Bug 1321428 - Introduce ScrollTimeline, a new kind of AnimationTimeline for scroll-driven animations.

https://reviewboard.mozilla.org/r/97284/#review126038

::: dom/animation/ScrollTimeline.h:34
(Diff revision 5)
> +#endif
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class ScrollPositionListener : public nsIScrollPositionListener {

Why does this need to be a separate class?  That is, why can the ScrollTimeline not be the listener itself?  In terms of lifetimes, it doesn't look like a timeline ever creates more than one listener, and it owns it in a way that makes their lifetimes exactly identical, right?

::: dom/animation/ScrollTimeline.cpp:80
(Diff revision 5)
> +  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> +  if (!doc) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Hmm.  So this ends up working, because our bindings enter the compartment of the global the constructor came from even when invoking a constructor via Xrays.  But it's pretty subtle that it works, and might be worth a comment that it's depending on this binding behavior.  If it were not for that, this constructor would always throw when called from something like a WebExtension content script.

::: dom/animation/ScrollTimeline.cpp:90
(Diff revision 5)
> +
> +  Element* scrollSource = aOptions.mScrollSource.WasPassed()
> +      ? &aOptions.mScrollSource.Value()
> +      : doc->GetDocumentElement();
> +  if (!scrollSource) {
> +    aRv.Throw(NS_ERROR_FAILURE);

This isn't specced to happen.  Of course what's specced to happen isn't self-consistent; I filed https://github.com/WICG/scroll-animations/issues/13 with a testcase that would hit this case.

Realistically, either this case should be specced to throw in the spec (and then the extension here should be whatever sort of extension the spec says to throw) or we should allow null scrollSource.  Throwing is probably fine.

::: dom/animation/ScrollTimeline.cpp:196
(Diff revision 5)
> +  {
> +    case eCSSUnit_Auto:
> +      aOutResult.AppendPrintf("auto");
> +      break;
> +    case eCSSUnit_Pixel:
> +      aOutResult.AppendPrintf("%d px", aValue.GetPixelLength());

Why the space after %d?  That seems odd.

Also, GetPixelLength() returns nscoord.  I really doubt that's what you want to be putting into the string here, in a web-exposed getter.

::: dom/animation/ScrollTimeline.cpp:199
(Diff revision 5)
> +      break;
> +    case eCSSUnit_Pixel:
> +      aOutResult.AppendPrintf("%d px", aValue.GetPixelLength());
> +      break;
> +    case eCSSUnit_Percent:
> +      aOutResult.AppendPrintf("%f %%", aValue.GetPercentValue());

AppendPrintf with the %f format is never what you want in web-exposed code: it will use a locale-depenent decimal separator, like printf itself.  Please use AppendFloat(), which does the right thing.

::: dom/animation/ScrollTimeline.cpp:207
(Diff revision 5)
> +      NS_WARNING("Unhandled CSS value type");
> +    }
> +}
> +
> +void
> +ScrollTimeline::GetStartScrollOffset(nsString& aOutResult) const {

This is the spec's <https://wicg.github.io/scroll-animations/#dom-scrolltimeline-startscrolloffset>, right?

In the spec, this is specified as:

> The attributes of the ScrollTimeline are
> initialized with the respective members of the
> options dictionary

and in the options dictionary startScrollOffset is allowed to be any string.  Why are we returning something other than what was passed into the constructor?

https://wicg.github.io/scroll-animations/#dom-scrolltimeline-startscrolloffset doesn't define an actual processing model or steps for the getter, so maybe this is just the spec not actually saying what it means, as opposed to our code not matching the spec?  Or maybe "initialized" in the constructor involves some processing as opposed to the normal meaning of "use those values"?  In either case, the spec needs clarification.

::: dom/animation/ScrollTimeline.cpp:213
(Diff revision 5)
> +  FormatValue(mComputedStartScrollOffset, aOutResult);
> +}
> +
> +void
> +ScrollTimeline::GetEndScrollOffset(nsString& aOutResult) const {
> +  FormatValue(mComputedEndScrollOffset, aOutResult);

Same issues as startScrollOffset here.

::: dom/animation/ScrollTimeline.cpp:219
(Diff revision 5)
> +}
> +
> +void
> +ScrollTimeline::QueryScrollValues()
> +{
> +  nsIScrollableFrame* frame = GetScrollFrame();

This will not process style changes and ensure frames are up to date.

That might be OK, depending on what the callers of this function expect.  But I just want us to be sure that's the behavior we want.  I did not carefully audit the callers.

::: dom/animation/ScrollTimeline.cpp:221
(Diff revision 5)
> +void
> +ScrollTimeline::QueryScrollValues()
> +{
> +  nsIScrollableFrame* frame = GetScrollFrame();
> +  mHaveFrame = (frame != nullptr);
> +  if (!mHaveFrame)

Curlies around the body, please.

More importantly, shouldn't this reset mMinScroll/mMaxScroll/mCurrentScroll to 0 or something?  Otherwise they will keep their last values once the frame goes away, right?

::: dom/animation/ScrollTimeline.cpp:295
(Diff revision 5)
> +ScrollTimeline::GetScrollFrame() const
> +{
> +  if (!mScrollSource) {
> +    return nullptr;
> +  }
> +  return nsLayoutUtils::FindScrollableFrameFor(

This seems a bit weird: We're creating a view id for mScrollSource, but the only thing that's later used for is to recover the mScrollSource pointer.  Why can we not have an API that just takes an Element* on nsLayoutUtils?

::: dom/animation/ScrollTimeline.cpp:296
(Diff revision 5)
> +{
> +  if (!mScrollSource) {
> +    return nullptr;
> +  }
> +  return nsLayoutUtils::FindScrollableFrameFor(
> +      nsLayoutUtils::FindOrCreateIDFor(mScrollSource.get()));

Why do you need the .get()?

::: dom/animation/ScrollTimeline.cpp:306
(Diff revision 5)
> +                                const nsString& aSpecifiedEndScrollOffset)
> +{
> +  // Parse scroll offsets
> +  nsIDocument* doc = mScrollSource->OwnerDoc();
> +  nsCSSParser parser(doc->CSSLoader());
> +  // Allow the same formats as for the "width" property.

This doesn't seem to match the spec (insofar as I can make sense of the spec), nor the other bits in this same file.

The rest of this file assumes that the mComputed* value can be "auto", pixel, or percent.  But the "width" property allows the following values: "auto", "initial", "inherit", "unset", "-moz-max-content", "-moz-min-content", "-moz-fit-content", "-moz-available", pixel, percent, calc() expression.

::: dom/animation/ScrollTimeline.cpp:340
(Diff revision 5)
> +
> +void
> +ScrollTimeline::UnregisterFromScrollSource()
> +{
> +  if (ScrollTimelineSet* scrollTimelineSet =
> +      ScrollTimelineSet::GetScrollTimelineSet(mScrollSource)) {

Please indent by two.

::: dom/animation/ScrollTimeline.cpp:360
(Diff revision 5)
> +
> +void
> +ScrollTimeline::UnregisterFromScrollSourceAsOwned()
> +{
> +  if (ScrollTimelineSet* scrollTimelineSet =
> +      ScrollTimelineSet::GetScrollTimelineSet(mScrollSource)) {

Please indent by two.

::: dom/animation/ScrollTimelineSet.cpp:76
(Diff revision 5)
> +}
> +
> +void
> +ScrollTimelineSet::AddScrollTimeline(dom::ScrollTimeline& aScrollTimeline)
> +{
> +  if (mAllScrollTimelines.Contains(&aScrollTimeline)) {

Do you really need this check?  Double-add should work fine, I'd think.

::: dom/animation/ScrollTimelineSet.cpp:86
(Diff revision 5)
> +}
> +
> +void
> +ScrollTimelineSet::RemoveScrollTimeline(dom::ScrollTimeline& aScrollTimeline)
> +{
> +  if (!mAllScrollTimelines.Contains(&aScrollTimeline)) {

Why do you need this check?  Removing it should be fine (a no-op) if it's not in there...

::: dom/animation/ScrollTimelineSet.cpp:96
(Diff revision 5)
> +}
> +
> +void
> +ScrollTimelineSet::AddOwnedScrollTimeline(dom::ScrollTimeline& aScrollTimeline)
> +{
> +  if (mOwnedScrollTimelines.Contains(&aScrollTimeline)) {

Again, is the Contains() check needed?

::: dom/animation/ScrollTimelineSet.cpp:106
(Diff revision 5)
> +}
> +
> +void
> +ScrollTimelineSet::RemoveOwnedScrollTimeline(dom::ScrollTimeline& aScrollTimeline)
> +{
> +  if (!mOwnedScrollTimelines.Contains(&aScrollTimeline)) {

And this Contains() check.

::: dom/base/FragmentOrElement.cpp:1378
(Diff revision 5)
>          nsIAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
>          for (uint32_t i = 0; effectProps[i]; ++i) {
>            tmp->DeleteProperty(effectProps[i]);
>          }
>        }
> +      tmp->DeleteProperty(nsGkAtoms::scrollTimelinesProperty);

This doesn't look right to me.  A non-HTML and non-SVG element can have a scroll timeline attached to it, hence have this property, right?  And that would leak because it's not being unlinked.

::: dom/base/FragmentOrElement.cpp:1964
(Diff revision 5)
>            if (effectSet) {
>              effectSet->Traverse(cb);
>            }
>          }
>        }
> +      if (ScrollTimelineSet* scrollTimelineSet =

Again, this will leak for non-HTML and non-SVG elements with a scroll timeline, since we would never traverse their stuff.

::: dom/base/nsNodeUtils.cpp:577
(Diff revision 5)
>      }
>  
> +    if (oldDoc != newDoc && aNode->IsElement()) {
> +      // Transfer element's scroll timelines from old document to new document.
> +      if (ScrollTimelineSet* scrollTimelineSet =
> +          ScrollTimelineSet::GetScrollTimelineSet(aNode->AsElement())) {

Please indent by two to make this clearly a continuation of the previous line's statement.

::: dom/base/nsNodeUtils.cpp:577
(Diff revision 5)
>      }
>  
> +    if (oldDoc != newDoc && aNode->IsElement()) {
> +      // Transfer element's scroll timelines from old document to new document.
> +      if (ScrollTimelineSet* scrollTimelineSet =
> +          ScrollTimelineSet::GetScrollTimelineSet(aNode->AsElement())) {

This is doing a hashtable lookup at this point, unconditionally.

Might be a good idea to have GetScrollTimelineSet() at least check for HasProperties() up front, since the common case is no properties.

::: dom/webidl/ScrollTimeline.webidl:33
(Diff revision 5)
> +interface ScrollTimeline : AnimationTimeline {
> +  readonly attribute Element sourceElement;
> +  readonly attribute ScrollDirection orientation;
> +  readonly attribute DOMString startScrollOffset;
> +  readonly attribute DOMString endScrollOffset;
> +  readonly attribute (double or ScrollTimelineAutoKeyword) timeRange;

This is a slightly weird API, but I think anything here would be somewhat weird, and at least people trying to do arithmetic on "auto" will hopefully notice quickly when their whole arithmetic expression is infected with strings and NaNs...

::: layout/base/nsRefreshDriver.cpp:1630
(Diff revision 5)
> +{
> +  if (!mPresContext) {
> +    return;
> +  }
> +
> +  nsCOMArray<nsIDocument> documents;

Is there a reason to not use an auto array (one with some inline storage) here?  If not, please do.

::: layout/generic/nsGfxScrollFrame.cpp:2085
(Diff revision 5)
>  
> +  // Register any scroll timelines associated with our content element, with
> +  // this scroll frame. The timeline registers itself when it's constructed,
> +  // but the frame can be reconstructed over the lifetime of the content
> +  // element.
> +  nsIScrollableFrame* sf = do_QueryFrame(mOuter);

Unfortunately, this is not quite equivalent to what ScrollTimeline itself does.

What ScrollTimeline does is it gets the element's primary frame and calls GetScrollTargetFrame() on it.  This will return non-null only if the primaty frame is a fieldset, text control, combobox, HTML/XULScrollFrame, or XUL menuframe.

An example mismatch: if you create a ScrollTimeline for a text input or <textarea>, it will find the scrollframe inside it.  But the mContent of that frame is not the input: it's the anonymous <div> inside that.  So this scrollframe code won't find the ScrollTimeline, afaict.

I _think_ that might be it, because for the fieldset/combobox cases the scrollframe has the fieldset/select (respectively) as its content, and the xul menu frame is not exposed to web content.  But the text control case is a problem.

::: layout/generic/nsGfxScrollFrame.cpp:2090
(Diff revision 5)
> +  nsIScrollableFrame* sf = do_QueryFrame(mOuter);
> +  MOZ_ASSERT(sf);
> +  if (nsIContent* content = mOuter->GetContent()) {
> +    if (content->IsElement()) {
> +      if (ScrollTimelineSet* scrollTimelineSet =
> +          ScrollTimelineSet::GetScrollTimelineSet(content->AsElement())) {

Indent by two, please.
Attachment #8816596 - Flags: review-
Flags: needinfo?(bzbarsky)
Comment on attachment 8851124 [details]
Bug 1321428 - Support timelines that do not track wallclock time.

https://reviewboard.mozilla.org/r/123522/#review126150

Looks good but I'd like to understand why the check for a null current/pending start time is needed towards the end of the patch.

::: layout/painting/nsDisplayList.cpp:662
(Diff revision 1)
> +    // This does not apply to scroll timelines, which don't track wallclock
> +    // time, and also don't have their start time updated with a wallclock time.
>      if (anim->PlayState() == AnimationPlayState::Pending &&
>          (anim->GetTimeline() &&
> -         !anim->GetTimeline()->TracksWallclockTime())) {
> +         ((anim->GetTimeline()->AsDocumentTimeline() || anim->GetCurrentOrPendingStartTime().IsNull()) &&
> +          !anim->GetTimeline()->TracksWallclockTime()))) {

This section needs to be wrapped to 80 chars. Also, it's not clear from the comment why the anim->GetCurrentOrPendingStartTime().IsNull() check is needed. Do we intend to say that pending scroll timelines will be triggered on the next tick? What are the cases where this check is needed?

Also, I notice the original code had some unnecessary braces (due to bug 1333539 where we changed the && to an ||). Perhaps we can make this rather large condition a little clearer somehow. e.g.

  if (anim->PlayState() == AnimationPlayState::Pending) {
    bool hasTimelineThatCannotUpdateStartTime =
      anim->GetTimeline() &&
      (anim->GetTimeline()->AsDocumentTimeline() ||
       anim->GetCurrentOrPendingStartTime().IsNull()) &&
      !anim->GetTimeline()->TracksWallclockTime();

    if (hasTimelineThatCannotUpdateStartTime) {
      continue;
    }
  }

But first I'd like to understand why the check for a null current/pending start time is needed.
Clearing my two remaining review requests now since, as discussed, part 1 already has a number of issues that need to be addressed and I'll wait for an answer to comment 91 before reviewing the other part. Also, I updated Web Animations to fix the language about active documents so I think we can probably update the scroll-linked animations spec in a similar way.

Incidentally, I un-bitrotted and demo-ed these patches at a developer meetup last Friday night which went well. We really need to work out the fill mode behavior though when you hit the end of the scroll range. That's probably blocked on me making updates to Web Animations to support timelines with infinite times.[1]

[1] https://github.com/w3c/web-animations/issues/174
Comment on attachment 8851124 [details]
Bug 1321428 - Support timelines that do not track wallclock time.

https://reviewboard.mozilla.org/r/123522/#review135532
Attachment #8851124 - Flags: review?(bbirtles)
Attachment #8818710 - Flags: review?(matt.woodrow)
I'd like to get back to working on this, but right now it's sitting in a TODO queue with several other things in front of it, so I'm going to unassign myself for the time being to reflect reality.
Assignee: botond → nobody
Depends on: 1467765
See Also: → 1676791
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: