Animation of scrolling tabstrip with scrollbutton is not smooth. It scrolls as the inchworm

VERIFIED FIXED in Firefox 57

Status

()

Toolkit
XUL Widgets
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: Alice0775 White, Assigned: mconley)

Tracking

({regression})

57 Branch
mozilla57
Unspecified
Windows 10
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Details

(Whiteboard: [photon-performance][qa-commented])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 months ago
Reproducible: always

Steps To Reproduce:
1. Open many 20-30 tabs
2. Scroll tabstrip with scrollbutton

Actual Results:
It scrolls one by one as the inchworm

Expected Results:
smooth

Updated

4 months ago
Flags: needinfo?(dao+bmo)

Updated

3 months ago
Duplicate of this bug: 1388070

Comment 2

3 months ago
More information from  bug 1387763 (resolved dup of bug 1388070 , further resolved dup of this bug. )

57.0a1 (2017-08-06) (32-bit) continues the jerky & slow behaviour with < or > tab-bar scrolling

Using mouse-wheel to scroll tabs remains acceptable smooth scroll. 

[ Keyboard arrows have no effect when tab is selected. I'm unsure of design spec. ] 


+ Sometimes if I hold the "<", the tabs will move, but when I release the "<", they jump a further few places

Playing around, I did note that using < or > can cause quite erratic jumps, worse under conditions of load = many browser windows & many tabs (+ maybe youtubes as well). 
Running under win10, on an oldish laptop Dell D520 (adequate, 4GB, Core2 @ 1.83GHz)
Works ok if I am cautious & careful). I suspect less of a problem for more capable rigs.

+ I think there was a config item at one time to smooth-scroll tab-bar (which still happens if I use mousewheel); I wonder if that is/was relevant.

+ Noted also, possibly irrelevant clue: horizontal scrolling inside a large rendered webpage seems to have similar behaviour, moves in jumps.

Comment 3

3 months ago
Also as :Gijs noted on 7Aug17:
"There's a regression range in bug 1388070, so duping there."  Q.v.

Comment 4

3 months ago
Looks like a photon-performance related regression.
Flags: needinfo?(mmucci)

Comment 5

3 months ago
Adding to Photon team for triage.
Flags: needinfo?(mmucci)
Whiteboard: [photon-performance] [triage]

Updated

3 months ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)

Updated

3 months ago
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-performance] [triage] → [reserve-photon-performance]

Updated

3 months ago
Flags: qe-verify? → qe-verify+

Updated

3 months ago
QA Contact: adrian.florinescu

Comment 6

3 months ago
Workaround userChrome.css that restores functionality at the expense of smooth scrolling:

> .tabbrowser-arrowscrollbox > scrollbox {
>   scroll-behavior: unset;
> }
Duplicate of this bug: 1387763
Duplicate of this bug: 1387760
See Also: → bug 1387718

Comment 9

3 months ago
Agreed, similar to bug 1387718

Dragging a tab to place it beyond the < >, movement is slower than a pulmonate gastropod.
Flags: needinfo?(mconley)
Stealing with dao's permission.

From dao in IRC:

"I think we basically just need to bring back _arrowScrollAnim. at least that would be the simplest way to undo the regression"
Assignee: dao+bmo → mconley
Flags: needinfo?(mconley)

Updated

3 months ago
Iteration: --- → 57.2 - Aug 29
Whiteboard: [reserve-photon-performance] → [photon-performance]
Comment hidden (mozreview-request)
Comment on attachment 8901371 [details]
Bug 1387130 - Increase the speed with which the scrollarrow scroll a scrollbox.

Is this too naive a fix? This certainly feels smoother to me, and it's simple as hell, but I suspect I'm missing some nuance.
Attachment #8901371 - Flags: feedback?(dao+bmo)
Comment on attachment 8901371 [details]
Bug 1387130 - Increase the speed with which the scrollarrow scroll a scrollbox.

This is faster for sure, but also makes it a bit harder to keep track of the scrolling. I think a bit slower but continuous scroll animation would be preferable.
Attachment #8901371 - Flags: feedback?(dao+bmo)

Updated

3 months ago
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Hey dao, when you said "bring back _arrowScrollAnim" (quoted you in IRC in comment 10), I assume you didn't mean essentially backing out bug 1356705, right?

Did you mean that we have a state object that knows which index we're supposed to be scrolling to, which updates off of a timer?
Flags: needinfo?(dao+bmo)
I meant specifically this object and the code using it:

https://hg.mozilla.org/mozilla-central/rev/3845af01ea9f#l2.373
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #15)
> I meant specifically this object and the code using it:
> 
> https://hg.mozilla.org/mozilla-central/rev/3845af01ea9f#l2.373

Right - but that object is powering itself using rAF and setting scrollPosition, which would re-introduce sync flushes. Do you mean to put the same object back, but power it using smooth scroll? Or go back to the old mechanism?
Flags: needinfo?(dao+bmo)
We can't use smooth scrolling there in a sensible way since we don't know the scroll destination until the user leaves the scroll button. (The line setting scrollPosition needs to call scrollByPixels instead, though.)
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Attachment #8901371 - Attachment is obsolete: true

