28ms uninterruptible reflow at set_scrollPosition@chrome://global/content/bindings/scrollbox.xml:207:13

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [ohnoreflow][qf:p1][photon-performance][qa-commented])

Attachments

(4 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
kip
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
19.25 KB, patch
Details | Diff | Splinter Review
Here's the stack:

set_scrollPosition@chrome://global/content/bindings/scrollbox.xml:207:13
scrollAnim_handleEvent@chrome://global/content/bindings/scrollbox.xml:396:11

This happened while I was scrolling around in the tab strip.

Assuming that scroll position is being _written_ to only, I wonder if we can do this via element.scrollBy(0, <amount>, { behavior: "smooth" }), which is GPU accelerated and shouldn't result in a layout flush.
Component: Untriaged → XUL Widgets
Flags: needinfo?(dao+bmo)
Product: Firefox → Toolkit
See Also: → bug 1356662
Flags: qe-verify?
Priority: -- → P2
This mostly works, but when I open tabs quickly, the scroll position seems to get stuck because setting scrollPosition or calling scrollIntoView becomes ineffective. Any idea what's going on here?
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Attachment #8858605 - Flags: feedback?(enndeakin)
Priority: P2 → P1
Blocks: 1356662
See Also: bug 1356662
Flags: qe-verify? → qe-verify-
Blocks: 1357061
Duplicate of this bug: 785284
(Assignee)

Updated

2 years ago
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Duplicate of this bug: 1358425

Comment 4

2 years ago
Sorry! I commented on the wrong bug accidentally :)

