Closed Bug 1383365 Opened 7 years ago Closed 7 years ago

Add a test for keyboard APZ

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

It'd be nice to have a test to assert that async keyboard scrolling can happen and isn't permablocked by focus or something else.

I have one mostly working that I'll try and clean up.
I should add that I've tested this with and without keyboard apz enabled, and it fails without it.
Comment on attachment 8889224 [details]
Bug 1383365 - Add a test to assert async key scrolling happens.

https://reviewboard.mozilla.org/r/160278/#review165708

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:40
(Diff revision 1)
> +      // Wait until we reach the bottom before proceeding.
> +      window.addEventListener('scroll', waitForScrollBottom);

It's generally good practice to register the listener before triggering the action - so in this case before you do the synthesizeKey.

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:45
(Diff revision 1)
> +      // Wait until we reach the bottom before proceeding.
> +      window.addEventListener('scroll', waitForScrollBottom);
> +    };
> +
> +    function waitForScrollBottom() {
> +      if (window.innerHeight + window.scrollY < document.body.scrollHeight) {

You can simplify the comparison to "if (window.scrollY < window.scrollMaxY)". Alternatively you could do "if (document.body.scrollTop < document.body.scrollTopMax)".

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:91
(Diff revision 1)
> +      var rcd = findRcdNode(apzcTree);
> +
> +      return !!rcd && rcd.hasAsyncKeyScrolled === "true";

Probably a good idea to log a warning or info if rcd is null here.

::: gfx/layers/apz/test/mochitest/test_key_scroll.html:8
(Diff revision 1)
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1383365
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test for TODO</title>

nit: fill in TODO

::: gfx/layers/apz/test/mochitest/test_key_scroll.html:14
(Diff revision 1)
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="apz_test_utils.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="application/javascript">
> +
> +    if (isKeyApzEnabled()) {

If isKeyApzEnabled() returns false, you still need at least one assertion in the test for the harness to consider it valid. You could just put a `SimpleTest.ok(true, "Keyboard APZ is disabled")` in an else branch and that should suffice. If you do that you can remove the skip-if for android since it is redundant.
Comment on attachment 8889223 [details]
Bug 1383365 - Fix APZ focus target log parameter order.

https://reviewboard.mozilla.org/r/160276/#review165848
Attachment #8889223 - Flags: review?(botond) → review+
Comment on attachment 8889224 [details]
Bug 1383365 - Add a test to assert async key scrolling happens.

https://reviewboard.mozilla.org/r/160278/#review165850

::: gfx/layers/apz/src/APZCTreeManager.cpp:856
(Diff revision 2)
>        }
>        // Note that the async scroll offset is in ParentLayer pixels
>        aState.mPaintLogger.LogTestData(aMetrics.GetScrollId(), "asyncScrollOffset",
>            apzc->GetCurrentAsyncScrollOffset(AsyncPanZoomController::eForHitTesting));
> +      aState.mPaintLogger.LogTestData(aMetrics.GetScrollId(), "hasAsyncKeyScrolled",
> +          apzc->TestHasAsyncKeyScrolled() ? "true" : "false");

Use boolean true / false values rather than string literals. LogTestData() will convert them to strings if test logging is enabled.

::: gfx/layers/apz/src/AsyncPanZoomController.h:1262
(Diff revision 2)
>    LayerToParentLayerScale mTestAsyncZoom;
>    // Flag to track whether or not the APZ transform is not used. This
>    // flag is recomputed for every composition frame.
>    bool mAsyncTransformAppliedToContent;
> +  // Flag to track whether or not this APZC has ever async key scrolled.
> +  bool mTestHasAsyncKeyScrolled;

Perhaps we should start using bit-fields here, to avoid increasing the amount of space an APZC object takes up for a test-only purpose?

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:75
(Diff revision 2)
> +    function checkHasAsyncKeyScrolled() {
> +      // Get the compositor-side test data from nsIDOMWindowUtils.
> +      var utils = SpecialPowers.getDOMWindowUtils(window);
> +      var compositorTestData = utils.getCompositorAPZTestData();
> +
> +      SimpleTest.ok(compositorTestData.paints.length > 0,

Should having no paints be treated as an error here, or just cause us to return false?

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:92
(Diff revision 2)
> +      // Reconstruct the APZC tree structure in the last paint.
> +      var apzcTree = buildApzcTree(compositorTestData.paints[lastPaintSeqNo]);
> +      var rcd = findRcdNode(apzcTree);
> +
> +      if (rcd) {
> +        return rcd.hasAsyncKeyScrolled === "true";

(If you make the adjustment I requested at the site where we log the data, this might need to change from "true" to "1".)

::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:103
(Diff revision 2)
> +  </script>
> +</head>
> +<body style="height: 500px; overflow: scroll">
> +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1383365">Async key scrolling test</a>
> +  <!-- Put enough content into the page to make it have a nonzero scroll range -->
> +  <div style="height: 1000px"></div>

I would prefer to be on the safer side and make this 5000px tall, for added certainty that it's larger than the viewport height. (I know the body has a specified height of 500px, but I don't know if that's actually respected if the viewport is taller than that.)

Update: looking at the rest of the test, I realized that this necessitates using Home/End instead of PageUp/PageDown. That's fine; it seems fragile to assume that a single PageDown will take you to the bottom anyways.
Attachment #8889224 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #8)
> Comment on attachment 8889224 [details]
> Bug 1383365 - Add a test to assert async key scrolling happens.
> 
> https://reviewboard.mozilla.org/r/160278/#review165850
> 
> ::: gfx/layers/apz/test/mochitest/helper_key_scroll.html:75
> (Diff revision 2)
> > +    function checkHasAsyncKeyScrolled() {
> > +      // Get the compositor-side test data from nsIDOMWindowUtils.
> > +      var utils = SpecialPowers.getDOMWindowUtils(window);
> > +      var compositorTestData = utils.getCompositorAPZTestData();
> > +
> > +      SimpleTest.ok(compositorTestData.paints.length > 0,
> 
> Should having no paints be treated as an error here, or just cause us to
> return false?
> 

I think it definitely makes sense for the final check. If we don't have any APZ test data paints after scrolling up and down the page, I feel like something went wrong.

The sanity check is different. I suppose if we don't have paints at that point, it's okay. I'll update the patches and push to try again.
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d0e2a451e7
Fix APZ focus target log parameter order. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/1db44c2e7903
Add a test to assert async key scrolling happens. r=botond
sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=117918804&repo=mozilla-inbound
Flags: needinfo?(rhunt)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7c09a77c29
Backed out changeset 1db44c2e7903 
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f55e239e9d
Backed out changeset 14d0e2a451e7 for perma crashes in test_wheel_transactions.html
The crashes are during shutdown from the compositor thread not shutting down. This is happening because of one of the compositors not being killed. I traced through things and eventually found we weren't discarding mActiveKeyboardBlock in InputQueue like the other input blocks. Fixing this seemed to fix the problem.

Here's a new try run with the patches:

https://treeherder.mozilla.org/#/jobs?repo=try&author=rhunt@eqrion.net&selectedJob=118294710
Flags: needinfo?(rhunt)
Attached patch discard-key-ibs.patch (obsolete) — Splinter Review
mozreview was acting up on me so I'm uploading the new patch the old fashioned way.
Attachment #8890684 - Flags: review?(bugmail)
Comment on attachment 8890684 [details] [diff] [review]
discard-key-ibs.patch

Review of attachment 8890684 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, looks like we're also missing keyboard block clauses in InputQueue::FindBlockForId() and InputQueue::Clear(). Can you add those also?
Attachment #8890684 - Flags: review?(bugmail)
Attachment #8890684 - Attachment is obsolete: true
Comment on attachment 8890888 [details]
Bug 1383365 - Add in missing code for mActiveKeyboardBlock in InputQueue.

https://reviewboard.mozilla.org/r/162098/#review167374

Thanks!
Attachment #8890888 - Flags: review?(bugmail) → review+
So just to finish my analysis of what was going on with the shutdown hang.

1. Key input creates a KeyboardBlockState
2. The KeyboardBlockState isn't freed by InputQueue in ProcessQueue or Clear
3. KeyboardBlockState holds a refptr to its target APZC
4. The target APZC holds a refptr to a CompositorController which is a CompositorBridgeParent
5. Each CompositorBridgeParent holds a refptr to the compositor thread

So I get how keeping alive the KeyboardBlockState in InputQueue will keep the compositor thread alive, but the APZCTreeManager owning the InputQueue should be destroyed by the CompositorBridgeParent when CBP::ActorDestroy is called. And I don't see why CBP::ActorDestroy wouldn't have been called?

Kats do you have any ideas? I'm just curious exactly what happened.
To be honest I'm not sure. Every time I investigate shutdown issues I have to do it from scratch because the shutdown order of stuff seems to be constantly changing. It might be that the APZ holds the ChromeProcessController alive, which holds on to the widget and prevents shutdown?
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d5da3bd0fa
Fix APZ focus target log parameter order. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/633262ca601a
Add in missing code for mActiveKeyboardBlock in InputQueue. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd10e5ba87d5
Add a test to assert async key scrolling happens. r=botond
You need to log in before you can comment on or make changes to this bug.