1486695 - Scrolling not working in an iframe on Android.

Status: VERIFIED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Karl Dubost💡 :karlcow]

This is a spin-off of https://webcompat.com/issues/18316



This is a test case created by Daniel Holbert
https://fiddle.jshell.net/g0Ljc9f4/show/

And the following comment
https://webcompat.com/issues/18316#issuecomment-414862903
==========================
Here's a reduced testcase, with a skinnier iframe to make the "trapping" more obvious: https://jsfiddle.net/g0Ljc9f4/

Here's a version you can load on your phone (without most of the jsfiddle ui):
https://fiddle.jshell.net/g0Ljc9f4/show/

STR, from that testcase:
STEP 1. Try tapping to the right of the purple box, and scrolling up and down.
STEP 2. Try tapping inside and scrolling up and down.
STEP 3. Try tapping to the right again and scrolling up and down. Does it still work? Can you scroll to see the bottom purple border?

In Firefox on Android, I'm unable to scroll the outer scrolling context in step 3 -- after I've tapped inside the iframe, that's the only piece that Firefox will let me scroll.

==========================

Comment 1

[Botond Ballo [:botond]]

Thanks for the reduced testcase!

This is a very simple page for us to be getting wrong... will investigate.

Comment 2

[Botond Ballo [:botond]]

Looking like an issue in the scroll frame activation code during display list building / FrameLayerBuilder.

Comment 3

[Botond Ballo [:botond]]

And the difference in behaviour between Fennec and RDM is probably due to containerful vs. containerless scrolling.

Comment 4

[Botond Ballo [:botond]]

Indeed, I can reproduce the problem on desktop (no RDM required even) if I flip layout.scroll.root-frame-containers.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1486695 - Only set nsDisplayListBuilder::mActiveScrolledRootForRootScrollframe for the RCD-RSF. r?mstange

We were mistakenly using it for subframe RSF's as well.

Comment 8

[Botond Ballo [:botond]]

This patch is causing layout/reftests/async-scrolling/fixed-pos-scrolled-clip-4.html to fail on Android.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 9011121 [details]
Bug 1486695 - Only set nsDisplayListBuilder::mActiveScrolledRootForRootScrollframe for the RCD-RSF. r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa1831cceeae
Only set nsDisplayListBuilder::mActiveScrolledRootForRootScrollframe for the RCD-RSF. r=mstange

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fa1831cceeae

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 9011121 [details]
Bug 1486695 - Only set nsDisplayListBuilder::mActiveScrolledRootForRootScrollframe for the RCD-RSF. r?mstange

Approval Request Comment
[Feature/Bug causing the regression]:
  Likely bug 1298218

[User impact if declined]:
  On pages with a certain structure (iframe inside 
  scrollable <div>), scrolling the iframe results in
  no longer being able to scroll the <div>.

[Is this code covered by automated tests?]:
  This particular scenario isn't, but test coverage of
  the code being touched is fairly good (e.g. several
  previous attempts at fixing this bug that were misguided
  were caught by test failures).

[Has the fix been verified in Nightly?]:
  Yes, by me.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None.

[Is the change risky?]:
  Low risk.

[Why is the change risky/not risky?]:
  Small, targeted, and (by now) well-understood change

[String changes made/needed]:
  None

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9011121 [details]
Bug 1486695 - Only set nsDisplayListBuilder::mActiveScrolledRootForRootScrollframe for the RCD-RSF. r?mstange

Targeted fix, uplift accepted for 63 beta 11, thanks.

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/53e361cf9567

Comment 16

[Bogdan Surd, QA [:bsurd, NI]]

I have also verified this fix.