Comment 19

2 months ago
mozreview-review
Comment on attachment 8905133 [details]
Bug 1387130 - Use original tabstrip scrolling behaviour when using scrollbuttons.

https://reviewboard.mozilla.org/r/176934/#review181940

::: toolkit/content/widgets/scrollbox.xml:721
(Diff revision 1)
>              this._scrollTimer.cancel();
>            this._mousedown = false;
>            this._scrollIndex = 0;
> +
> +          if (this.smoothScroll) {
> +            this._arrowScrollAnim.stop();

Is there a reason why you didn't restore _stopScroll to as it was before <https://hg.mozilla.org/mozilla-central/rev/3845af01ea9f#l2.425>?
Attachment #8905133 - Flags: review?(dao+bmo)
(Assignee)

Comment 20

2 months ago
mozreview-review-reply
Comment on attachment 8905133 [details]
Bug 1387130 - Use original tabstrip scrolling behaviour when using scrollbuttons.

https://reviewboard.mozilla.org/r/176934/#review181940

> Is there a reason why you didn't restore _stopScroll to as it was before <https://hg.mozilla.org/mozilla-central/rev/3845af01ea9f#l2.425>?

Thought I needed to keep that arrangement because of the browser_overflowScroll.js test failure that I wrestled with in bug 1356705 - but you're right, not necessary it seems. I've reverted that part too.
Comment hidden (mozreview-request)

Comment 22

2 months ago
mozreview-review
Comment on attachment 8905133 [details]
Bug 1387130 - Use original tabstrip scrolling behaviour when using scrollbuttons.

https://reviewboard.mozilla.org/r/176934/#review182020

Thanks!
Attachment #8905133 - Flags: review?(dao+bmo) → review+

Comment 23

2 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad5c7dbaa4d6
Use original tabstrip scrolling behaviour when using scrollbuttons. r=dao
(In reply to Dão Gottwald [::dao] from comment #22)
> Comment on attachment 8905133 [details]
> Bug 1387130 - Use original tabstrip scrolling behaviour when using
> scrollbuttons.
> 
> https://reviewboard.mozilla.org/r/176934/#review182020
> 
> Thanks!

And thank you for the fast review!
https://hg.mozilla.org/mozilla-central/rev/ad5c7dbaa4d6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
For QA:

Testing this one should be straight-forward. Ensure you have an overflowed tab strip, lots and lots of tabs, then click and hold the tab scrollbuttons. With this fix, it should be a reasonably constant quick speed in the scrolling. The bad case is when the tab strip "pulses" and goes one tab at a time.
Whiteboard: [photon-performance] → [photon-performance][qa-commented]
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected

Comment 27

2 months ago
The current incarnation works much better, as of old - here I have 57.0a1 (2017-09-12) (32-bit)

However, the corollary function -  moving a tab outside of current range - still displays the errant slow, hestitant behaviour.

I feel this is part of the same bug; do I have agreement?

To demonstrate:
get lots of tabs up, sufficient that the < scroll arrow appears.
lock on a tab with the mouse, attempt to move the tab left, outside of currently visible row of tabs
(the move initially seems to hit barriers, and) once it starts working the move is slow & jerky as per 'inchworm'

This is close to unusable [ and it is not possible to drag a tab from the drop-down list ].

Moving a tab within the visible range seems fluent & fine.

_____

Pursuant to my earlier merged report on this topic, wide web-pages also display less than optimal behaviour, but it may be acceptable.

Get a web-page and magnify it ctrl+ until it exceeds the screen-width, and (ideally but not always) the horizontal scroll bar appears.
Try to scroll laterally using the -> & <- keys

I find the scrolling is slow to start, then moves quite fast (enough) but with noticeable granularity.
(In reply to Bill his name from comment #27)
> However, the corollary function -  moving a tab outside of current range -
> still displays the errant slow, hestitant behaviour.
> 
> I feel this is part of the same bug; do I have agreement?

That sounds like bug 1387718.

Comment 29

2 months ago
Thanks,	Dão I agree with your https://bugzilla.mozilla.org/show_bug.cgi?id=1387130#c28

However I feel these visible bugs may all be rooted in the same place, and would be better fixed at root rather than with sticking plaster.

I've moved my additional comments 27 to 1387718.
Verified on: Windows 10x64(quantum ref.), Windows 7, Windows 8.1, Mac OSX 10.12, Ubuntu 16.04 -
with FF57 x64 9/15 and also covered with the pre-beta smoke tests.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+

Comment 31

2 months ago
(In reply to Adrian Florinescu [:AdrianSV] from comment #30)
> Verified on: Windows 10x64(quantum ref.), Windows 7, Windows 8.1, Mac OSX
> 10.12, Ubuntu 16.04 -
> with FF57 x64 9/15 and also covered with the pre-beta smoke tests.

Thanks,
probably now shifted to bug 1387718
You need to log in before you can comment on or make changes to this bug.