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

The eleement.isPointerInteractable check does not currently work with <select multiple> because options are rendered in the DOM, and at the container element’s centre point you might find an <option>.

This was accounted for in the WebDriver standard with https://github.com/w3c/webdriver/pull/894/commits/40abcefd6acb86ac64befe2cad4b729b5e566932.

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

Comment on attachment 8860993 [details]
Bug 1359079 - Take <select multiple> into account for obscured click test;

https://reviewboard.mozilla.org/r/133026/#review136066

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:280
(Diff revision 1)
> +        # options are rendered and visible in the DOM, the <select>'s
> +        # centre point will be one of the options.
> +        #
> +        # This tests that the <option> is an inclusive descandant of
> +        # its containing element.
> +        option.click()

I'm missing a check that the click was successful. Cannot we simply check that the specified option is selected now?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8860993 [details]
Bug 1359079 - Take <select multiple> into account for obscured click test;

https://reviewboard.mozilla.org/r/133026/#review136066

> I'm missing a check that the click was successful. Cannot we simply check that the specified option is selected now?

We can do, but this test is actually about option.click() not returning an ‘element click intercepted’ error.  Let me explain.

When clicking an <option> element, because it is an <option>, we test whether its containing element, that is the <select>, is pointer-interactable.  The way we check pointer-interactability is currently to call document.elementsFromPoint at <select>’s in-view centre point.  But the snug is that at the centre point we find an <option> element.

This means the first element of the pointer-interactable paint tree will be <option>.  Since we compare the sequence’s first element, the test will fail.  This is only a problem with <select multiple>, because <option>s are painted individually.

To work around this, I changed the specification to check if the first element is an _inclusive descendant_ of <select>.  It is, so the test passes.  Because of the inclusivity, the test also passes if the first indexed element were to be the targetted element.

I can change the test to be more specific, by clicking the <select>, but I feel it is wrong to test if the <option> is selected/clicked.  Hopefully clicking <select> instead will make it clearer that we don’t expect anything to be selected.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8860993 [details]
Bug 1359079 - Take <select multiple> into account for obscured click test;

https://reviewboard.mozilla.org/r/133026/#review136066

> We can do, but this test is actually about option.click() not returning an ‘element click intercepted’ error.  Let me explain.
> 
> When clicking an <option> element, because it is an <option>, we test whether its containing element, that is the <select>, is pointer-interactable.  The way we check pointer-interactability is currently to call document.elementsFromPoint at <select>’s in-view centre point.  But the snug is that at the centre point we find an <option> element.
> 
> This means the first element of the pointer-interactable paint tree will be <option>.  Since we compare the sequence’s first element, the test will fail.  This is only a problem with <select multiple>, because <option>s are painted individually.
> 
> To work around this, I changed the specification to check if the first element is an _inclusive descendant_ of <select>.  It is, so the test passes.  Because of the inclusivity, the test also passes if the first indexed element were to be the targetted element.
> 
> I can change the test to be more specific, by clicking the <select>, but I feel it is wrong to test if the <option> is selected/clicked.  Hopefully clicking <select> instead will make it clearer that we don’t expect anything to be selected.

I have changed it to click on <select> and improved the comment attached to the assertion in the latest revision.

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

Comment on attachment 8860993 [details]
Bug 1359079 - Take <select multiple> into account for obscured click test;

https://reviewboard.mozilla.org/r/133026/#review136066

> I have changed it to click on <select> and improved the comment attached to the assertion in the latest revision.

For me it would have been just enough to update the comment, which should indicate that no intercepted error gets thrown in such a case. It's not that clear what the expected outcome is by reading the test name, and the comment.

But thank you for doing the update.

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

Comment on attachment 8860993 [details]
Bug 1359079 - Take <select multiple> into account for obscured click test;

https://reviewboard.mozilla.org/r/133026/#review136466

	Mozilla Autoland
	Comment 8 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8e5331a88c30 -d 38154f2aad4e: rebasing 392048:8e5331a88c30 "Bug 1359079 - Take <select multiple> into account for obscured click test; r=whimboo" (tip)
merging testing/marionette/element.js
merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py
merging testing/marionette/interaction.js
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Mozilla Autoland
	Comment 10 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3e96af1aef73 -d 2454031a4836: rebasing 392049:3e96af1aef73 "Bug 1359079 - Take <select multiple> into account for obscured click test; r=whimboo" (tip)
merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py
merging testing/marionette/interaction.js
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Mozilla Autoland
	Comment 11 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3e96af1aef73 -d 42efa1423067: rebasing 392090:3e96af1aef73 "Bug 1359079 - Take <select multiple> into account for obscured click test; r=whimboo" (tip)
merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py
merging testing/marionette/interaction.js
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Mozilla Autoland
	Comment 13 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0884a8d3aec6 -d 472c91312420: rebasing 392104:0884a8d3aec6 "Bug 1359079 - Take <select multiple> into account for obscured click test; r=whimboo" (tip)
merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py
merging testing/marionette/interaction.js
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/test_click.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 15 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5780512b2a96
Take <select multiple> into account for obscured click test; r=whimboo

	Carsten Book [:Tomcat]
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5780512b2a96