Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

The test TestClickScrolling.test_should_scroll_elements_if_click_point_is_out_of_view_but_element_is_in_view in testing/marionette/harness/marionette_harness/tests/unit/test_click_scrolling.py (a mouthful!) is wrong.

The test assumes the click point is at the centre of the element, whereas the WebDriver standard uses the _in-view_ centre point to perform clicks.

This means that if parts of the element is rendered outside the viewport, the click point is calculated from the visible portion of the element that is seen in the viewport.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 7 years ago

So mind explaining me why we do not need the tests at all then? It's a bit hard for me to understand. Maybe you will add specific tests with the patch on bug 1321516?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #2)
> So mind explaining me why we do not need the tests at all then? It's a bit
> hard for me to understand. Maybe you will add specific tests with the patch
> on bug 1321516?

I will quote the commit message, which should help explain the motivation:

> The test assumes the click point is the centre of the element, whereas
> WebDriver uses the _in-view_ centre point to perform clicks.
> 
> If parts of the element is rendered outside the viewport, the click
> point is calculated from visible portion of the element that is seen in
> the viewport.
> 
> This means that if any portion of an element is within the boundaries of
> the viewport, it is clickable.  If it is not, then it is not interactable.
> 
> This change is unfortunately not accompanied with any
> implementation changes, but prepares Marionette for the changes
> to the Element Click implementation that will come as part of

The wrong behaviour is only triggered when the WebDriver-conforming Element Click implementation is being used.  The tests for that feature are located in testing/marionette/harness/marionette_harness/tests/unit/test_click.py TestClick (http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_click.py#132), but the scroll tests are not run with that enabled.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago

Why are the scroll tests not run with that enabled? I don't see that, sorry. TestClick is a subclass of TestLegacyClick and as such runs all the tests again, which have been added to TestLegacyClick.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Why are the scroll tests not run with that enabled? I don't see that, sorry.
> TestClick is a subclass of TestLegacyClick and as such runs all the tests
> again, which have been added to TestLegacyClick.

We _don’t_ run the scroll tests with the Element Click implementation conforming to WebDriver because it has so far only had a limited amount of test coverage.  As part of my work on https://bugzilla.mozilla.org/show_bug.cgi?id=1321516, I discovered that it fails this test.  This test is wrong per the specification, which is the motivation for deleting it.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 7 years ago

So normally I would suggest we could run test_click_scrolling.py also with the new Element Click implementation, but it has other bugs and dependencies that make that impossible.

I’m slowly working my way through picking patches out of the changeset you currently see attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 and fixing the things that can be fixed individually in separate bugs.  They are marked as blockers for the overall bug to enable WebDriver clicks in Marionette.

In any case, deleting test_should_scroll_elements_if_click_point_is_out_of_view_but_element_is_in_view makes sense as an individual change because the test is wrong and we shouldn’t support that use case, and we will start running the test with the new code path once we make the switch.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8860953 [details]
Bug 1359043 - Remove incorrect click/scroll test

https://reviewboard.mozilla.org/r/132958/#review136452

	Pulsebot
	Comment 8 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38154f2aad4e
Remove incorrect click/scroll test r=whimboo

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/38154f2aad4e