Closed Bug 1387084 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: UI Widgets, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: mconley, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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?
(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: → 1320467
Attached patch WIP patch (obsolete) — Splinter Review
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: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8893460 - Attachment is obsolete: true
Hmm, looks like this makes scrollPosition unused.
(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
I won't have time to review this today, likely not before Monday - sorry.
Keywords: regression
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Version: unspecified → Trunk
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)
(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 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+
Component: XP Toolkit/Widgets: XUL → XUL Widgets
Product: Core → Toolkit
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11d86d5c2159
Use instant scroll behavior when doing pixel scrolling. r=Gijs
Blocks: 1387700
https://hg.mozilla.org/mozilla-central/rev/11d86d5c2159
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1387861
Depends on: 1387721
Depends on: 1387763
No longer depends on: 1387721
Since this is fixed, no need to track it for 57.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: