Closed Bug 1348321 Opened 7 years ago Closed 6 years ago

Write more automated tests for async scrollbar dragging

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: botond, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

Bug 1348321 - Extract a scrollbar-dragging helper function.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1348321 - Add a mochitest for dragging a scrollbar inside an SVGEffects item.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1348321 - Add a mochitest for dragging a scrollbar on a fixed transformed scrollframe.
59 bytes, text/x-review-board-request
botond
: review+
Details 
I recently realized that the test I wrote in bug 1346632 is actually the first automated test for async scrollbar dragging.

We should write a few more tests. Some specific ideas:

  - One for the testcase in bug 1326290

  - One for the testcase in bug 1331693

  - One for a simple testcase, that verifies the drag is
    actually going through APZ
Also:
- one for the testcase in bug 1462961
- one for the testcase in bug 1451168
- one for bug 1449478, if we can reduce it down

I'll take this and work on it after we have async-scene-building enabled in CI, because then we should get reliable hit-testing in WebRender as well.
Assignee: nobody → bugmail
Depends on: 1452845
bug 1451168 already has a reftest. and bug 1449478 seems like a lot of work to reduce at this point. But I wrote tests for the other bugs. 3 out of 5 ain't bad.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f95fe37e88b178241e1de84f54670d8806bbc0
The patches here apply on top of the patches in bug 1468545.
Comment on attachment 8985358 [details]
Bug 1348321 - Extract a scrollbar-dragging helper function.

https://reviewboard.mozilla.org/r/250990/#review257560
Attachment #8985358 - Flags: review?(botond) → review+
Comment on attachment 8985358 [details]
Bug 1348321 - Extract a scrollbar-dragging helper function.

https://reviewboard.mozilla.org/r/250990/#review257594

::: gfx/layers/apz/test/mochitest/helper_bug1326290.html:36
(Diff revision 1)
> -  yield synthesizeNativeMouseEvent(scrollableDiv, mouseX, mouseY + 20, nativeMouseUpEventMsg());
>  
>    // the events above might be stuck in APZ input queue for a bit until the
>    // layer is activated, so we wait here until the scroll event listener is
> -  // triggered. We do this by not passing testDriver to the mouse-up synth
> -  // function above.
> +  // triggered.
> +  yield;

Is it possible that the scroll event will fire earlier, while we're waiting for the observer callback to be fired for one of the mouse events above?

Or is that fine, as long as the number of yields matches the number of times we reenter the test driver?
(In reply to Botond Ballo [:botond] from comment #8)
> Is it possible that the scroll event will fire earlier, while we're waiting
> for the observer callback to be fired for one of the mouse events above?

It's possible, yes

> Or is that fine, as long as the number of yields matches the number of times
> we reenter the test driver?

Yeah, it should be fine as long as the number matches. If we want to be more explicit about it I can use the maybeDone() mechanism we used in helper_bug1346632.html.
Comment on attachment 8985359 [details]
Bug 1348321 - Add a mochitest for dragging a scrollbar inside an SVGEffects item.

https://reviewboard.mozilla.org/r/250992/#review257562

::: gfx/layers/apz/test/mochitest/helper_bug1331693.html:10
(Diff revision 1)
> +  <meta name="viewport" content="width=device-width; initial-scale=1.0">
> +  <title>Dragging the mouse on a scrollframe inside an SVGEffects</title>
> +  <script type="application/javascript" src="apz_test_native_event_utils.js"></script>
> +  <script type="application/javascript" src="apz_test_utils.js"></script>
> +  <script type="application/javascript" src="/tests/SimpleTest/paint_listener.js"></script>
> +  <style>

Would it make any difference if we had the <style> after the <script>? I feel like the <style> and the IDs in the <body> sort of go together, in that you need to look at the combination to understand the structure and proportions of the page.
Attachment #8985359 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> > Or is that fine, as long as the number of yields matches the number of times
> > we reenter the test driver?
> 
> Yeah, it should be fine as long as the number matches. If we want to be more
> explicit about it I can use the maybeDone() mechanism we used in
> helper_bug1346632.html.

Thanks for clarifying; this is fine.
Comment on attachment 8985360 [details]
Bug 1348321 - Add a mochitest for dragging a scrollbar on a fixed transformed scrollframe.

https://reviewboard.mozilla.org/r/250994/#review257602
Attachment #8985360 - Flags: review?(botond) → review+
Thanks very much for writing these tests!
(In reply to Botond Ballo [:botond] from comment #10)
> Would it make any difference if we had the <style> after the <script>? I
> feel like the <style> and the IDs in the <body> sort of go together, in that
> you need to look at the combination to understand the structure and
> proportions of the page.

Sure, I can swap the order of those. I'll do it in the other test as well.

(In reply to Botond Ballo [:botond] from comment #13)
> Thanks very much for writing these tests!

No problem :)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3250ddd5dd08
Extract a scrollbar-dragging helper function. r=botond
https://hg.mozilla.org/integration/autoland/rev/b70a3c599d79
Add a mochitest for dragging a scrollbar inside an SVGEffects item. r=botond
https://hg.mozilla.org/integration/autoland/rev/0d8d27188067
Add a mochitest for dragging a scrollbar on a fixed transformed scrollframe. r=botond
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: