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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Justin D'Arcangelo [:justindarc]]

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

Comment 1

[:kip (Kearwood Gilbert)]

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.

Comment 2

[:kip (Kearwood Gilbert)]

This can be reproduced on OSX with APZ enabled.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Yeah not having an APZC for overflow: hidden elements is intentional. Can we fall back to main-thread smooth scroll for this scenario?

Comment 4

[:kip (Kearwood Gilbert)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 6

[Botond Ballo [:botond]]

(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.

Comment 7

[Justin D'Arcangelo [:justindarc]]

(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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[:kip (Kearwood Gilbert)]

(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.

Comment 10

[Justin D'Arcangelo [:justindarc]]

(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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 12

[Justin D'Arcangelo [:justindarc]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test case

This is the JSBin thing packaged up into a standalone test file.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

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.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

[Tracking Requested - why for this release]: regression from 45 and older versions.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression with APZ

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1104356 - When doing a smooth scroll on a non-APZ'd scrollframe, fall back to the main thread machinery. r?kip

Review commit: https://reviewboard.mozilla.org/r/33773/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33773/

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1104356 - Add a reftest. r?kip

Review commit: https://reviewboard.mozilla.org/r/33775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33775/

Comment 22

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=579fc7337eed

Comment 23

[:kip (Kearwood Gilbert)]

https://reviewboard.mozilla.org/r/33771/#review30487

This looks good, thanks!

Comment 24

[:kip (Kearwood Gilbert)]

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!

Comment 25

[:kip (Kearwood Gilbert)]

Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

https://reviewboard.mozilla.org/r/33775/#review30493

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17a54cf0454b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a556e78d4a62

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/17a54cf0454b
https://hg.mozilla.org/mozilla-central/rev/a556e78d4a62

Comment 28

[Kartikaya Gupta (email:[e-mail])]

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

Comment 29

[Kartikaya Gupta (email:[e-mail])]

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)

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

New reftest for APZ, please uplift to aurora.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb1aa0960ac2
https://hg.mozilla.org/releases/mozilla-aurora/rev/215b8f3a6f8a

Comment 33

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 34

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

As Comment 33, this should be uplifted as well.

Comment 35

[Josh Cheng [:josh]]

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

Comment 36

[Josh Cheng [:josh]]

Comment on attachment 8716283 [details]
MozReview Request: Bug 1104356 - Add a reftest. r?kip

Approve for TV 2.5

Comment 37

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/311b64761900
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fee39861e880

Comment 38

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/311b64761900
> remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fee39861e880

Thanks for the help.