Closed Bug 1054901 Opened 6 years ago Closed 6 years ago

Add scroll start/stop notifications from apz to scroll observers

Categories

(Core :: Panning and Zooming, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → pchang
Blocks: 1020801
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #8474428 - Flags: review?(roc)
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8474428 [details] [diff] [review]
WIP

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

Will work on the v2 to rename ScrollPositionChangeStarted/Stopped
Attachment #8474428 - Flags: review?(roc)
That sounds ok to me, but I'm not a layout peer so I'm not the one to make a decision on this.
Flags: needinfo?(bugmail.mozilla)
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #8474428 - Attachment is obsolete: true
Attachment #8474954 - Flags: review?(roc)
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #8474954 - Attachment is obsolete: true
Attachment #8474954 - Flags: review?(roc)
Attachment #8474955 - Flags: review?(roc)
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."
Attachment #8474955 - Flags: review?(roc) → review+
Attached patch WIP v3 (obsolete) — Splinter Review
Address review's comment.

Wait for try server result.
https://tbpl.mozilla.org/?tree=Try&rev=6aa9caceae7e
Attachment #8474955 - Attachment is obsolete: true
Attachment #8476420 - Flags: review+
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 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch WIP v4Splinter Review
Change the uuid and update func name to AsyncPanZoomStarted/Stopped.
Attachment #8476420 - Attachment is obsolete: true
Attachment #8477356 - Flags: review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35535bbbd6d1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.