is bug 854054 a duplicate of this? (not the other bug which was already a duplicate of this one, d'oh!)

Comment 5

2 years ago
I don't know much about scrolling to be able to answer this offhand.
Comment on attachment 8858605 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling

(In reply to Dão Gottwald [::dao] from comment #1)
> Created attachment 8858605 [details] [diff] [review]
> Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling
> 
> This mostly works, but when I open tabs quickly, the scroll position seems
> to get stuck because setting scrollPosition or calling scrollIntoView
> becomes ineffective. Any idea what's going on here?

masayuki, maybe you have ideas what's broken here or what I could be doing wrong?
Attachment #8858605 - Flags: feedback?(masayuki)
Comment on attachment 8858605 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling

Hmm, I really don't know... Anyway, both of them reach ScrollFrameHelper::ScrollToWithOrigin(), though.

What happens if you comment out calls of scrollIntoView? scrollIntoView's implementation is really complicated.

If you can reproduce it without scrollIntoView, it could be that scrollPosition is called before new tab expands overflow rect. Then, scroll destination might be clamped with size of its scroll area.

According to the commit log, tnikkel might now something about scrollable frame behavior.
Attachment #8858605 - Flags: feedback?(masayuki)
Comment on attachment 8858605 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> What happens if you comment out calls of scrollIntoView? scrollIntoView's
> implementation is really complicated.

scrollIntoView isn't called when opening new tabs in succession since I added this special case:

>+          } else if (index == elements.length - 1) {
>+            //XXX special case for Firefox tabs using negative margin, remove after bug 1349555
>+            this.scrollPosition = this.scrollSize;

When I hadn't added that special case and called scrollIntoView unconditionally, the same thing happened, i.e. at some point new tabs wouldn't scroll into view anymore.

> If you can reproduce it without scrollIntoView, it could be that
> scrollPosition is called before new tab expands overflow rect. Then, scroll
> destination might be clamped with size of its scroll area.

If the overflow rect was just lagging, I'd expect that opening another tab would at least scroll to the previously opened tab, but even that doesn't happen once scrolling gets stuck.
(In reply to Dão Gottwald [::dao] from comment #1)
> Created attachment 8858605 [details] [diff] [review]
> Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling
> 
> This mostly works, but when I open tabs quickly, the scroll position seems
> to get stuck because setting scrollPosition or calling scrollIntoView
> becomes ineffective. Any idea what's going on here?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> Comment on attachment 8858605 [details] [diff] [review]
> Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling
> 
> Hmm, I really don't know... Anyway, both of them reach
> ScrollFrameHelper::ScrollToWithOrigin(), though.
> 
> What happens if you comment out calls of scrollIntoView? scrollIntoView's
> implementation is really complicated.
> 
> If you can reproduce it without scrollIntoView, it could be that
> scrollPosition is called before new tab expands overflow rect. Then, scroll
> destination might be clamped with size of its scroll area.
> 
> According to the commit log, tnikkel might now something about scrollable
> frame behavior.

tnikkel, any ideas?
Flags: needinfo?(tnikkel)
Posted patch improve the situation (obsolete) — Splinter Review
It seems as though we init a mAsyncSmoothMSDScroll, but it never finishes (not sure why). So the mAsyncSmoothMSDScroll sticks around, the next scroll changes the destination on that mAsyncSmoothMSDScroll, but the scrollrange on it never changes after it is created. The destination is clamped to that range and so we never complete that scroll either.

Here's a patch that improves things, but they are still not perfect.

BTW, scrollIntoView also flushes

https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/layout/base/PresShell.cpp#3495
Flags: needinfo?(tnikkel) → needinfo?(kgilbert)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> BTW, scrollIntoView also flushes

Hopefully not with every frame, though? Because that's what we currently do. I don't think there's a way around flushing layout at the beginning of the animation one way ore another.
See Also: → bug 1358453

Comment 12

2 years ago
Comment on attachment 8858605 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling

I won't have time to do this. mstange would probably be better anyway.
Attachment #8858605 - Flags: feedback?(enndeakin)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Created attachment 8862756 [details] [diff] [review]
> improve the situation
> 
> It seems as though we init a mAsyncSmoothMSDScroll, but it never finishes
> (not sure why). So the mAsyncSmoothMSDScroll sticks around, the next scroll
> changes the destination on that mAsyncSmoothMSDScroll, but the scrollrange
> on it never changes after it is created. The destination is clamped to that
> range and so we never complete that scroll either.
> 
> Here's a patch that improves things, but they are still not perfect.
> 
> BTW, scrollIntoView also flushes
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 0b77ed3f26c5335503bc16e85b8c067382e7bb1e/layout/base/PresShell.cpp#3495

mstange, can you help?
Flags: needinfo?(mstange)
I don't have too much time for this at the moment. Can you create an HTML testcase that also hits this bug?
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #14)
> I don't have too much time for this at the moment. Can you create an HTML
> testcase that also hits this bug?

I tried to create a simple one from scratch, that didn't work. Probably have to start with the tab strip, remove stuff that's not needed to reproduce the issue, and then somehow convert this from XBL + XUL to HTML, which might be quite a big effort :/
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Created attachment 8862756 [details] [diff] [review]
> improve the situation
> 
> It seems as though we init a mAsyncSmoothMSDScroll, but it never finishes
> (not sure why). So the mAsyncSmoothMSDScroll sticks around, the next scroll
> changes the destination on that mAsyncSmoothMSDScroll, but the scrollrange
> on it never changes after it is created. The destination is clamped to that
> range and so we never complete that scroll either.
> 
> Here's a patch that improves things, but they are still not perfect.
> 
> BTW, scrollIntoView also flushes
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 0b77ed3f26c5335503bc16e85b8c067382e7bb1e/layout/base/PresShell.cpp#3495

The conditions that the MSD scroll may not complete I can think of:
- It stops receiving refresh driver ticks to call mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::Simulate.
- If the refresh driver ticks are extremely delayed, the simulation might not converge at the destination point.  This could be measured by the maximum aDeltaTime value passed into AsyncSmoothMSDScroll::Simulate().
- The MSD parameters are under-damped, resulting in a long oscillation around the destination point.  This should be visible if occurring, as we terminate the animation when the remaining movement reaches a sub-pixel level.
- The range of the scroll changes during the animation such that the destination point is out of range.

The logic that determines when the animation is complete is here:

mozilla::layers::AxisPhysicsMSDModel::IsFinished

There is a comment in that function describing in more detail the conditions for ending the animation.

I'd recommend adding a [conditional] breakpoint within that function or at the call sites to see if the condition is being met when expected.
Flags: needinfo?(kgilbert)
It's been a while since I've looked at this, so hopefully this is not too naive a suggestion...

Can you see any layerization changes happening right after the scroll stops?

My instinct would be to try watching what is happening with LayerScope:

https://wiki.mozilla.org/Platform/GFX/LayerScope

Perhaps some of these hitches could be eliminated by forcing layerization for things that are expected to scroll.
(Assignee)

Updated

2 years ago
Blocks: 1363755
No longer blocks: 1348289
(In reply to :kip (Kearwood Gilbert) from comment #16)
> - The range of the scroll changes during the animation such that the
> destination point is out of range.

AFAIK, this is definitely possible. Since the scroll happens over a period of time there is nothing to stop the page from changing the size of the contents of the scroll frame. Does the current code deal with that?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 19

2 years ago
I tested Dao's patch in the overflowed state, and this brings us down from 1 reflow per requestAnimationFrame, to a predictable 3 reflows per tab open. I think that's a huge improvement.
(Assignee)

Comment 20

2 years ago
Sorry, scratch that - there are more than 3 reflows per tab open in the overflowed state because of this stack from scroll events:

get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:182:1
_updateScrollButtonsDisabledState@chrome://global/content/bindings/scrollbox.xml:430:1
onxblscroll@chrome://global/content/bindings/scrollbox.xml:602:1

But I believe the amount is still predictable[1], which is better for the reflow tests.

[1]: The old mechanism, which uses requestAnimationFrame, is quite difficult to write reflow tests for, because the number of reflows is proportional to the number of times requestAnimationFrame is fired during the tab open, which is currently non-deterministic.
(In reply to Mike Conley (:mconley) from comment #20)
> Sorry, scratch that - there are more than 3 reflows per tab open in the
> overflowed state because of this stack from scroll events:
> 
> get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:182:1
> _updateScrollButtonsDisabledState@chrome://global/content/bindings/scrollbox.
> xml:430:1
> onxblscroll@chrome://global/content/bindings/scrollbox.xml:602:1

That's bug 1358453, I'm gonna comment over there.
Duplicate of this bug: 1356662
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> (In reply to :kip (Kearwood Gilbert) from comment #16)
> > - The range of the scroll changes during the animation such that the
> > destination point is out of range.
> 
> AFAIK, this is definitely possible. Since the scroll happens over a period
> of time there is nothing to stop the page from changing the size of the
> contents of the scroll frame. Does the current code deal with that?

The current code handles this outside of AxisPhysicsMSDModel.  Effectively, if there is any overscroll caused by changing the size of the scroll range, the animation is replaced with a new one with a call to AsyncPanZoomController::StartOverscrollAnimation.
Flags: needinfo?(kgilbert)
(Assignee)

Updated

2 years ago
Blocks: 1367797
No longer blocks: 1356662, 1357061
(In reply to Mike Conley (:mconley) from comment #20)
> Sorry, scratch that - there are more than 3 reflows per tab open in the
> overflowed state because of this stack from scroll events:
> 
> get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:182:1
> _updateScrollButtonsDisabledState@chrome://global/content/bindings/scrollbox.
> xml:430:1
> onxblscroll@chrome://global/content/bindings/scrollbox.xml:602:1

How do things look now that bug 1358453 is resolved?
Flags: needinfo?(mconley)
(Assignee)

Comment 26

2 years ago
It's better, yes!
Flags: needinfo?(mconley)
Unassigning -- somebody will have to dig deeper to figure out what's breaking this patch on (presumably) the layout side. NI mconley, maybe he can help find an owner for this.
Assignee: dao+bmo → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mconley)
Priority: P1 → P2
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
(Assignee)

Comment 30

2 years ago
(In reply to :kip (Kearwood Gilbert) from comment #23)
> The current code handles this outside of AxisPhysicsMSDModel.  Effectively,
> if there is any overscroll caused by changing the size of the scroll range,
> the animation is replaced with a new one with a call to
> AsyncPanZoomController::StartOverscrollAnimation.

What about the non-APZ case? Remember that we don't (currently) have APZ enabled in the parent process for the tab strip. So we're using the stuff from bug 1026023.
Flags: needinfo?(mconley) → needinfo?(kgilbert)
(Assignee)

Comment 31

2 years ago
Hey tnikkel, attachment 8862756 [details] [diff] [review] seems to fix the issue completely for me (though we can probably use the scrollRange already calculated here: http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/generic/nsGfxScrollFrame.cpp#2249

(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Created attachment 8862756 [details] [diff] [review]
> improve the situation
> 
> Here's a patch that improves things, but they are still not perfect.
> 

What exactly is not perfect? Are there non-APZ smooth scrolling overscroll cases here that aren't being captured?
Flags: needinfo?(tnikkel)
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #31)
> Hey tnikkel, attachment 8862756 [details] [diff] [review] seems to fix the
> issue completely for me (though we can probably use the scrollRange already
> calculated here:
> http://searchfox.org/mozilla-central/rev/
> 01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/generic/nsGfxScrollFrame.
> cpp#2249
> 
> (In reply to Timothy Nikkel (:tnikkel) from comment #10)
> > Created attachment 8862756 [details] [diff] [review]
> > improve the situation
> > 
> > Here's a patch that improves things, but they are still not perfect.
> > 
> 
> What exactly is not perfect? Are there non-APZ smooth scrolling overscroll
> cases here that aren't being captured?

Sorry, I don't remember anymore. I think the scrolling was still "glitchy", ie it jumped around. IIRC it seems like there is a fundamental bug in that code if the size of the scrollable content changes while a scroll is in progress and I think there will need to be more changes to overhaul the code to deal with that situation. I never understood that code fully so I can't say for sure.
Flags: needinfo?(tnikkel)
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #31)
> Hey tnikkel, attachment 8862756 [details] [diff] [review] seems to fix the
> issue completely for me (though we can probably use the scrollRange already
> calculated here:
> http://searchfox.org/mozilla-central/rev/
> 01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/generic/nsGfxScrollFrame.
> cpp#2249
> 
> (In reply to Timothy Nikkel (:tnikkel) from comment #10)
> > Created attachment 8862756 [details] [diff] [review]
> > improve the situation
> > 
> > Here's a patch that improves things, but they are still not perfect.
> > 
> 
> What exactly is not perfect? Are there non-APZ smooth scrolling overscroll
> cases here that aren't being captured?

That patch didn't seem to help when I tried it on Linux. It's possible that I messed something up (e.g. I had to disable artifact builds since I normally don't do full builds) or maybe things have changed in the last couple of weeks.
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8862756 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8884245 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1385034
(Assignee)

Updated

2 years ago
Attachment #8890988 - Flags: review?(jaws)
Attachment #8890989 - Flags: review?(kgilbert)
Attachment #8890990 - Flags: review?(mconley)
Attachment #8890991 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

2 years ago
mozreview-review
Comment on attachment 8890989 [details]
Bug 1356705 - Update range when an async smooth scroll is interrupted with a new scroll.

https://reviewboard.mozilla.org/r/162172/#review167538

LGTM, good find, thanks!
Attachment #8890989 - Flags: review?(kgilbert) → review+
Comment on attachment 8890988 [details]
Bug 1356705 - Modernize browser_overflowScroll.js and move into the tabs/ testing directory.

https://reviewboard.mozilla.org/r/162170/#review167558
Attachment #8890988 - Flags: review?(jaws) → review+

Comment 45

2 years ago
mozreview-review
Comment on attachment 8890990 [details]
Bug 1356705 - Use CSS smooth scroll when smooth scrolling a XUL scrollbox.

https://reviewboard.mozilla.org/r/162174/#review167758

::: commit-message-a0d4f:1
(Diff revision 2)
> +Bug 1356705 - Use CSS smooth scroll when smooth scrolling a XUL scrollbox. r?mconley

The user info is messed up, both the encoding and the email address...

Comment 46

2 years ago
mozreview-review
Comment on attachment 8890991 [details]
Bug 1356705 - Fixes to fold on to previous patch.

https://reviewboard.mozilla.org/r/162176/#review167810

::: toolkit/content/widgets/scrollbox.xml:789
(Diff revision 2)
>              this._scrollTimer.cancel();
>            this._mousedown = false;
> -          if (!this._scrollIndex)
> +          if (!this._scrollIndex || !this.smoothScroll)
>              return;
>  
>            this.scrollByIndex(this._scrollIndex);

Hmm, what's the idea behind this change? smoothScroll or not shouldn't make a difference here anymore, I think, but the scrollByIndex call may be completely obsolete now.
(Assignee)

Comment 47

2 years ago
mozreview-review-reply
Comment on attachment 8890991 [details]
Bug 1356705 - Fixes to fold on to previous patch.

https://reviewboard.mozilla.org/r/162176/#review167810

> Hmm, what's the idea behind this change? smoothScroll or not shouldn't make a difference here anymore, I think, but the scrollByIndex call may be completely obsolete now.

Without this, browser_overflowScroll.js fails. That test disables smooth scrolling, and on mouseup, `_stopScroll` is called. Unless we bail out when not smooth scrolling, we seem to end up scrolling another tab over.
(Assignee)

Comment 48

2 years ago
mozreview-review-reply
Comment on attachment 8890991 [details]
Bug 1356705 - Fixes to fold on to previous patch.

https://reviewboard.mozilla.org/r/162176/#review167810

> Without this, browser_overflowScroll.js fails. That test disables smooth scrolling, and on mouseup, `_stopScroll` is called. Unless we bail out when not smooth scrolling, we seem to end up scrolling another tab over.

Okay, I played around with this, and I think you're right - the scrollByIndex isn't needed anymore. I'll just remove it.
(Assignee)

Comment 49

2 years ago
mozreview-review-reply
Comment on attachment 8890990 [details]
Bug 1356705 - Use CSS smooth scroll when smooth scrolling a XUL scrollbox.

https://reviewboard.mozilla.org/r/162174/#review167758

> The user info is messed up, both the encoding and the email address...

mingw32 ate some characters up it seems. :/ Fixed!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 54

2 years ago
mozreview-review
Comment on attachment 8890990 [details]
Bug 1356705 - Use CSS smooth scroll when smooth scrolling a XUL scrollbox.

https://reviewboard.mozilla.org/r/162174/#review167986

Really pleased to see this JS-driven stuff come out. Thanks!
Attachment #8890990 - Flags: review?(mconley) → review+
Clearing old NI, as have already reviewed patch for fix.  Please NI again if you have more questions, thanks!
Flags: needinfo?(kgilbert)

Comment 56

2 years ago
mozreview-review
Comment on attachment 8890991 [details]
Bug 1356705 - Fixes to fold on to previous patch.

https://reviewboard.mozilla.org/r/162176/#review168076

::: toolkit/content/widgets/scrollbox.xml:288
(Diff revision 3)
> +            let prop = vertical ? "top" : "left";
> +            opts[prop] = amountToScroll;
> +
> +            this._scrollbox.scrollBy(opts);
>            } else {
> -            element.scrollIntoView({ behavior: aSmoothScroll == false ? "instant" : "auto" });
> +           element.scrollIntoView({ behavior: aSmoothScroll == false ? "instant" : "auto" });

messed up indent, please revert
Attachment #8890991 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 57

2 years ago
Thanks. I think I'm going to try to land this stuff after the uplift.
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment on attachment 8890991 [details]
Bug 1356705 - Fixes to fold on to previous patch.

If bug 1349555 sticks, this part won't be needed. We'll also need to rebase my patch for this (should be easy): https://hg.mozilla.org/integration/autoland/diff/c9594a10a88a/toolkit/content/widgets/scrollbox.xml
(In reply to Dão Gottwald [::dao] from comment #58)
> Comment on attachment 8890991 [details]
> Bug 1356705 - Fixes to fold on to previous patch.
> 
> If bug 1349555 sticks, this part won't be needed.

Well, except for the _stopScroll part.
(Assignee)

Comment 61

2 years ago
I'll wait for the square tab stuff to land and stick before landing this to avoid dao and I bitrotting each other.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8890991 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1349555
(Assignee)

Comment 65

2 years ago
Square tabs have landed and stuck, so I'm going to land this soon.

One thing to point out is that, at least on my MBP touchpad, scrolling the tab strip seems a bit slower now. It looks like we're doing pixel scrolling there. Doesn't seem to affect the mouse case. I suspect we'll need to tune the wheel event handler now that we're using smooth-scroll in the tab strip. I'm going to file that as a follow-up bug.

Comment 66

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c2d4c05b890
Modernize browser_overflowScroll.js and move into the tabs/ testing directory. r=jaws
https://hg.mozilla.org/integration/autoland/rev/39f353cad1a4
Update range when an async smooth scroll is interrupted with a new scroll. r=kip
https://hg.mozilla.org/integration/autoland/rev/3845af01ea9f
Use CSS smooth scroll when smooth scrolling a XUL scrollbox. r=mconley
Flags: qe-verify- → qe-verify+
QA Contact: adrian.florinescu
(Assignee)

Comment 67

2 years ago
Comment for QA:

So this is changing the smooth scrolling mechanism in the front-end for several scrollable UI elements, with a particular focus on the tab strip when it is overflowed (scrollable). We're going to want to compare its behaviour from before and after it landed to see what the differences are and whether or not we can live with them.

I already noticed that macOS trackpads result in slower scrolling in comment 65, and will be filing a follow-up, but it might be worth checking other scrolling devices as well, along with touch scrolling.

To be clear, this is about scrolling _just_ in the UI, and not in web content, which doesn't use the same scrolling mechanism.

Places to look at scrolling behaviour are:

1) The tab strip
2) The Library window, particularly long lists of downloads[1]
3) Long autocomplete popups[1]

[1]: I'm requesting these because both of them use richlistbox's, which inherit from scrollbox, and I think will also be affected by this change.
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][photon-performance][qa-commented]
(Assignee)

Updated

2 years ago
Depends on: 1387084

Updated

2 years ago
Blocks: 1351466

Updated

2 years ago
Depends on: 1387130

Comment 68

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c2d4c05b890
https://hg.mozilla.org/mozilla-central/rev/39f353cad1a4
https://hg.mozilla.org/mozilla-central/rev/3845af01ea9f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

2 years ago
Depends on: 1387718
Depends on: 1388070
No longer depends on: 1388070

Updated

2 years ago
Depends on: 1387721
No longer depends on: 1387721

Updated

2 years ago
Depends on: 1390188
Verified as fixed on: Windows 10x64(quantum ref.), Windows 7, Windows 8.1, Mac OSX 10.12, Ubuntu 16.04 -
with FF57 x64 9/15
scroll actions: on FF dropdown menus - FF Options, Library, Form History and also covered with the pre-beta smoke tests;
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+

Updated

2 years ago
Depends on: 1403563

Updated

a year ago
Depends on: 1422171
You need to log in before you can comment on or make changes to this bug.