Closed Bug 1104356 Opened 5 years ago Closed 4 years ago

scrollTo() smooth-scrolling behavior does not work on overflow:hidden elements

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
blocking-b2g 2.5+
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed
b2g-v2.5 --- fixed

People

(Reporter: justindarc, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [ft:conndevices])

Attachments

(3 files, 1 obsolete file)

The following demo works in Nightly Desktop, but does not work in B2G nightly builds. When passing the `behavior: 'smooth'` option, the result appears to be a NOOP.

http://jsbin.com/duqiqu
Modifying the demo to use "overflow: scroll" rather than "overflow: hidden" enables the smooth scrolling.

Perhaps for reasons described in Bug 942995 (APZC for overflow:hidden element should not allow panning), we are not creating the APZC for the overflow: hidden layers or otherwise disabling scrolling through touch gestures in a manner that affects the smooth scrolls.
Assignee: nobody → kgilbert
This can be reproduced on OSX with APZ enabled.
Yeah not having an APZC for overflow: hidden elements is intentional. Can we fall back to main-thread smooth scroll for this scenario?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Yeah not having an APZC for overflow: hidden elements is intentional. Can we
> fall back to main-thread smooth scroll for this scenario?
Yes, I have a main thread implementation that we could fall back to; however, the animation on the main thread would potentially not be as smooth, especially if there is long running Javascript.
Is the "we don't want APZC for overflow:hidden" a "we didn't get to changing the behaviour", or "this is the right thing to do"?  I couldn't quite tell from the comments above.
(In reply to Milan Sreckovic [:milan] from comment #5)
> Is the "we don't want APZC for overflow:hidden" a "we didn't get to changing
> the behaviour", or "this is the right thing to do"?  I couldn't quite tell
> from the comments above.

I suspect it's "we've never had a reason to have APZ for non-root overflow:hidden elements".

We do have APZ for a root overflow:hidden element, so that we can zoom it and scroll the zoomed content within the original bounds. To accomodate this, APZ has support for respecting overflow:hidden (i.e. not allowing you to scroll past the original bounds). In theory, we can enable APZ for non-root overflow:hidden elements the same way. However, there might be performance implications due to the extra layerization.
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Milan Sreckovic [:milan] from comment #5)
> > Is the "we don't want APZC for overflow:hidden" a "we didn't get to changing
> > the behaviour", or "this is the right thing to do"?  I couldn't quite tell
> > from the comments above.
> 
> I suspect it's "we've never had a reason to have APZ for non-root
> overflow:hidden elements".
> 
> We do have APZ for a root overflow:hidden element, so that we can zoom it
> and scroll the zoomed content within the original bounds. To accomodate
> this, APZ has support for respecting overflow:hidden (i.e. not allowing you
> to scroll past the original bounds). In theory, we can enable APZ for
> non-root overflow:hidden elements the same way. However, there might be
> performance implications due to the extra layerization.

This is along the same lines as my thoughts on this as well. Perhaps a sufficient compromise would be to promote an `overflow: hidden` element to an APZ layer if/when `scrollTo()/scrollBy()` is called on it? I understand that this may result in an initial latency while the layer gets created, but this seems like a reasonable expectation. Simply promoting every `overflow: hidden` element to its own APZ layer sounds like a bad idea in general to me. There are hundreds of cases where `overflow: hidden` is used to obscure parts of content within the bounds of an element where the element will never be scrolled.
I agree with all the stuff in comments 4-7. The one thing I would note is that simply forcing a layer for a scrollTo'd frame will not be enough. The way overflow:hidden works on the APZ side is that the APZ actually doesn't know that the layer is scrollable and so if we just ran the smooth-scroll animation on it as-is it would likely think it ran into the end of the document and hand off that scroll upwards through the handoff chain. We would need some extra plumbing to allow the APZ side to know how much the document is actually scrollable.

Also note that scrollBy() currently is always done on the main thread; we have bug 959793 open for making that work with APZ.
Summary: scrollTo() smooth behavior broke in B2G → scrollTo() smooth-scrolling behavior does not work on overflow:hidden elements
(In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > (In reply to Milan Sreckovic [:milan] from comment #5)
> > > Is the "we don't want APZC for overflow:hidden" a "we didn't get to changing
> > > the behaviour", or "this is the right thing to do"?  I couldn't quite tell
> > > from the comments above.
> > 
> > I suspect it's "we've never had a reason to have APZ for non-root
> > overflow:hidden elements".
> > 
> > We do have APZ for a root overflow:hidden element, so that we can zoom it
> > and scroll the zoomed content within the original bounds. To accomodate
> > this, APZ has support for respecting overflow:hidden (i.e. not allowing you
> > to scroll past the original bounds). In theory, we can enable APZ for
> > non-root overflow:hidden elements the same way. However, there might be
> > performance implications due to the extra layerization.
> 
> This is along the same lines as my thoughts on this as well. Perhaps a
> sufficient compromise would be to promote an `overflow: hidden` element to
> an APZ layer if/when `scrollTo()/scrollBy()` is called on it? I understand
> that this may result in an initial latency while the layer gets created, but
> this seems like a reasonable expectation. Simply promoting every `overflow:
> hidden` element to its own APZ layer sounds like a bad idea in general to
> me. There are hundreds of cases where `overflow: hidden` is used to obscure
> parts of content within the bounds of an element where the element will
> never be scrolled.
Perhaps the initial latency could be eliminated by automatically promoting an 'overflow: hidden' element to an APZ layer if the 'scroll-behavior: smooth' property is set on it.  This wouldn't work for all cases, as the CSSOM-View DOM methods can actually scroll smoothly without the default set in CSS; however, it could be a useful optimization for the common case.
(In reply to :kip (Kearwood Gilbert) from comment #9)
> Perhaps the initial latency could be eliminated by automatically promoting
> an 'overflow: hidden' element to an APZ layer if the 'scroll-behavior:
> smooth' property is set on it.  This wouldn't work for all cases, as the
> CSSOM-View DOM methods can actually scroll smoothly without the default set
> in CSS; however, it could be a useful optimization for the common case.

+1 -- Sounds like a suitable optimization to me.
I'd rather use something like |will-change: scroll-position| to force layerization, rather than overloading the scroll-behaviour property to have this "secret" side-effect. I would say this sort of latency-hiding optimization is exactly what will-change was intended for.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> I'd rather use something like |will-change: scroll-position| to force
> layerization, rather than overloading the scroll-behaviour property to have
> this "secret" side-effect. I would say this sort of latency-hiding
> optimization is exactly what will-change was intended for.

I was unaware that `scroll-position` was an acceptable value for `will-change`. If so, that would be even better.
I'm assuming it is based on https://developer.mozilla.org/en-US/docs/Web/CSS/will-change. If it doesn't exist yet we can probably add it.
This is a regression with APZ enabled in 46; the demo doesn't scroll at all if the smooth checkbox is enabled. We need to fix this before ship.
Assignee: kgilbert → bugmail.mozilla
Keywords: regression
Attached file Test case
This is the JSBin thing packaged up into a standalone test file.
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Attached patch WIP (obsolete) — Splinter Review
Here's a patch that fixes it by falling back to the main-thread machinery. It's small and should be safe to uplift to 46.

However a few lines below where I change this, I see that we set a displayport on the scrollframe if there isn't one already, so in theory that should cause the scrollframe to get layerized for APZ. For some reason it's not working the way it should so I want to investigate a bit more and figure out what's going on there.
[Tracking Requested - why for this release]: regression from 45 and older versions.
Tracking since this is a recent regression with APZ
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> However a few lines below where I change this, I see that we set a
> displayport on the scrollframe if there isn't one already, so in theory that
> should cause the scrollframe to get layerized for APZ. For some reason it's
> not working the way it should so I want to investigate a bit more and figure
> out what's going on there.

Ah, this is because the scrollable rect size remains the same as the composition bounds, so even though it *does* get layerized, APZ can't scroll it past 0. I suppose we could change this but it seems more risky than just doing the main-thread scroll in this case.
Comment on attachment 8716282 [details]
MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

https://reviewboard.mozilla.org/r/33773/#review30489

Looks good, thanks!
Attachment #8716282 - Flags: review?(kgilbert) → review+
Attachment #8716283 - Flags: review?(kgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/17a54cf0454b
https://hg.mozilla.org/mozilla-central/rev/a556e78d4a62
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716282 [details]
MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: doing a programmatic smooth scroll on an overflow:hidden frame will fail entirely
[Describe test coverage new/current, TreeHerder]: added a reftest
[Risks and why]: small patch, relatively low risk.
[String/UUID change made/needed]: none
Attachment #8716282 - Flags: approval-mozilla-aurora?
Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

Approval Request Comment
(This is a test that goes with the other patch)
Attachment #8716283 - Flags: approval-mozilla-aurora?
Comment on attachment 8716282 [details]
MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

Fix for scrolling in frames with APZ, includes tests, please uplift to aurora
Attachment #8716282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

New reftest for APZ, please uplift to aurora.
Attachment #8716283 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8716282 [details]
MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): APZ, and according to Bug 1262369 Comment 20
User impact if declined: doing a programmatic smooth scroll on an overflow:hidden frame will fail.
Testing completed: Tested on b2g44-v2.5 on nexus-player
Risk to taking this patch (and alternatives if risky): Small patch, relative low risk
String or UUID changes made by this patch: none
Attachment #8716282 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

As Comment 33, this should be uplifted as well.
Attachment #8716283 - Flags: approval‑mozilla‑b2g44?
blocking-b2g: --- → 2.5+
Whiteboard: [ft:conndevices]
Comment on attachment 8716282 [details]
MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

Approve for TV 2.5
Attachment #8716282 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

Approve for TV 2.5
Attachment #8716283 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
See Also: → 1262369
Blocks: 1262369
No longer blocks: 1262369
You need to log in before you can comment on or make changes to this bug.