659549 - Tab bar scrolling using arrow buttons should use mozRequestAnimationFrame

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Michael Ventnor]

Attachment: Patch

A while ago we adopted the tab smooth scrolling animation to our animation API, when a tab was selected offscreen. However, the scrolling used when you hold down the arrow buttons on either side was not updated, thus making it seem jerky in comparison.

We should update that animation too.

Comment 1

[Michael Ventnor]

Comment on attachment 534976 [details] [diff] [review]
Patch

Huh, the new bug form no longer lets you mark attachments as patches.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 534976 [details] [diff] [review]
Patch

>+      <field name="_arrowSmoothScroll"><![CDATA[({

"_arrowSmoothScrollAnim" or just "_arrowScrollAnim" for consistency with _scrollAnim?

>+        stop: function arrowSmoothScroll_stop() {
>+          window.removeEventListener("MozBeforePaint", this, false);
>+          this.scrollbox.scrollByIndex(this.scrollbox._scrollIndex);
>+          this.scrollbox._scrollIndex = 0;

I think I'd prefer to keep the scrollByIndex call and _scrollIndex = 0 in the _stopScroll method.

>+        handleEvent: function arrowSmoothScroll_handleEvent(event) {
>+          const scrollIndex = this.scrollbox._scrollIndex;
>+          const timePassed = event.timeStamp - this.lastFrameTime;
>+          this.lastFrameTime = event.timeStamp;
>+          
>+          const scrollDelta = 0.5 * timePassed * scrollIndex;
>+          this.scrollbox.scrollPosition += scrollDelta;
>+
>+          if (scrollIndex != 0)
>+            window.mozRequestAnimationFrame();
>+        }

Why is "if (scrollIndex != 0)" needed?

Comment 3

[Michael Ventnor]

Attachment: Patch 2

(In reply to comment #2)
> >+        handleEvent: function arrowSmoothScroll_handleEvent(event) {
> >+          const scrollIndex = this.scrollbox._scrollIndex;
> >+          const timePassed = event.timeStamp - this.lastFrameTime;
> >+          this.lastFrameTime = event.timeStamp;
> >+          
> >+          const scrollDelta = 0.5 * timePassed * scrollIndex;
> >+          this.scrollbox.scrollPosition += scrollDelta;
> >+
> >+          if (scrollIndex != 0)
> >+            window.mozRequestAnimationFrame();
> >+        }
> 
> Why is "if (scrollIndex != 0)" needed?

I thought that would be necessary to stop requesting frames when we stop the scrolling by setting the index to 0. But looking at it again, it does seem unnecessary so I removed it.

Comment 4

[Michael Ventnor]

http://hg.mozilla.org/mozilla-central/rev/f4321cdcef37