1492194 - Write an APZ mochitest to exercise the codepath where the main thread clears the APZ callback transform

Status: RESOLVED WONTFIX

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

Description

[Botond Ballo [:botond]]

In the review of the last patch in bug 1484597 [1], which adds a codepath where the main thread can clear the APZ callback transform, Kats suggested that we write a test that exercises this codepath. Will do so in the bug.

The test will likely need to synthesize a long-tap event, followed by a drag gesture on the selection caret that appears, and verify that the appropriate amount of scrolling occurs.

[1] https://phabricator.services.mozilla.com/D6077#137728

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from comment #0)
> The test will likely need to synthesize a long-tap event, followed by a drag
> gesture on the selection caret that appears, and verify that the appropriate
> amount of scrolling occurs.

Upon some reflection, I think trying to synthesize a drag on a selection caret in a mochitest is in the extended neighbourhood of madness.

The scenario we want to test here boils down to two consecutive calls to ScrollToShowRect(), with the starting state being one where there is a large difference between the layout and visual viewport scroll offsets, and no APZ repaint between the two calls. We can test that fairly directly by taking control of the refresh driver, and using an nsIDOMWindowUtils API (to be added) to trigger the calls to ScrollToShowRect().

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from comment #1)
> using an nsIDOMWindowUtils API (to be added) to trigger the calls to ScrollToShowRect().

Alternatively, we could use the DOM Selection API to trigger these calls.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)
> The scenario we want to test here boils down to two consecutive calls to
> ScrollToShowRect(), with the starting state being one where there is a large
> difference between the layout and visual viewport scroll offsets, and no APZ
> repaint between the two calls.

So I've tried this, but this is actually not sufficient to reproduce the bug.

To reproduce the bug, the second call to ScrollToShowRect() needs to receive a bad rect that was derived from input event coordinates that picked up an out-of-date APZ callback transform. In other words, we really do need the "scroll into view" logic to be triggered via input events of some sort (such as the logic in AccessibleCaretEventHub to handle dragging a text selection caret) rather than using the DOM Selection API or another more direct method.

Comment 4

[Botond Ballo [:botond]]

I've been continuing to make attempts at this.

To resolve the issue described in comment 3, I tried to emulate the operation of the AccessibleCaretEventHub code without having to actually trigger it via input, by synthesizing mouse events, installing an event listener that observes their coordinates (which are affected by the APZ callback transform), and using those coordinates to trigger ScrollToShowRect() calls via an nsIDOMWindowUtils API added for this purpose.

Unfortunately, I'm still failing to trigger the bug I intend to test, in spite of now achieving the conditions described in comment 1. I tried to back out the fix locally, to better understand how the bug was originally triggered, but I'm finding that I can no longer reproduce the bug (as described in bug 1484597 comment 7) with the fix backed out (on either the reduced testcase or the original).

Kats, how would you like to proceed here? I know this is a test you were eager to see added, but I feel like we're hitting diminishing returns from continuing attempts...

Comment 5

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

If you have a test basically written, one thing to try would be to checkout the version of code prior to bug 1484597 landing and see if the test fails there. If it does, and it passes on new code, then great, let's land that. If the test doesn't fail there (i.e. the test would need more effort to get working) then I'm fine with abandoning this effort. Maybe just post your WIP and close the bug as incomplete.

Another thought is that if backing out the fix doesn't bring back the original bug, then maybe we can just back it out in m-c and eliminate the extra codepath entirely. (Referring specifically to the last patch from bug 1484597 since the first three look like change we want to keep). But it's a little risky doing this so I'm not particularly keen on it.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)

If you have a test basically written, one thing to try would be to checkout the version of code prior to bug 1484597 landing and see if the test fails there. If it does, and it passes on new code, then great, let's land that. If the test doesn't fail there (i.e. the test would need more effort to get working) then I'm fine with abandoning this effort. Maybe just post your WIP and close the bug as incomplete.

That sounds reasonable, thanks. I'll test that as suggested, and follow up here.

Another thought is that if backing out the fix doesn't bring back the original bug, then maybe we can just back it out in m-c and eliminate the extra codepath entirely. (Referring specifically to the last patch from bug 1484597 since the first three look like change we want to keep). But it's a little risky doing this so I'm not particularly keen on it.

I'm not keen on doing that either. Conceptually, the change in that patch still makes sense to me, even though triggering a situation where it matters is proving to be tricky.

Comment 7

[Botond Ballo [:botond]]

Attachment: Errors while compiling style crate

I tried to build the version of the code prior to the bug 1484597 patch in question, but ran into a bunch of errors like this while compiling the style crate:

obj-x86_64-pc-linux-gnu/dist/include/nsStyleStruct.h:2066:12: error: no type named 'StyleDisplay' in namespace 'mozilla'

The full error output is attached. Clobbering didn't help.

Comment 8

[Botond Ballo [:botond]]

(Not sure why the bug comment didn't link to the attachment but the attachment is there. Direct link: https://bugzilla.mozilla.org/attachment.cgi?id=9041021)

Comment 9

[Botond Ballo [:botond]]

^ Emilio, do you perhaps know if there's an extra step I need to perform (such as downgrading something) to successfully build the style crate from a September m-c?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Yeah, I suspect you need to either downgrade cbindgen or (preferable, I guess), cherry-pick one of the "keep mozilla-* building" patches in bug 1503401, see that bug for the context.

You need to to copy all the things that are in that cbindgen.toml list and remove the "Style" prefix.

Comment 11

[Botond Ballo [:botond]]

Thanks - I was able to get the relevant revision to build.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)

If you have a test basically written, one thing to try would be to checkout the version of code prior to bug 1484597 landing and see if the test fails there.

Unfortunately, the test doesn't even stand a chance of passing with the tree in that state (whether before or after the commit in question), as it relies on the bugfixes / improvements to our test utilities made in bug 1470504 and bug 1496864 (which is why this bug is marked as depending on those).

Comment 12

[Botond Ballo [:botond]]

And also on an nsIDOMWindowUtils function added in bug 1476221.

Comment 13

[Botond Ballo [:botond]]

I rebased all the dependencies onto the revision in question. With that in place, the behaviour is the same as the behaviour on recent m-c with the fix backed out.

(In reply to Botond Ballo [:botond] from comment #4)

I tried to back out the fix locally, to better understand how the bug was originally triggered, but I'm finding that I can no longer reproduce the bug (as described in bug 1484597 comment 7) with the fix backed out (on either the reduced testcase or the original).

I suspect that the explanation for this is that the bug described in bug 1484597 comment 7 was fixed by the "Use the visual viewport offset in ScrollToShowRect()" patch in that bug. The "clear the APZ callback transform" patch is fixing a potential issue that I noticed while debugging that bug, but doesn't actually occur with that STR with the "Use the visual viewport offset in ScrollToShowRect()" patch in place (but could still occur in other scenarios; in other words, I maintain it's conceptually a valid and important change).

So I think the challenge here is nailing down STR / test steps that do trigger the potential issue fixed by that patch.

Comment 14

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

At any rate I'm fine with abandoning the mochitest since it seems like too much effort at this point.

Comment 15

[Botond Ballo [:botond]]

While it would be nice to improve test coverage of scroll offset sync issues, this particular issue has proven tricky to write a test for, and the amount of time elapsed since the fix makes it even trickier, so I'm going to wontfix and move on.