Since bug 1356705 landed, scrolling in the tab strip with a MBP trackpad feels slow

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
15 days ago
2 days ago

People

(Reporter: mconley, Assigned: dao)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla57
All
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 days ago
I noticed this before landing bug 1356705, but I figured this is something we can probably sort out post-landing.

STR:

0) Wait until bug 1356705 is available in a Nightly, and run that Nightly. Or, if you're feeling adventurous, apply the patches yourself - though you'll need to apply it to a revision that already has the patches for bug 1349555 applied
1) Ensure that toolkit.scrollbox.smoothScroll is set to true (this is the default)
2) Add a bunch of tabs to your Firefox window so that the tab strip overflows and becomes scrollable
3) On your macOS trackpad, scroll up and down with your cursor over the strip

ER:

The tab strip should scroll at roughly the same speed as before bug 1356705 landed.

AR:

The tab strip seems to pixel scroll, so it's pretty slow - even with a high velocity scroll gesture.

This seems almost like the inverse of bug 764145. I wonder if that workaround can just be removed now?
(Reporter)

Comment 1

15 days ago
(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #0)
> This seems almost like the inverse of bug 764145. I wonder if that
> workaround can just be removed now?

Whoops. Wrong bug, and bad information. Ignore this part.

What I meant to say was that this sounds similar to bug 1320467.
Blocks: 1356705
See Also: → bug 1320467
(Assignee)

Comment 2

15 days ago
Created attachment 8893460 [details] [diff] [review]
WIP patch

I think this is because we're using smooth scroll behavior when we shouldn't. Something like this should fix it? I haven't tested this.
(Assignee)

Updated

15 days ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

15 days ago
Attachment #8893460 - Attachment is obsolete: true
(Assignee)

Comment 4

15 days ago
Hmm, looks like this makes scrollPosition unused.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

15 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08486de0dfd61ef4bffadf97bf3671126c7333c6
(Assignee)

Comment 7

15 days ago
(In reply to Dão Gottwald [::dao] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=08486de0dfd61ef4bffadf97bf3671126c7333c6

Apparently try is broken. Here's an older push with the previous patch revision:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58fe0d156a865ce4858f282ed885a01673453a4d

Comment 8

14 days ago
I won't have time to review this today, likely not before Monday - sorry.
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
tracking-firefox57: --- → ?
Keywords: regression
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Version: unspecified → Trunk
Comment hidden (mozreview-request)

Comment 10

14 days ago
mozreview-review
Comment on attachment 8893757 [details]
Bug 1387084 - Use instant scroll behavior when doing pixel scrolling.

https://reviewboard.mozilla.org/r/164860/#review170512

Mixing the inversion of the meaning of the parameter with a fix to how we use it is causing me headaches. I think this looks fine, but I had two notes below.

::: toolkit/content/widgets/scrollbox.xml:240
(Diff revision 3)
>            // scrolling, we need to complete it before starting a new one.
>            if (this._scrollTarget) {
>              let elements = this._getScrollableElements();
>              if (this._scrollTarget != elements[0] &&
>                  this._scrollTarget != elements[elements.length - 1])
>                this.ensureElementIsVisible(this._scrollTarget, false);

Shouldn't this be changed to `true` ?

::: toolkit/content/widgets/scrollbox.xml:272
(Diff revision 3)
>        <method name="scrollByPage">
>          <parameter name="pageDelta"/>
> -        <parameter name="aSmoothScroll"/>
> +        <parameter name="aInstant"/>

I couldn't find any callers for this. Am I missing something, or should we remove this?
Attachment #8893757 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 11

14 days ago
(In reply to :Gijs from comment #10)
> Comment on attachment 8893757 [details]
> Bug 1387084 - Use instant scroll behavior when doing pixel scrolling.
> 
> https://reviewboard.mozilla.org/r/164860/#review170512
> 
> Mixing the inversion of the meaning of the parameter with a fix to how we
> use it is causing me headaches.

Yeah, well... I figured now would be a good time to do this since I introduced another such parameter.

> ::: toolkit/content/widgets/scrollbox.xml:240
> (Diff revision 3)
> >            // scrolling, we need to complete it before starting a new one.
> >            if (this._scrollTarget) {
> >              let elements = this._getScrollableElements();
> >              if (this._scrollTarget != elements[0] &&
> >                  this._scrollTarget != elements[elements.length - 1])
> >                this.ensureElementIsVisible(this._scrollTarget, false);
> 
> Shouldn't this be changed to `true` ?

Looks like it. It never makes sense to pass 'false' here with the meaning changed.

> ::: toolkit/content/widgets/scrollbox.xml:272
> (Diff revision 3)
> >        <method name="scrollByPage">
> >          <parameter name="pageDelta"/>
> > -        <parameter name="aSmoothScroll"/>
> > +        <parameter name="aInstant"/>
> 
> I couldn't find any callers for this. Am I missing something, or should we
> remove this?

It looks like _distanceScroll should maybe call this. I can't say off-hand if there are differences and whether they would be intentional.
Comment hidden (mozreview-request)

Comment 13

14 days ago
mozreview-review
Comment on attachment 8893757 [details]
Bug 1387084 - Use instant scroll behavior when doing pixel scrolling.

https://reviewboard.mozilla.org/r/164860/#review170518

(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to :Gijs from comment #10)
> > Comment on attachment 8893757 [details]
> > Bug 1387084 - Use instant scroll behavior when doing pixel scrolling.
> > 
> > https://reviewboard.mozilla.org/r/164860/#review170512
> > 
> > Mixing the inversion of the meaning of the parameter with a fix to how we
> > use it is causing me headaches.
> 
> Yeah, well... I figured now would be a good time to do this since I
> introduced another such parameter.

Yep, I'm not criticizing the decision to do both in this bug, but separating the two things into 2 commits would have made it easier to understand. :-)


Can you file a followup to use or remove scrollByPage?
Attachment #8893757 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

14 days ago
Component: XP Toolkit/Widgets: XUL → XUL Widgets
Product: Core → Toolkit

Comment 14

14 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11d86d5c2159
Use instant scroll behavior when doing pixel scrolling. r=Gijs
(Assignee)

Updated

14 days ago
Blocks: 1387700
https://hg.mozilla.org/mozilla-central/rev/11d86d5c2159
Status: ASSIGNED → RESOLVED
Last Resolved: 13 days ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

12 days ago
Depends on: 1387861
status-firefox-esr52: --- → unaffected

Updated

12 days ago
Depends on: 1387721

Updated

12 days ago
Depends on: 1387763

Updated

12 days ago
Duplicate of this bug: 1387718
(Assignee)

Updated

11 days ago
No longer depends on: 1387721
Since this is fixed, no need to track it for 57.
tracking-firefox57: ? → ---
No longer depends on: 1387763
You need to log in before you can comment on or make changes to this bug.