1567237 - overflow:scroll on elements not needing to scroll prevents keyboard panning on main document, unlike Chrome

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski [:twisniewski]]

Attachment: testcase.html

As shown in the attached test-case, if an element that needs no scrollbar is given overflow:scroll, Firefox and Chrome differ in how they handle key events while that element has focus.

Firefox will simply eat up/down keypresses, as the element's scrollbar is useless and has no content to scroll to. Chrome does not just eat the key presses, and allow the background document to scroll.

This is causing webcompat issues, as reported at https://github.com/helm/helm-www/issues/139, as seen on https://helm.sh/docs/?origin_team=T027LFU12 (where focusing the content with the boat animation breaks the ability to scroll the document, unlike in Chrome).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I just took a look at this and I think this should be pretty easy to fix... I'll try to take a look sometime this week.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1567237 - Only use scroll range to select scrollable frames to scroll to, don't use scrollbar visibility. r=tnikkel

This is what other browsers do, and it does make sense to me, it's useless to
try to scroll a frame with no scroll range in a given direction.

I think all callers of this function should be treated like this, so this is
more like a RFC / feedback request than a patch per se.

The wheel handling code already checks scroll range, so there's no difference of
behavior in that case, if I'm reading the code right.

There are a few other functions that check the result of
GetPerceivedScrollingDirections(), but I think if we change this we should
change this consistently.

I also think that if we do this we should rename the method to something like
GetAvailableScrollingDirections() or such.

Anyhow, wdyt? I should also add a test for this if we go with this.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I sent a patch, with a few comments / questions. If the reviewer doesn't like that patch I can do something more special-casey, or something more sophisticated like "if there's no scroll range in the given direction, like downwards, then go to the closest scroll frame.

That'd change behavior for nested scrollers, when after we'd reached the top of the range, then we'd scroll the parent one. Not 100% sure that's desirable.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Also, fwiw, we don't seem to have test coverage for the existing behavior: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf71d6a7c36b4f3425e8ca8b0a22ed6dfd47600e

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d602a56edc7
Only use scroll range to select scrollable frames to scroll to, don't use scrollbar visibility. r=tnikkel

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90048e3d6eb3
followup: Fix ESLint complaints in test.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72699e27e033
followup: Fix test on windows. r=whoops

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for for ESlint and mochitest failures on test_scroll_space_no_range_overflow_scroll.html .

Backout link: https://hg.mozilla.org/integration/autoland/rev/30e8de1723edc7ea368513923902c8f22b443ebb
test_scroll_space_no_range_overflow_scroll.html log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258016027&repo=autoland&lineNumber=11302

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fe42a3dda2c
Only use scroll range to select scrollable frames to scroll to, don't use scrollbar visibility. r=tnikkel

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 2fe42a3dda2c (bug 1567237) for causing leaks

Backout: https://hg.mozilla.org/integration/autoland/rev/f65b096aa0f7be02d8ce95a3b739b10575cd5239

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2fe42a3dda2c3a72f5ed829ba145cfce4a242752

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258128219&repo=autoland&lineNumber=46553

Comment 11

[Emilio Cobos Álvarez (:emilio)]

So I'm pretty sure than the test rather than the patch causing the leaks... And they look persistent... huh.

It looks like we had bug 1567448 for the same leak, my patch probably just toggled the tests so that it became perma...

Botond, Andrew, any idea off-hand of where this / bug 1567448 may be coming from? We're leaking a whole ton of stuff here... I'll try to repro locally, and remember how to figure out the root of the leaked graph to fix it...

Comment 12

[Andrew McCreight [:mccr8]]

Bug 1567448 is just what any leak that leaks a window is going to look like. There's a bunch of variants of it: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=intermittent%20leakcheck%20APZEventState&list_id=14820911 In an ideal world, these leaks would show up differently in treeherder.

The general outline of how to figure out where the leak is starting is here: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD/Heap_Scan_Mode

You can ignore anything about DMD there to start with.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Hmm, maybe the test is to blame after all (i.e., it uncovers another bug), this seems to make the leak go away:

diff --git a/layout/base/tests/test_scroll_space_no_range_overflow_scroll.html b/layout/base/tests/test_scroll_space_no_range_overflow_scroll.html
index 3a56a0d68d9a..54698d2fa575 100644
--- a/layout/base/tests/test_scroll_space_no_range_overflow_scroll.html
+++ b/layout/base/tests/test_scroll_space_no_range_overflow_scroll.html
@@ -19,7 +19,7 @@ function waitForScrollEvent(target) {
   });
 }
 
-const selectionController =
+let selectionController =
   SpecialPowers.wrap(window)
              .docShell
              .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
@@ -56,5 +56,8 @@ promise_test(async function() {
   await doPageDown(window);
   assert_equals(unscrollable.scrollTop, 0, "Should not be able to scroll the unscrollable div");
   assert_not_equals(rootScroller.scrollTop, rootScrollTop, "Root should be able to scroll");
+
+  // Otherwise we somehow leak!?!?
+  selectionController = null;
 }, "Overflow scroll without range doesn't block scrolling of the main document");
 </script>

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Ok, I filed bug 1568686 on the issue that causes the leak.

I prevented the test from leaking while we figure that out... So I'll try again, third time's the charm?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3b65ecc2afb
Only use scroll range to select scrollable frames to scroll to, don't use scrollbar visibility. r=tnikkel

Comment 16

[Daniel Varga [:dvarga]]

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