1054901 - Add scroll start/stop notifications from apz to scroll observers

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Peter Chang[:pchang]]

Based on bug 1020801 comment 19, create this bug to add scroll start/stop notification from apz and then pass these events to scroll observers.

Comment 1

[Peter Chang[:pchang]]

Attachment: WIP

This patch tries to add scroll start/stop notification from apz. And these notification will be used to show/hide the selection bubble from bug 1020801

Comment 2

[Peter Chang[:pchang]]

Comment on attachment 8474428 [details] [diff] [review]
WIP

Review of attachment 8474428 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIScrollObserver.h
@@ +21,5 @@
>     * Called when the scroll position of some element has changed.
>     */
>    virtual void ScrollPositionChanged() = 0;
> +  virtual void ScrollPositionChangeStarted(){};
> +  virtual void ScrollPositionChangeStopped(){};

Have empty implementation here because not every scroll observer want to listen scrollStart/Stop events.
Or it requires to declare all functions as pure virtual function.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Note that the APZStateChange::TransformBegin|End notifications are fired any time there is a change to the transform. This doesn't necessarily mean that the scroll position is changing; it might be the zoom that is changing while the scroll position stays the same. So it might be better to rename your functions accordingly.

Comment 4

[Peter Chang[:pchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Note that the APZStateChange::TransformBegin|End notifications are fired any
> time there is a change to the transform. This doesn't necessarily mean that
> the scroll position is changing; it might be the zoom that is changing while
> the scroll position stays the same. So it might be better to rename your
> functions accordingly.

Thanks for the note. I also noticed that APZStateChange::TransformEnd is received but no scroll position changed events for scroll snapback case. How about the ScrollViewChangeStarted/ScrollViewChangeStopped?

Comment 5

[Peter Chang[:pchang]]

Comment on attachment 8474428 [details] [diff] [review]
WIP

Review of attachment 8474428 [details] [diff] [review]:
-----------------------------------------------------------------

Will work on the v2 to rename ScrollPositionChangeStarted/Stopped

Comment 6

[Kartikaya Gupta (email:[e-mail])]

That sounds ok to me, but I'm not a layout peer so I'm not the one to make a decision on this.

Comment 7

[Peter Chang[:pchang]]

Attachment: WIP v2

Rename funcation to ScrollViewChangeStart/Stop based on comment 3.

roc, this patch is trying to add the notification of scroll view changes(transform changes) from APZ and dispatch to scroll oberservers.

The selection bubble in bug 1020801 will use the new ScrollViewChangeStart/Stop callback to show/hide itself.

Comment 8

[Peter Chang[:pchang]]

Attachment: WIP v2

Attached to correct patch.

Rename funcation to ScrollViewChangeStart/Stop based on comment 3.

roc, this patch is trying to add the notification of scroll view changes(transform changes) from APZ and dispatch to scroll oberservers.

The selection bubble in bug 1020801 will use the new ScrollViewChangeStart/Stop callback to show/hide itself.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8474955 [details] [diff] [review]
WIP v2

Review of attachment 8474955 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIScrollObserver.h
@@ +24,5 @@
> +
> +  /**
> +   * Called when the scroll view starts to change, i.e. like the transform
> +   */
> +  virtual void ScrollViewChangeStarted(){};

This isn't really clear enough.

Let's call the functions AsyncPanZoomStarted/Stopped and the comment should say
"Called when an async panning/zooming transform has started being applied."
"Called when an async panning/zooming transform is no longer applied."

Comment 10

[Peter Chang[:pchang]]

Attachment: WIP v3

Address review's comment.

Wait for try server result.
https://tbpl.mozilla.org/?tree=Try&rev=6aa9caceae7e

Comment 11

[Kartikaya Gupta (email:[e-mail])]

The updated patch didn't rename the functions as roc requested:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Let's call the functions AsyncPanZoomStarted/Stopped and ...

Comment 12

[(no longer active)]

Comment on attachment 8476420 [details] [diff] [review]
WIP v3

Review of attachment 8476420 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIScrollObserver.h
@@ +29,5 @@
> +
> +  /**
> +   * Called when an async panning/zooming transform is no longer applied.
> +   */
> +  virtual void ScrollViewChangeStopped(){};

Note that you need to change NS_ISCROLLOBSERVER_IID because of the functions you've added to this interface.

Comment 13

[Peter Chang[:pchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> The updated patch didn't rename the functions as roc requested:
> 
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > Let's call the functions AsyncPanZoomStarted/Stopped and ...

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> Comment on attachment 8476420 [details] [diff] [review]
> WIP v3
> 
> Review of attachment 8476420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsIScrollObserver.h
> @@ +29,5 @@
> > +
> > +  /**
> > +   * Called when an async panning/zooming transform is no longer applied.
> > +   */
> > +  virtual void ScrollViewChangeStopped(){};
> 
> Note that you need to change NS_ISCROLLOBSERVER_IID because of the functions
> you've added to this interface.

Thanks for the note, I will re-upload patch again.

Comment 14

[Peter Chang[:pchang]]

Attachment: WIP v4

Change the uuid and update func name to AsyncPanZoomStarted/Stopped.

Comment 15

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #10)
> Created attachment 8476420 [details] [diff] [review]
> WIP v3
> 
> Address review's comment.
> 
> Wait for try server result.
> https://tbpl.mozilla.org/?tree=Try&rev=6aa9caceae7e

Previous try is all green.
And he difference between v3 and v4 are the uuid and function name changes. It won't impact the testing result.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35535bbbd6d1

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/35535bbbd6d1