1674935 - Cannot scroll with keyboard on result page of google search when non-WebRender

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: scroll with keyboard is broken if non-WebRender

Reproducible: always

Steps to reproduce:

1. Set gfx.webrender.force-disabled to true in about config
   OR
   Set layers.acceleration.disabled to true
2. Restart browser
3. Type "? mozilla" (without quotation marks) in address bar and hit enter
4. Try scroll with keyboard

Actual Results:
Page would not scroll.

Expected Results:
Page should scroll.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cab81767be70469b780cd372ff463df59c2264e0&tochange=32a2e59ab7260118d8d5f430e503967fad6993cf

Before land Bug 1657822 : Bug 1674802
After land Bug 1657822 : this bug appears

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Timothy, your patch in bug 1657822 seems to have caused this regression. Could you look and tell us if this needs fixing before we ship 83? Thanks

Comment 2

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

I investigated this a bit just now. It looks like the page structure is making it so that this NotifyApzTransaction() call is getting run and clearing out the pending ScrollPositionUpdate without it actually getting sent over in the paint transaction.

Comment 3

[Timothy Nikkel (:tnikkel)]

I have a potential fix for this but try server seemed upset in general, not at my patch, this evening

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=da153e066348ed3e0da6de0b692d9b38d2c3ff72

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1674935. Collect and notify scroll frame in nsDisplayListBuilder not in ContainerState. r?mattwoodrow

Scroll frames can appear in multiple containers.

Notifying the scroll frame causes it to clear it list of scroll updates. So we lose the scroll updates prematurely.

Previously https://phabricator.services.mozilla.com/D88650 moved the notify from happening the first time the scroll frame was encountered in a container to when the container is finished.

This patch moves this handling out of the container state and into FrameLayerBuilder.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/babbf6dcd496
Collect and notify scroll frame in nsDisplayListBuilder not in ContainerState. r=mattwoodrow

Comment 6

[Timothy Nikkel (:tnikkel)]

Filed bug 1675709 to ensure the webrender code path can't suffer the same problem.

Comment 7

[Razvan Maries]

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

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9185820 [details]
Bug 1674935. Collect and notify scroll frame in nsDisplayListBuilder not in ContainerState. r?mattwoodrow

Beta/Release Uplift Approval Request

• User impact if declined: (main thread initiated) scrolling doesn't work on some pages
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I specifically crafted the patch to be low risk, moving further fixes into bug 1675709
• String changes made/needed:

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9185820 [details]
Bug 1674935. Collect and notify scroll frame in nsDisplayListBuilder not in ContainerState. r?mattwoodrow

Approved for 83.0b10.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0202468c6752

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1674935. Add mochitest.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9649e35c269e
Add mochitest.

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9649e35c269e

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1674935. Improve test.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef6aa636ed54
Improve test. r=kats

Comment 17

[Alexandru Michis [:malexandru]]

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