Closed Bug 1387130 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: UI Widgets, defect, P1)

57 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: alice0775, Assigned: mconley)

References

Details

(Keywords: regression, Whiteboard: [photon-performance][qa-commented])

Attachments

(1 file, 1 obsolete file)

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
Flags: needinfo?(dao+bmo)
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.
Also as :Gijs noted on 7Aug17:
"There's a regression range in bug 1388070, so duping there."  Q.v.
Looks like a photon-performance related regression.
Flags: needinfo?(mmucci)
Adding to Photon team for triage.
Flags: needinfo?(mmucci)
Whiteboard: [photon-performance] [triage]
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-performance] [triage] → [reserve-photon-performance]
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Workaround userChrome.css that restores functionality at the expense of smooth scrolling:

> .tabbrowser-arrowscrollbox > scrollbox {
>   scroll-behavior: unset;
> }
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)
Iteration: --- → 57.2 - Aug 29
Whiteboard: [reserve-photon-performance] → [photon-performance]
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)
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)
Attachment #8901371 - Attachment is obsolete: true
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)
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 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+
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
Closed: 7 years ago
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]
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.
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
Flags: qe-verify+
(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.