Write an APZ mochitest to exercise the codepath where the main thread clears the APZ callback transform
Categories
(Core :: Panning and Zooming, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox64 | --- | affected |
People
(Reporter: botond, Assigned: botond)
References
Details
Attachments
(1 file)
(deleted),
text/plain
|
Details |
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
(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)
Assignee | ||
Comment 9•6 years ago
|
||
^ 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•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
Thanks - I was able to get the relevant revision to build.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
Assignee | ||
Comment 12•6 years ago
|
||
And also on an nsIDOMWindowUtils
function added in bug 1476221.
Assignee | ||
Comment 13•6 years ago
|
||
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•6 years ago
|
||
At any rate I'm fine with abandoning the mochitest since it seems like too much effort at this point.
Assignee | ||
Comment 15•4 years ago
|
||
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.
Description
•