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

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
XUL Widgets
P1
normal
RESOLVED FIXED
4 months ago
2 days ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 fixed)

Details

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

MozReview Requests

()

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

Attachments

(4 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
jaws
: review+
Details | Review
59 bytes, text/x-review-board-request
kip
: review+
Details | Review
59 bytes, text/x-review-board-request
mconley
: review+
Details | Review
19.25 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 months ago
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.

Updated

4 months ago
Component: Untriaged → XUL Widgets
Flags: needinfo?(dao+bmo)
Product: Firefox → Toolkit

Updated

4 months ago
See Also: → bug 1356662

Updated

4 months ago
Flags: qe-verify?
Priority: -- → P2

Comment 1

4 months ago
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?
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Attachment #8858605 - Flags: feedback?(enndeakin)

Updated

4 months ago
Priority: P2 → P1

Updated

4 months ago
Blocks: 1356662
See Also: bug 1356662

Updated

4 months ago
Flags: qe-verify? → qe-verify-

Updated

4 months ago
Blocks: 1357061

Updated

4 months ago
Duplicate of this bug: 785284
(Assignee)

Updated

4 months ago
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]

Updated

4 months ago
Duplicate of this bug: 1358425

Comment 4

4 months 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!)
I don't know much about scrolling to be able to answer this offhand.

Comment 6

4 months ago
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 8

4 months ago
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.

Comment 9

4 months ago
(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)
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
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 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

3 months 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

3 months 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

3 months 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.

Updated

3 months ago
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

3 months ago
Blocks: 1367797
Created attachment 8871856 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling (rebased)
Attachment #8858605 - Attachment is obsolete: true

Updated

3 months ago
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 months ago
It's better, yes!
Flags: needinfo?(mconley)
Created attachment 8880775 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling (rebased)
Attachment #8871856 - Attachment is obsolete: true
Created attachment 8884245 [details] [diff] [review]
Use scroll-behavior: smooth; for XUL scrollbox smooth scrolling (rebased)
Attachment #8880775 - Attachment is obsolete: true
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)

Updated

a month ago
Priority: P1 → P2

Updated

a month ago
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
(Assignee)

Comment 30

a month 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

a month 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.

Updated

25 days ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

22 days ago
Attachment #8862756 - Attachment is obsolete: true
(Assignee)

Updated

22 days ago
Attachment #8884245 - Attachment is obsolete: true
(Assignee)

Updated

22 days ago
Blocks: 1385034
(Assignee)

Comment 38

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

Updated

22 days 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

22 days 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 44

22 days ago
mozreview-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

22 days 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

22 days 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

21 days 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

21 days 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

21 days 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

21 days 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

21 days 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

18 days ago
Thanks. I think I'm going to try to land this stuff after the uplift.

Updated

17 days ago
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.
Created attachment 8892949 [details] [diff] [review]
main patch rebased after bug 1349555
(Assignee)

Comment 61

16 days 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

16 days ago
Attachment #8890991 - Attachment is obsolete: true
(Assignee)

Updated

16 days ago
Depends on: 1349555
(Assignee)

Comment 65

16 days 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

16 days 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

Updated

16 days ago
Flags: qe-verify- → qe-verify+
QA Contact: adrian.florinescu
(Assignee)

Comment 67

16 days 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

15 days ago
Depends on: 1387084

Updated

15 days ago
Blocks: 1351466

Updated

15 days ago
Depends on: 1387130

Comment 68

15 days 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: 15 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

11 days ago
Depends on: 1387718
Depends on: 1388070

Updated

11 days ago
No longer depends on: 1388070

Updated

11 days ago
Depends on: 1387721

Updated

11 days ago
No longer depends on: 1387721

Updated

4 days ago
Depends on: 1390188
Duplicate of this bug: 1358427
No longer depends on: 1390188
You need to log in before you can comment on or make changes to this bug.