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)
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?
Reporter | ||
Comment 1•7 years 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.
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8893460 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
Hmm, looks like this makes scrollPosition unused.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08486de0dfd61ef4bffadf97bf3671126c7333c6
Assignee | ||
Comment 7•7 years 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•7 years ago
|
||
I won't have time to review this today, likely not before Monday - sorry.
Updated•7 years ago
|
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•7 years 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•7 years 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•7 years 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•7 years ago
|
Component: XP Toolkit/Widgets: XUL → XUL Widgets
Product: Core → Toolkit
Comment 14•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/11d86d5c2159 Use instant scroll behavior when doing pixel scrolling. r=Gijs
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11d86d5c2159
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Since this is fixed, no need to track it for 57.
tracking-firefox57:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•