Bug 1663537. Fix various tests for desktop zooming scrollbars. r?kats
ClosedPublic
Actions

Authored by tnikkel on Sep 7 2020, 11:31 PM.

Details

Reviewers

kats

Commits

rMOZILLACENTRAL30cd33572b3d: Bug 1663537. Fix various tests for desktop zooming scrollbars. r=kats

Bugzilla Bug ID

1663537

Summary

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

#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: 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]
#03: mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4742913]
#04: mozilla::PresShell::FlushPendingScrollAnchorAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4650069]
#05: mozilla::PresShell::ProcessReflowCommands(bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x465742b]
#06: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656af8]
#07: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#08: 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]
#09: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#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]
#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]
#12: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#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]
#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

#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: 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]
#03: 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]
#04: mozilla::PresShell::DoScrollContentIntoView() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4653776]
#05: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656b11]
#06: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#07: 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]
#08: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#09: 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]
#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]
#11: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#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]
#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.

Diff Detail

Repository: rMOZILLACENTRAL mozilla-central

Event Timeline

tnikkel created this revision.Sep 7 2020, 11:31 PM

Herald added a project: secure-revision. · View Herald TranscriptSep 7 2020, 11:31 PM

Harbormaster completed remote builds in B237670: Diff 336249.Sep 7 2020, 11:31 PM

phab-bot published this revision for review.Sep 7 2020, 11:31 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added a subscriber: emilio. · View Herald TranscriptSep 7 2020, 11:31 PM

This mostly looks ok, but I'm a bit concerned about the waitAndCheckNoScrollEvent function (see comment below). With that addressed I'm happy to r+

editor/libeditor/tests/test_bug549262.html
34	Looks like you're using this function to ensure there's no scrolls in cases where the scroll would have gone through APZ. I'm not sure we can guarantee that two rAF calls are enough for that, it might be better to also do an APZ flush and wait for that to complete. That way you're guaranteed there's no more pending repaint requests that will trigger a scroll. I'd change this to do: return new Promise(resolve => {aWindow.requestAnimationFrame(() => { aWindow.requestAnimationFrame( () => { flushApzRepaints( function() { ... }, aWindow ); }); }); }); See e.g. this to see how to pull in apz_test_utils.js which gets you the `flushApzRepaints` function.
editor/libeditor/tests/test_selection_move_commands.html
106	I'm guessing these yields are so that the first scroll command doesn't get clobbered by the second command. It looks like without the yield they would get processed in the same refresh driver tick and probably might not do the right thing in terms of getting serialized onto the framemetrics. If that's the problem, then I would expect my ScrollPositionUpdate refactoring to fix this, and you may not need to insert these yields.

This revision now requires changes to proceed.Sep 8 2020, 9:18 AM

tnikkel added inline comments.Sep 8 2020, 9:51 PM

editor/libeditor/tests/test_selection_move_commands.html
106	I didn't observed a problem like you describe (two scrolls on one framemetrics not getting handled correctly), but that's not to say that it wasn't possible/happening. I put a yield after every line in this test that I expected to cause a scroll event, so it would hit the scroll event listener in the main test loop below, so the scroll event would get consumed, and so we wouldn't have any "extra" scroll events pending that would cause us to read the scroll position before the scrolling we were actually interested in finished. Similar to other tests (except this test used a unique test structure involving yields). I can try removing the yield (working from my code that is based upon your ScrollPositionUpdate refactoring) but then we have to rely on the two scrolls getting coalesced into one resulting scroll event. That may be true now (I'll check) but I'm not sure that is guaranteed moving forward. \|SpecialPowers.doCommand(window, "cmd_scrollPageUp")\| could theoretically cause flushing or whatever else to happen.

kats accepted this revision.Sep 9 2020, 5:57 PM

kats added inline comments.

editor/libeditor/tests/test_selection_move_commands.html
106	Yeah that's a fair point. Adding the yield statements should be fine for this test because it's not central to what the test is testing. If needed we can add other tests that explicitly check for scroll events getting "coalesced" and such.

This revision is now accepted and ready to land.Sep 9 2020, 5:57 PM

tnikkel marked 3 inline comments as done.Sep 10 2020, 3:19 AM

tnikkel added inline comments.