Skip to content

Commit

Permalink
Bug 1663537. Fix various tests for desktop zooming scrollbars. r=kats
Browse files Browse the repository at this point in the history 
The test fixes all fell into the follow categories:

A) The test uses requestAnimationFrame to wait one frame and expects scrolling to be complete. With the desktop zooming scrollbars in order for the scrolling to show up on the main thread we need to send the scroll request to the compositor and then hear back from it via an apz repaint request (apz callback helper). Waiting on requestAnimationFrame will complete the first part, but not necessarily the second part. The fix is to wait for a scroll event.

B) Switching tests to wait for scroll events exposes another problem: the test can do things that cause a scroll in order to setup the test (and that may not be obvious that it causes a scroll) before actually proceeding to do the test and do something that causes a scroll and then checks for the scroll change of the second thing. Waiting for a requestAnimationFrame would include both those scrolls without desktop zooming scrollbars, but if we wait for a scroll event we will get the scroll event for the first thing which we are not interested in. So we need to make sure scroll events are cleared out before waiting for any scroll events. We do this by waiting two requestAnimationFrame's and waiting for apz to be flushed. We also use this when a test does something and it wants to test that scrolling is not performed.

The main thing that causes scrolling that may not be obvious: calling node.focus(). With stacks like:

from test_scroll_per_page.html

```
#1: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#2: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#3: mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4742913]
#4: mozilla::PresShell::FlushPendingScrollAnchorAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4650069]
#5: mozilla::PresShell::ProcessReflowCommands(bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x465742b]
#6: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656af8]
#7: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
mozilla#8: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
mozilla#9: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
mozilla#10: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
mozilla#11: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
mozilla#12: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
mozilla#13: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
mozilla#14: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]
```

from editor/libeditor/tests/test_bug549262.html

```
#1: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#2: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#3: mozilla::PresShell::ScrollFrameRectIntoView(nsIFrame*, nsRect const&, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x46541bc]
#4: mozilla::PresShell::DoScrollContentIntoView() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4653776]
#5: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656b11]
#6: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#7: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
mozilla#8: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
mozilla#9: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
mozilla#10: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
mozilla#11: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
mozilla#12: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
mozilla#13: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]
```

C) Several tests use nsIDOMWindowUtils advanceTimeAndRefresh/restoreNormalRefresh and expect scrolling to be done after a call to advanceTimeAndRefresh. This is basically A), advanceTimeAndRefresh does a refresh driver tick but doesn't allow a repaint request to come back to the main thread.

Differential Revision: https://phabricator.services.mozilla.com/D89403
  • Loading branch information
@tnikkel
tnikkel committed Sep 11, 2020
1 parent 927eea2 commit 86ef6ee
Show file tree
Hide file tree
Showing 9 changed files with 211 additions and 83 deletions.
1 change: 1 addition & 0 deletions editor/libeditor/tests/mochitest.ini
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ support-files =
file_select_all_without_body.html
green.png
spellcheck.js
!/gfx/layers/apz/test/mochitest/apz_test_utils.js

[test_bug46555.html]
[test_bug200416.html]
Expand Down
71 changes: 54 additions & 17 deletions editor/libeditor/tests/test_bug549262.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
<script src="/tests/SimpleTest/EventUtils.js"></script>
<script type="text/javascript" src="/tests/gfx/layers/apz/test/mochitest/apz_test_utils.js"></script>
</head>
<body>
<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=549262">Mozilla Bug 549262</a>
Expand All @@ -24,31 +25,56 @@
var win = window.open("file_bug549262.html", "_blank",
"width=600,height=600,scrollbars=yes");

// grab the timer right at the start
var cwu = SpecialPowers.getDOMWindowUtils(win);
function step() {
cwu.advanceTimeAndRefresh(100);
function waitForScrollEvent(aWindow) {
return new Promise(resolve => {
aWindow.addEventListener("scroll", () => { SimpleTest.executeSoon(resolve); }, {once: true, capture: true});
});
}

function waitAndCheckNoScrollEvent(aWindow) {
let gotScroll = false;
function recordScroll() {
gotScroll = true;
}
aWindow.addEventListener("scroll", recordScroll, {capture: true});
return new Promise(resolve => {aWindow.requestAnimationFrame(() => { aWindow.requestAnimationFrame(
() => { flushApzRepaints(
function() {
aWindow.removeEventListener("scroll", recordScroll, {capture: true});
is(gotScroll, false, "check that we didn't get a scroll");
resolve();
}, aWindow
); }
); }); });
}

function waitToClearOutAnyPotentialScrolls(aWindow) {
return new Promise(resolve => {aWindow.requestAnimationFrame(() => { aWindow.requestAnimationFrame(() => { flushApzRepaints(resolve, aWindow); }); }); });
}

SimpleTest.waitForFocus(function() {
SpecialPowers.pushPrefEnv({"set": [[smoothScrollPref, false]]}, startTest);
}, win);
function startTest() {
async function startTest() {
// Make sure that pressing Space when a contenteditable element is not focused
// will scroll the page.
var ed = win.document.getElementById("editor");
var sc = win.document.querySelector("a");
sc.focus();
await waitToClearOutAnyPotentialScrolls(win);
is(win.scrollY, 0, "Sanity check");
let waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForScrolling;

isnot(win.scrollY, 0, "Page is scrolled down");
is(ed.textContent, "abc", "The content of the editable element has not changed");
var oldY = win.scrollY;
waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {shiftKey: true}, win);

step();
await waitForScrolling;

ok(win.scrollY < oldY, "Page is scrolled up");
is(ed.textContent, "abc", "The content of the editable element has not changed");
Expand All @@ -57,71 +83,82 @@
// will not scroll the page, and will edit the element.
ed.focus();
win.getSelection().collapse(ed.firstChild, 1);
await waitToClearOutAnyPotentialScrolls(win);
oldY = win.scrollY;
let waitForNoScroll = waitAndCheckNoScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForNoScroll;

ok(win.scrollY <= oldY, "Page is not scrolled down");
is(ed.textContent, "a bc", "The content of the editable element has changed");
sc.focus();
await waitToClearOutAnyPotentialScrolls(win);
waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForScrolling;

isnot(win.scrollY, 0, "Page is scrolled down");
is(ed.textContent, "a bc", "The content of the editable element has not changed");
ed.focus();
win.getSelection().collapse(ed.firstChild, 3);
await waitToClearOutAnyPotentialScrolls(win);
waitForNoScroll = waitAndCheckNoScrollEvent(win);
synthesizeKey(" ", {shiftKey: true}, win);

step();
await waitForNoScroll;

isnot(win.scrollY, 0, "Page is not scrolled up");
is(ed.textContent, "a b c", "The content of the editable element has changed");

// Now let's test the down/up keys
sc = document.body;

step();

ed.blur();
sc.focus();
await waitToClearOutAnyPotentialScrolls(win);
oldY = win.scrollY;
waitForScrolling = waitForScrollEvent(win);
synthesizeKey("VK_UP", {}, win);

step();
await waitForScrolling;

ok(win.scrollY < oldY, "Page is scrolled up");
oldY = win.scrollY;
ed.focus();
win.getSelection().collapse(ed.firstChild, 3);
await waitToClearOutAnyPotentialScrolls(win);
waitForNoScroll = waitAndCheckNoScrollEvent(win);
synthesizeKey("VK_UP", {}, win);

step();
await waitForNoScroll;

is(win.scrollY, oldY, "Page is not scrolled up");
is(win.getSelection().focusNode, ed.firstChild, "Correct element selected");
is(win.getSelection().focusOffset, 0, "Selection should be moved to the beginning");
win.getSelection().removeAllRanges();
await waitToClearOutAnyPotentialScrolls(win);
waitForScrolling = waitForScrollEvent(win);
synthesizeMouse(sc, 300, 300, {}, win);
synthesizeKey("VK_DOWN", {}, win);

step();
await waitForScrolling;

ok(win.scrollY > oldY, "Page is scrolled down");
ed.focus();
win.getSelection().collapse(ed.firstChild, 3);
await waitToClearOutAnyPotentialScrolls(win);
oldY = win.scrollY;
waitForNoScroll = waitAndCheckNoScrollEvent(win);
synthesizeKey("VK_DOWN", {}, win);

step();
await waitForNoScroll;

is(win.scrollY, oldY, "Page is not scrolled down");
is(win.getSelection().focusNode, ed.firstChild, "Correct element selected");
is(win.getSelection().focusOffset, ed.textContent.length, "Selection should be moved to the end");

cwu.restoreNormalRefresh();
win.close();

SimpleTest.finish();
Expand Down
59 changes: 44 additions & 15 deletions editor/libeditor/tests/test_bug915962.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
<script src="/tests/SimpleTest/EventUtils.js"></script>
<script type="text/javascript" src="/tests/gfx/layers/apz/test/mochitest/apz_test_utils.js"></script>
</head>
<body>
<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=915962">Mozilla Bug 915962</a>
Expand All @@ -24,71 +25,99 @@
var win = window.open("file_bug915962.html", "_blank",
"width=600,height=600,scrollbars=yes");

// grab the timer right at the start
var cwu = SpecialPowers.getDOMWindowUtils(win);
function step() {
cwu.advanceTimeAndRefresh(100);

function waitForScrollEvent(aWindow) {
return new Promise(resolve => {
aWindow.addEventListener("scroll", () => { SimpleTest.executeSoon(resolve); }, {once: true, capture: true});
});
}

function waitAndCheckNoScrollEvent(aWindow) {
let gotScroll = false;
function recordScroll() {
gotScroll = true;
}
aWindow.addEventListener("scroll", recordScroll, {capture: true});
return new Promise(resolve => {aWindow.requestAnimationFrame(() => { aWindow.requestAnimationFrame(
() => { flushApzRepaints(
function() {
aWindow.removeEventListener("scroll", recordScroll, {capture: true});
is(gotScroll, false, "check that we didn't get a scroll");
resolve();
}, aWindow
); }
); }); });
}

function waitToClearOutAnyPotentialScrolls(aWindow) {
return new Promise(resolve => {aWindow.requestAnimationFrame(() => { aWindow.requestAnimationFrame(() => { flushApzRepaints(resolve, aWindow); }); }); });
}

SimpleTest.waitForFocus(function() {
SpecialPowers.pushPrefEnv({"set": [[smoothScrollPref, false]]}, startTest);
}, win);
function startTest() {
async function startTest() {
// Make sure that pressing Space when a tabindex=-1 element is focused
// will scroll the page.
var button = win.document.querySelector("button");
var sc = win.document.querySelector("div");
sc.focus();
await waitToClearOutAnyPotentialScrolls(win);
is(win.scrollY, 0, "Sanity check");
let waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForScrolling;

isnot(win.scrollY, 0, "Page is scrolled down");
var oldY = win.scrollY;
waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {shiftKey: true}, win);

step();
await waitForScrolling;

ok(win.scrollY < oldY, "Page is scrolled up");

// Make sure that pressing Space when a tabindex=-1 element is focused
// will not scroll the page, and will activate the element.
button.focus();
await waitToClearOutAnyPotentialScrolls(win);
var clicked = false;
button.onclick = () => clicked = true;
oldY = win.scrollY;
let waitForNoScroll = waitAndCheckNoScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForNoScroll;

ok(win.scrollY <= oldY, "Page is not scrolled down");
ok(clicked, "The button should be clicked");
synthesizeKey("VK_TAB", {}, win);

step();
await waitToClearOutAnyPotentialScrolls(win);

oldY = win.scrollY;
waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForScrolling;

ok(win.scrollY >= oldY, "Page is scrolled down");

cwu.restoreNormalRefresh();
win.close();

win = window.open("file_bug915962.html", "_blank",
"width=600,height=600,scrollbars=yes");
cwu = SpecialPowers.getDOMWindowUtils(win);
SimpleTest.waitForFocus(function() {

SimpleTest.waitForFocus(async function() {
is(win.scrollY, 0, "Sanity check");
waitForScrolling = waitForScrollEvent(win);
synthesizeKey(" ", {}, win);

step();
await waitForScrolling;

isnot(win.scrollY, 0, "Page is scrolled down without crashing");

cwu.restoreNormalRefresh();
win.close();

SimpleTest.finish();
Expand Down
Loading

0 comments on commit 86ef6ee

Please sign in to comment.