1534584 - Remove deprecated legacy actions from AccessibleCaret tests

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With bug 1354578 we are working to get rid of the legacy actions implementation in Marionette. But as noticed some layout tests still depend on those, and after some first quick tries I haven't found a way to get those converted.

The tests can be found here:
https://searchfox.org/mozilla-central/search?q=layout%2Fbase%2Ftests%2Fmarionette%2F&path=

Ting-Yu Lin, would you mind to have a look at those tests? It would be good to know about the future of those. Also why specifically Marionette has been used here and not browser chrome or mochitests.

Note that this partly blocks my work to reach a better conformance with the WebDriver specification due to old and unmaintained code.

Thanks!

Comment 1

[Ting-Yu Lin [:TYLin] (UTC-8)]

Ting-Yu Lin, would you mind to have a look at those tests? It would be good to know about the future of those. Also why specifically Marionette has been used here and not browser chrome or mochitests.

test_accessiblecaret_cursor_mode.py and test_accessiblecaret_selection_mode.py are important tests for AccessibleCaret, i.e. the selection handles when long-pressing on text on touchable device. Marionette was chosen because flick can simulate the user's movement of the caret. I'm not aware browser chrome or mochitests can do something similar.

I think rewrite the tests in browser chrome or mochitests is out of scope beacuse we need to rewrite selection.py [1] which the two tests havily rely on.

I saw new APIs was added in Bug 1533786. The tests use only flick, key_down, key_up in the legacy action. Is it possible to rewrite flick in terms of the new APIs? key_down and key_up should be covered already.

With bug 1354578 we are working to get rid of the legacy actions implementation in Marionette. But as noticed some layout tests still depend on those, and after some first quick tries I haven't found a way to get those converted.

I'm curious to know what have you tried, and what fails. Note that by default, AccessibleCaret is not working with mouse actions, but we can turn on layout.accessiblecaret.hide_carets_for_mouse_input preference to have it show up with mouse actions. Also the coordinate is on the element while pointer_move defaults to viewport [2]. That's something I can think of that your attempt might have failed.

In all, if this is blocking you immediately, I can looking into it, and get rid of legacy actions in the two tests. Otherwise, if you have chance to get the tests work under new APIs, I'm also happy to review the patch.

[1] https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/testing/marionette/client/marionette_driver/selection.py
[2] https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/testing/marionette/client/marionette_driver/marionette.py#93-94

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #1)

test_accessiblecaret_cursor_mode.py and test_accessiblecaret_selection_mode.py are important tests for AccessibleCaret, i.e. the selection handles when long-pressing on text on touchable device. Marionette was chosen because flick can simulate the user's movement of the caret. I'm not aware browser chrome or mochitests can do something similar.

I think rewrite the tests in browser chrome or mochitests is out of scope beacuse we need to rewrite selection.py [1] which the two tests havily rely on.

Alright. So lets see...

I saw new APIs was added in Bug 1533786. The tests use only flick, key_down, key_up in the legacy action. Is it possible to rewrite flick in terms of the new APIs? key_down and key_up should be covered already.

I tried to do that with the Action primitives pointerDown, pointerMove, pointerUp, but wasn't lucky. Maybe I just missed a small thing. If you need flick, you would have to create a shared module in the test folder both test files could make use of.

With bug 1354578 we are working to get rid of the legacy actions implementation in Marionette. But as noticed some layout tests still depend on those, and after some first quick tries I haven't found a way to get those converted.

I'm curious to know what have you tried, and what fails. Note that by default, AccessibleCaret is not working with mouse actions, but we can turn on layout.accessiblecaret.hide_carets_for_mouse_input preference to have it show up with mouse actions.

Oh, that might have been the problem here. I used the pointerType mouse(which is the default), but didn't select touch. If enabling mouse actions would be ok, it might be easier. Note that I can't remember that we have added tests for touch yet, so it might fail. Also the new actions will only work in content context.

Also the coordinate is on the element while pointer_move defaults to viewport [2]. That's something I can think of that your attempt might have failed.

Yes, for now it would be fine to use the moz:useNonSpecCompliantPointerOrigin (https://firefox-source-docs.mozilla.org/testing/geckodriver/Capabilities.html#moz-usenonspeccompliantpointerorigin) capability to keep the old behavior. If there is time to make it without the capability, we would appreciate that.

In all, if this is blocking you immediately, I can looking into it, and get rid of legacy actions in the two tests. Otherwise, if you have chance to get the tests work under new APIs, I'm also happy to review the patch.

Sadly I won't have the time to work on it myself. But we would really appreciate if we could get rid of using the legacy actions here. It blocks a refactoring we have to do, which itself is necessary to further improve various action endpoints.

Comment 3

[Ting-Yu Lin [:TYLin] (UTC-8)]

Add this to my backlog.

Comment 4

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: Bug 1534584 Part 2 - Remove legacy_actions from AccessibleCaret tests.

The idea is to re-implement flick() and send_keys() required by
AccessibleCaret tests. The two new methods perform action immediately.
Thus manually calling perform() is not required.

Other minor notes:

• Delete skip_if_not_rotatable() because it's unused.

• Change the dragging distance from 50px to 40px in
  test_carets_not_jump_when_dragging_to_editable_content_boundary and
  test_caret_not_jump_to_front_when_dragging_up_to_editable_content_boundary
  to avoid MoveTargetOutOfBoundsException. Changing the distance doesn't
  alter the meaning of the tests.

• Some of the flick() calls has duration=1. Remove them, and use the
  default 200ms.

Comment 5

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: Bug 1534584 - Remove legacy_actions from AccessibleCaret tests.

• Re-implement flick() and send_keys() required by AccessibleCaret tests.

• Delete skip_if_not_rotatable() because it's unused.

• Change the dragging distance from 50px to 40px in
  test_carets_not_jump_when_dragging_to_editable_content_boundary and
  test_caret_not_jump_to_front_when_dragging_up_to_editable_content_boundary
  to avoid MoveTargetOutOfBoundsException. Changing the distance doesn't
  alter the meaning of the tests.

Comment 6

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: Bug 1534584 Part 1 - Move selection.py to layout/base/tests/marionette/.

selection.py is used only by AccessibleCaret tests.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d8c85e1f976
Part 1 - Move selection.py to layout/base/tests/marionette/. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/aad29c67d0ad
Part 2 - Remove legacy_actions from AccessibleCaret tests. r=whimboo

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6d8c85e1f976
https://hg.mozilla.org/mozilla-central/rev/aad29c67d0ad

Comment 9

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/6d8c85e1f976c8ea68fc501cd144bfa717abe299
Bug 1534584 Part 1 - Move selection.py to layout/base/tests/marionette/. r=whimboo

https://hg.mozilla.org/projects/ash/rev/aad29c67d0ad810a4de5dac7bd8728e5d5b56d36
Bug 1534584 Part 2 - Remove legacy_actions from AccessibleCaret tests. r=whimboo