Closed Bug 1414322 Opened 7 years ago Closed 7 years ago

Switch to WebDriver conformant interactability checks for event.sendKeysToElement

Categories

(Remote Protocol :: Marionette, enhancement, P2)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [spec][17/11])

Attachments

(4 files)

Bug 1414322 - Refactor keyboard and visibility tests.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1414322 - Refactor sendKeysToElement methods.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.
59 bytes, text/x-review-board-request
yzen
: review+
Details 
On bug 1321516 we changed clickElement to make use of the WebDriver conformant interactability checks. But while discussing with Andreas about our strategy to remove `isVisible`, I noticed that `event.sendKeysToElement()` still depends on it, and would also need to be updated.

It would be great to get this fixed soon, so that we could also ship it with Firefox 58.

Maybe we need a new capability here like `moz:webdriverSendKeys`, to allow people to switch back to the legacy code.
Just to note the current typing tests we have are a real mess. It has been taken me a while to clean those up. But now we are in a much better shape.

The patch as attached is half-baked and not ready yet.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Blocks: 1417925
No longer blocks: webdriver
I'm still not done yet in getting a better coverage for send_keys added to the Marionette unit tests. As discussed with David probably all of them will be moved to wdspec tests, but that cannot be done yet, given that we still have lots of broken behavior in our implementation.

As part of this bug I will move the tests related to interactability checks over to wdspec tests. Other tests will be moved later once a specific underlying bug has been fixed.
Whiteboard: [spec][17/11]
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review208326

::: testing/marionette/harness/marionette_harness/www/test_accessibility.html:16
(Diff revision 2)
>  </head>
>  <body>
>    <button id="button1">button1</button>
>    <button id="button2" aria-label="button2"></button>
>    <span id="button3">I am a bad button with no accessible</span>
> -  <h1 id="button4">I am a bad button that is actually a header</h1>
> +  <h1 contenteditable id="button4">I am a bad button that is actually a header</h1>

I'm not entirely sure why contenteditable needs to be here, could you give some more info why does it need to be editable? button4 and, especially, button 5 are cases of insaccessible markup that can be seen in the wild.
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review208326

> I'm not entirely sure why contenteditable needs to be here, could you give some more info why does it need to be editable? button4 and, especially, button 5 are cases of insaccessible markup that can be seen in the wild.

Ok, maybe my changes are wrong given that I do not know that much about the a11y internals. So here is what will happen with webdriver spec interactability checks turned on... We only allow to use "Element Send Keys" to elements which actually are focusable and as such have a valid tabIndex, or are contenteditable. Exceptions here are body, and documentElement which will also be allowed.

The h1 element as in use here is not interactable by our webdriver definition, and as such using Send Keys will raise a not interactable exception. Similar to Element Click the a11y checks are performed after the webdriver interactability checks, and as such would not raise the expected failure.

From looking at the test I'm not fully sure what it is testing, but I thought a faked button is what we need, and making it contenteditable should change its markup.

So the question is, if we fail in the interactability checks now, do we still need those a11y tests? If yes, which kind of focusable element should be used?
Comment on attachment 8927450 [details]
Bug 1414322 - Refactor keyboard and visibility tests.

https://reviewboard.mozilla.org/r/198752/#review208450

Assuming that most of these changes are correct.

::: testing/marionette/harness/marionette_harness/tests/unit/test_rendered_element.py:14
(Diff revision 5)
>  #
> -#Unless required by applicable law or agreed to in writing, software
> -#distributed under the License is distributed on an "AS IS" BASIS,
> -#WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> -#See the License for the specific language governing permissions and
> -#limitations under the License.
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.

I kind of feel you’ve refactored this test sufficiently that we can
claim that it has been rewritten and drop the Selenium copyright
header, but I’ll leave this up for you to judge. (-:
Attachment #8927450 - Flags: review?(ato) → review+
Comment on attachment 8927451 [details]
Bug 1414322 - Refactor sendKeysToElement methods.

https://reviewboard.mozilla.org/r/198754/#review208458

It’s a great change to move the isVisible check out of the interaction
function.

::: testing/marionette/event.js:1342
(Diff revision 5)
>  
>  /**
>   * @param {string} keyString
>   * @param {Element} element
> - * @param {Object.<string, boolean>=} opts
>   * @param {Window=} window

This should really be {WindowProxy=}.

::: testing/marionette/interaction.js:415
(Diff revision 5)
> +  }
> +
>    let acc = await a11y.getAccessible(el, true);
>    a11y.assertActionable(acc, el);
> -  event.sendKeysToElement(value, el, {ignoreVisibility: false}, win);
> +
> +  await event.sendKeysToElement(value, el, win);

AFAICT this isn’t an async function?
Attachment #8927451 - Flags: review?(ato) → review+
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

This is excellent work.  I think we need to nail down exactly what
elements are meant to be keyboard-interactable.

::: commit-message-e415a:3
(Diff revision 5)
> +Enables webdriver spec keyboard interactability tests for 'Element Send Keys'
> +by default by re-using the same capability 'moz:webdriverClick' from
> +'Element Click'. It can be disabled by turning off this preference.

I guess this should be documented in the geckodriver README.

::: testing/marionette/interaction.js:416
(Diff revision 5)
> +  // Only focusable elements are keyboard-interactable,
> +  // including body, and the document element.
> +  if (containerEl.tabIndex === -1 &&
> +      !containerEl.isContentEditable &&
> +      containerEl.localName != "body" &&
> +      containerEl !== win.document.documentElement) {
> +    throw new ElementNotInteractableError(
> +        pprint`Element '${el}' is not keyboard-interactable`);
> +  }

First of all, I feel this should be implemented in element.js,
alongside element.isInView().

Using tabIndex is a clever trick, but I don’t think this fully
covers all focusable areas [1] in HTML.  For example, in addition
to the tabindex focus flag, it has provisions for the element not
actually being disabled, not being expressly inert, &c.  See the
table of focusable areas.

That being said, I think this is a pretty good start and probably
covers the 95% of use cases.  For that reason I’m happy to accept
this patch with this (limited) implementation, but there should
probably be a comment that this isn’t exhaustive of ‘keyboard
interactable’ elements.

  [1] https://html.spec.whatwg.org/#focusable-area

::: testing/marionette/interaction.js:435
(Diff revision 5)
> +  }
> +
> +  let acc = await a11y.getAccessible(el, true);
> +  a11y.assertActionable(acc, el);
> +
> +  event.sendKeysToElement(value, el, win);

I also feel that <input type=file>, <input type=time>, and <input
type=date> should be handled in this function.  It is currently
handled in listener.js’ sendKeysToElement function.

::: testing/web-platform/tests/webdriver/tests/element_retrieval/get_active_element.py
(Diff revision 5)
> -def get_active_element(session):
> -    return session.transport.send("GET", "session/%s/element/active" % session.session_id)
> -

I think this should be kept hard-coded in each test.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:18
(Diff revision 5)
> +def test_body_is_interactable(session):
> +    session.url = inline("<input>")
> +
> +    # By default the body has the focus
> +    element = session.find.css("body", all=False)
> +    assert_same_element(session, element, session.active_element)
> +
> +    # Sending TAB moves the focus to the input element
> +    response = send_keys_to_element(session, element, u"\ue004")  # TODO keys.TAB
> +    assert_success(response)
> +
> +    input = session.find.css("input", all=False)
> +    assert_same_element(session, input, session.active_element)

This doesn’t test that the body is interactable.  If you have a key
event listener attached to <body> that should receive input you
type.

The Tab key is unrelated to <body> and is a characteristic of the
browser.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:29
(Diff revision 5)
> +    input = session.find.css("input", all=False)
> +    assert_same_element(session, input, session.active_element)

Let’s not override the "input" global in Python.  Applies throughout
this file.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:33
(Diff revision 5)
> +
> +    input = session.find.css("input", all=False)
> +    assert_same_element(session, input, session.active_element)
> +
> +
> +def test_document_element_is_interactable(session):

The same applies to this test.  This should test that input without
focussing any element (the default focus) should reach an event
handler on the document or window.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:65
(Diff revision 5)
> +def test_not_displayed(session):
> +    session.url = inline("<input style=\"display: none\">")
> +    element = session.find.css("input", all=False)
> +
> +    response = send_keys_to_element(session, element, "foo")
> +    assert_success(response)
> +    assert element.property("value") == ""

As far as I know this element is not focusable either and this
should return an error?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:74
(Diff revision 5)
> +def test_not_visible(session):
> +    session.url = inline("<input style=\"visibility: hidden\">")
> +    element = session.find.css("input", all=False)
> +
> +    response = send_keys_to_element(session, element, "foo")
> +    assert_success(response)
> +    assert element.property("value") == ""

Similarly, a non-visible element isn’t focusable?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:83
(Diff revision 5)
> +def test_readonly_element(session):
> +    session.url = inline("<input readonly>")
> +    element = session.find.css("input", all=False)
> +
> +    response = send_keys_to_element(session, element, "foo")
> +    assert_success(response)
> +    assert element.property("value") == ""

It is likely a bug in the WebDriver spec that readonly isn’t covered.
The ‘focusable area’ definition covers elements that are disabled,
but not readonly.  Please confirm with the HTML spec.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:90
(Diff revision 5)
> +    element = session.find.css("input", all=False)
> +
> +    response = send_keys_to_element(session, element, "foo")
> +    assert_success(response)
> +    assert element.property("value") == ""
> +

Test also <input disabled>, which should return an error.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:95
(Diff revision 5)
> +
> +
> +def test_obscured_element(session):
> +    session.url = inline("""
> +      <input type="text" />
> +      <div style="position: relative; top: -3em; height: 5em; background-color: red"></div>

Nit: Don’t use the colour red as it usually indicates an error in
most other WPT tests.  Otherwise this is an excellent test!

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:10
(Diff revision 5)
> +# 14.3
> +
> +# Step 2: Scroll into view
> +
> +def test_element_outside_of_not_scrollable_viewport(session):
> +    session.url = inline("<input style=\"position: relative; left: -9999px;\">")

Do you mean position: absolute here?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:13
(Diff revision 5)
> +
> +def test_element_outside_of_not_scrollable_viewport(session):
> +    session.url = inline("<input style=\"position: relative; left: -9999px;\">")
> +    element = session.find.css("input", all=False)
> +
> +    element.send_keys("foo")

Please make a raw call instead of relying on the API.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:23
(Diff revision 5)
> +    session.url = inline("<input style=\"margin-top: 102vh;\">")
> +    element = session.find.css("input", all=False)
> +
> +    element.send_keys("foo")
> +    assert is_element_in_viewport(session, element)
> +

Can you add a test for an element that is absolutely outside the
viewport and which scrollIntoView will _fail_ to bring into view?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:26
(Diff revision 5)
> +    session.url = inline("""
> +        <select style="margin-top: 102vh;">
> +          <option value="foo">foo</option>
> +          <option value="bar" id="bar">bar</option>
> +        </select>
> +    """)

Can you add a test where an <option> element, say in a <select
multiple> is physically located outside the viewport?

It is an interesting edge case because Marionette will not actually
bring the <option> element into view because it interacts with the
container element.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:40
(Diff revision 5)
> +# The following scroll tests need clarification on:
> +# https://github.com/w3c/webdriver/issues/1155

Good edge case.

I think the intention here is that, for as long as the element is
inside the viewport, it is interactable.  We should just assume
that whatever scrollIntoView does for this is the right behaviour.
That is to say, I _think_ this test is correct the way it is written.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:281
(Diff revision 5)
> +        let rect = el.getBoundingClientRect();
> +        let viewport_width = window.innerWidth || document.documentElement.clientWidth;
> +        let viewport_height = window.innerHeight || document.documentElement.clientHeight;
> +
> +        // Check if element is outside of the viewport
> +        if (rect.right < 0 || rect.bottom < 0 ||

As I understand it, the X axis of the viewport can be relative too.
Attachment #8927452 - Flags: review?(ato) → review-
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review208326

> Ok, maybe my changes are wrong given that I do not know that much about the a11y internals. So here is what will happen with webdriver spec interactability checks turned on... We only allow to use "Element Send Keys" to elements which actually are focusable and as such have a valid tabIndex, or are contenteditable. Exceptions here are body, and documentElement which will also be allowed.
> 
> The h1 element as in use here is not interactable by our webdriver definition, and as such using Send Keys will raise a not interactable exception. Similar to Element Click the a11y checks are performed after the webdriver interactability checks, and as such would not raise the expected failure.
> 
> From looking at the test I'm not fully sure what it is testing, but I thought a faked button is what we need, and making it contenteditable should change its markup.
> 
> So the question is, if we fail in the interactability checks now, do we still need those a11y tests? If yes, which kind of focusable element should be used?

Thanks for context. So if I understand correctly, the exception that would happen if one would call click or tap on these elements, the test would raise an exception earliere (before accessibility checks), or would excpetions only happen for keyboard? If exceptions would happen for keyboard only, it's a good addition, because a11y checks where primarily focused on mouse/touch.
Comment on attachment 8927450 [details]
Bug 1414322 - Refactor keyboard and visibility tests.

https://reviewboard.mozilla.org/r/198752/#review208450

Tests are/were all passing for this particular commit. Mainly this was only refactoring.

> I kind of feel you’ve refactored this test sufficiently that we can
> claim that it has been rewritten and drop the Selenium copyright
> header, but I’ll leave this up for you to judge. (-:

I'm happy to get this removed. Will help us later anyway to move tests over to wp-tests.
Comment on attachment 8927450 [details]
Bug 1414322 - Refactor keyboard and visibility tests.

https://reviewboard.mozilla.org/r/198752/#review208450

> I'm happy to get this removed. Will help us later anyway to move tests over to wp-tests.

My thought exactly.
Comment on attachment 8927451 [details]
Bug 1414322 - Refactor sendKeysToElement methods.

https://reviewboard.mozilla.org/r/198754/#review208458

> This should really be {WindowProxy=}.

We should change that with a follow-up patch as best for a refactoring given that this affects not only this method but lots of others too.

> AFAICT this isn’t an async function?

No, it's not. Thank you for catching that. Not sure why this was left for the legacy method.
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review208326

> Thanks for context. So if I understand correctly, the exception that would happen if one would call click or tap on these elements, the test would raise an exception earliere (before accessibility checks), or would excpetions only happen for keyboard? If exceptions would happen for keyboard only, it's a good addition, because a11y checks where primarily focused on mouse/touch.

The patch here really only changes the behavior of `Element Send Keys` (so keyboard interaction) and not `Element Click` (mouse interaction). Here a try build without the change to the HTML testcase:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=870df9cdd2e53fd9cbda7842578e5b78a87b6746&selectedJob=147439201

The failing tests are test_send_keys_raises_no_exception, and test_send_keys_raises_element_not_accessible
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> I guess this should be documented in the geckodriver README.

I can do another commit specifically for geckodriver related updates. So long I will keep this issue open.

> I also feel that <input type=file>, <input type=time>, and <input
> type=date> should be handled in this function.  It is currently
> handled in listener.js’ sendKeysToElement function.

This is not part of this patch because handling those elements is not related to scroll into view and keyboard interactability. The patches on this bug should not change any other behavior compared to the legacy mode.

> I think this should be kept hard-coded in each test.

Strange. I thought I reverted this, but looks like I missed it. The fixture does no longer contain this method, so the test should have been failing. But maybe the failure was not visible on try because I didn't pull central for a while, and your patch to unhide those errors was not in place.

> As far as I know this element is not focusable either and this
> should return an error?

As you said last week all visibility checks have been removed for determining the keyboard interactability. I also cannot find those in the spec. So I based my patch on that information. What's correct then?

Basically I understand that an invisible element without taking space at all is not interactable...

> Similarly, a non-visible element isn’t focusable?

Similar question as for the above issue. Lets keep the other issue for tracking it.

> It is likely a bug in the WebDriver spec that readonly isn’t covered.
> The ‘focusable area’ definition covers elements that are disabled,
> but not readonly.  Please confirm with the HTML spec.

I tested (https://jsbin.com/bojegitube) this before, and read-only text boxes clearly get the focus. Also all registered keydown, keypress, and keyup event listeners are getting triggered, but there is no input event.

So this case should be fine.

> Do you mean position: absolute here?

No, I meant `relative`. If you think I should add other tests for different values too, I can do it. But I don't think that changes something for our checks.

https://developer.mozilla.org/en-US/docs/Web/CSS/position#Relative_positioning

> Please make a raw call instead of relying on the API.

Why do you think this is necessary here, and not for all the other places including the interactability checks? I mean if I change this we should change all of them, and that for each and every test module in that folder.

> Can you add a test for an element that is absolutely outside the
> viewport and which scrollIntoView will _fail_ to bring into view?

What should be different to the test above which uses a relative positioned element, which cannot be brought into view?

> Can you add a test where an <option> element, say in a <select
> multiple> is physically located outside the viewport?
> 
> It is an interesting edge case because Marionette will not actually
> bring the <option> element into view because it interacts with the
> container element.

I can do. But while doing that I noticed that Element.focus() by default scrolls the element into view. So the extra step, as we specify in the spec is not really necessary:

https://html.spec.whatwg.org/#focus-management-apis

element.focus([ { preventScroll: true } ])

By default, this method also scrolls the element into view. Providing the preventScroll option and setting it to true prevents this behavior.

> Good edge case.
> 
> I think the intention here is that, for as long as the element is
> inside the viewport, it is interactable.  We should just assume
> that whatever scrollIntoView does for this is the right behaviour.
> That is to say, I _think_ this test is correct the way it is written.

Please note that it is irrelevant if the element is visible in the viewport or not, it's always keyboard interactable. See https://jsbin.com/zejenuwuro, and press Tab to select the input field. Then each keypress will trigger the input event.

> As I understand it, the X axis of the viewport can be relative too.

Can you please explain more? I don't get what you are referring to here. Thanks.
Blocks: 1420888
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> As you said last week all visibility checks have been removed for determining the keyboard interactability. I also cannot find those in the spec. So I based my patch on that information. What's correct then?
> 
> Basically I understand that an invisible element without taking space at all is not interactable...

Just found on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file:

> Note: opacity is used to hide the file input instead of visibility: hidden or display: none, because assistive technology interprets the latter two styles to mean the file input isn't interactive.

So both tests should indeed test for an error.
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review208326

> The patch here really only changes the behavior of `Element Send Keys` (so keyboard interaction) and not `Element Click` (mouse interaction). Here a try build without the change to the HTML testcase:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=870df9cdd2e53fd9cbda7842578e5b78a87b6746&selectedJob=147439201
> 
> The failing tests are test_send_keys_raises_no_exception, and test_send_keys_raises_element_not_accessible

Thanks for the try build. Yes I think the error is correct there. And it makes sense to fail with ElementNotInteractableException. So I think the test should be updated as this: instead of making button4 and button5 interactive, the test should either expect ElementNotInteractableException exception for button4 and button5, or they should be removed from a group that it being tested in test_send_keys_raises_element_not_accessible and test_send_keys_raises_no_exception.
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> First of all, I feel this should be implemented in element.js,
> alongside element.isInView().
> 
> Using tabIndex is a clever trick, but I don’t think this fully
> covers all focusable areas [1] in HTML.  For example, in addition
> to the tabindex focus flag, it has provisions for the element not
> actually being disabled, not being expressly inert, &c.  See the
> table of focusable areas.
> 
> That being said, I think this is a pretty good start and probably
> covers the 95% of use cases.  For that reason I’m happy to accept
> this patch with this (limited) implementation, but there should
> probably be a comment that this isn’t exhaustive of ‘keyboard
> interactable’ elements.
> 
>   [1] https://html.spec.whatwg.org/#focusable-area

It's interaction related code, so I see it more appropriate to be located in interaction.js. But I agree to move this out into a separate method.

Lots of local tests have been shown that with using tabIndex it works perfectly. Maybe I should add tests for any of the cases as pointed out in the HTML spec? What is hard to decide is <area>, and expressly inert, which I would like to move to a differnt bug.

I added a some more checks for disabled, `display: none`, and `visibility: hidden` now while refactoring the code. Please close this issue when you are happy with the update.
Blocks: 1421298
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> This is not part of this patch because handling those elements is not related to scroll into view and keyboard interactability. The patches on this bug should not change any other behavior compared to the legacy mode.

But if you read the specification remote end steps and compare them
to the new function you are introducing, it looks as if Marionette
isn’t spec compliant because <input type=file> and <input type=time>
are handled elsewhere.

> I tested (https://jsbin.com/bojegitube) this before, and read-only text boxes clearly get the focus. Also all registered keydown, keypress, and keyup event listeners are getting triggered, but there is no input event.
> 
> So this case should be fine.

OK, thanks for checking this.
Comment on attachment 8931922 [details]
Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements.

https://reviewboard.mozilla.org/r/202994/#review209028

This looks good , thanks.
Attachment #8931922 - Flags: review?(yzenevich) → review+
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> But if you read the specification remote end steps and compare them
> to the new function you are introducing, it looks as if Marionette
> isn’t spec compliant because <input type=file> and <input type=time>
> are handled elsewhere.

Is that the only concern you currently have? If yes I could introduce a new commit which centralizes everything here. But I really don't want to do a full refactoring. My intent was really only to touch the interactability checks, and well scroll to into view.
In our meeting today we were thinking about adding tests for any available form control to the tests. After checking the WebDriver spec I do not think that this is really the right bug for, given that my patch handles step 6 to 8 of the remote end steps, and sending data is handled in step 10.
Also I was able to use the following code to get all elements identified correctly as keyboard-interactable. Well except for `<input type=file>` which actually cannot be focused directly, but only reached via Tab.

> interaction.isKeyboardInteractable = function(el) {
>   const win = getWindow(el);
>
>   // body and document element are always keyboard-interactable
>   if (el.localName === "body" || el === win.document.documentElement) {
>     return true;
>   }
>
>   el.focus();
>
>   return win.document.activeElement === el;
> }

All the created tests so far are passing. I will continue checking tomorrow.
(In reply to Henrik Skupin (:whimboo) from comment #46)
> Also I was able to use the following code to get all elements identified
> correctly as keyboard-interactable. Well except for `<input type=file>`
> which actually cannot be focused directly, but only reached via Tab.

Please scratch that. I was checking the focus event, which indeed is not firing for `input type=file` elements. But checking for the active element it works great. It means I do not see any downside why we cannot use focus for the following purposes:

1) automatically let it scroll the element into view
2) set the element as the active element

This is all based on https://html.spec.whatwg.org/multipage/interaction.html#focus-management-apis and should be fine to be taken into the spec.

Andreas, what do you think? I would vote strongly to let the keyboard-interactability check be performed by the browser itself.
Flags: needinfo?(ato)
Comment on attachment 8927451 [details]
Bug 1414322 - Refactor sendKeysToElement methods.

Andreas, given your request to refactor the input file/datetime parts, this patch got changed a bit. So please re-review. Thanks.
Attachment #8927451 - Flags: review?(ato)
Comment on attachment 8927451 [details]
Bug 1414322 - Refactor sendKeysToElement methods.

https://reviewboard.mozilla.org/r/198754/#review209782

Still looks good.
Flags: needinfo?(ato)
Attachment #8927451 - Flags: review?(ato)
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> I can do another commit specifically for geckodriver related updates. So long I will keep this issue open.

Also remember to update the change log.

> It's interaction related code, so I see it more appropriate to be located in interaction.js. But I agree to move this out into a separate method.
> 
> Lots of local tests have been shown that with using tabIndex it works perfectly. Maybe I should add tests for any of the cases as pointed out in the HTML spec? What is hard to decide is <area>, and expressly inert, which I would like to move to a differnt bug.
> 
> I added a some more checks for disabled, `display: none`, and `visibility: hidden` now while refactoring the code. Please close this issue when you are happy with the update.

<area> and expressly inert are an edge cases so I’m happy to
leave this for now.

I think the updates for display: none and visibility: hidden look good.

> Is that the only concern you currently have? If yes I could introduce a new commit which centralizes everything here. But I really don't want to do a full refactoring. My intent was really only to touch the interactability checks, and well scroll to into view.

OK, I’m sure it can be fixed later if you don’t want to make
the change here.

> No, I meant `relative`. If you think I should add other tests for different values too, I can do it. But I don't think that changes something for our checks.
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/position#Relative_positioning

OK.

> Why do you think this is necessary here, and not for all the other places including the interactability checks? I mean if I change this we should change all of them, and that for each and every test module in that folder.

It is fine to rely on the hig-level APIs for commands not
under test.  For the commands under test we should use
session.transport.send together with assert_success and
assert_error.  You’re right we should follow up and make the
remainder of the Element Send Keys tests follow that practice.

> What should be different to the test above which uses a relative positioned element, which cannot be brought into view?

I guess I would have used an absolute positioned styling rule
to bring it out of the viewport because I misunderstood how the
relative positioning worked.  In any case, this is indeed what the
earlier test tests.

> I can do. But while doing that I noticed that Element.focus() by default scrolls the element into view. So the extra step, as we specify in the spec is not really necessary:
> 
> https://html.spec.whatwg.org/#focus-management-apis
> 
> element.focus([ { preventScroll: true } ])
> 
> By default, this method also scrolls the element into view. Providing the preventScroll option and setting it to true prevents this behavior.

OK, that actually sounds more ideal.
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review209792

Thanks for the updates, it looks much more solid now.  In our talk
yesterday I actually assumed you would use the trick of attaching a
keypress event listener to the element as well using {mozSystemGroup:
true} and not just for testing, but I guess that would not work so
well for non-interactable elements where you’d time out.

::: testing/marionette/interaction.js:358
(Diff revision 8)
> + * To decide if an element is keyboard-interactable various properties,
> + * and computed CSS styles have to be evaluated. Whereby it has to be taken
> + * into account that the element can be part of a container (eg. option),
> + * and as such the container has to be checked instead.
> + *
> + * @param {DOMElement} el

This should actually be just "Element".  The other references
to "DOMElement" in this file are wrong because there is no such
prototype.

::: testing/marionette/interaction.js:493
(Diff revision 8)
> +  let containerEl = element.getContainer(el);
> +
> +  // TODO: Wait for interactible
> +  if (!interaction.isKeyboardInteractable(el)) {
> +    throw new ElementNotInteractableError(
> +        pprint`Element '${el}' is not keyboard-interactable`);

Drop the single quotes around el for harmony with
interaction.clickElement.

Further I’m not sure “keyboard-interactable” is a useful
error message for users.  It is very much a spec-alese definition.
Perhaps we could rephrase this as "Element <…> could not be
reached using a keyboard"?

(I also agree we could improve the interaction.clickElement error
message.)

::: testing/marionette/interaction.js:496
(Diff revision 8)
> +  // Try to scroll the element or its container into view,
> +  // but do not fail if it's not possible.
> +  if (!element.isInView(containerEl)) {
> +    element.scrollIntoView(containerEl);
> +  }

And this was the thing you said was not necessary because
interaction.focusElement below brings it into view, if possible?

If you haven’t already it is possible you should file a bug
against the spec on this.

::: testing/marionette/interaction.js:507
(Diff revision 8)
> +  if (el.type == "file") {
> +    await interaction.uploadFile(el, value);
> +  } else if ((el.type == "date" || el.type == "time") &&
> +      Preferences.get("dom.forms.datetime")) {
> +    interaction.setFormControlValue(el, value);
> +  } else {
> +    event.sendKeysToElement(value, el, win);
> +  }

This is much clearer, so thanks for making this change.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:11
(Diff revision 8)
> +    return session.transport.send(
> +        "POST",
> +        "/session/{session_id}/element/{element_id}/value".format(
> +            session_id=session.session_id,
> +            element_id=element.id),
> +        {"text": text})

Hmm.  I guess this is fine for the interactability test
specifically, but when we add tests for data sanity and input for
this command, we need to be able to pass in an arbitrary dictionary.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:14
(Diff revision 8)
> +# 14.3 - Step 7, 8: Keyboard interactability checks
> +
> +# Tests for keyboard interactable elements

Drop these.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:30
(Diff revision 8)
> +    element = session.find.css("body", all=False)
> +    result = session.find.css("input", all=False)
> +
> +    response = send_keys_to_element(session, element, "foo")
> +    assert_success(response)
> +    assert_same_element(session, element, session.active_element)

I think maybe we should also assert that the active element is
<body> as a precondition before testing the command, like you do in
the next test?

::: testing/web-platform/tests/webdriver/tests/element_send_keys/interactability.py:54
(Diff revision 8)
> +    assert_success(response)
> +    assert_same_element(session, element, session.active_element)
> +    assert result.property("value") == "foo"
> +
> +
> +def test_iframe_is_interactable(session):

Good test.

::: testing/web-platform/tests/webdriver/tests/element_send_keys/scroll_into_view.py:15
(Diff revision 8)
> +# 14.3
> +
> +# Step 6: Scroll into view

Drop.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:277
(Diff revision 8)
> +        let viewport_width = window.innerWidth || document.documentElement.clientWidth;
> +        let viewport_height = window.innerHeight || document.documentElement.clientHeight;

Not a big thing, but I’d avoid snake-casing here by using an
object which has a bit more parity to rect:

> let viewport = {
>   width: …,
>   height: …,
> };

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:280
(Diff revision 8)
> +
> +        let rect = el.getBoundingClientRect();
> +        let viewport_width = window.innerWidth || document.documentElement.clientWidth;
> +        let viewport_height = window.innerHeight || document.documentElement.clientHeight;
> +
> +        // Check if element is outside of the viewport

I would move this to the docstring instead.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:281
(Diff revision 8)
> +        if (rect.right < 0 || rect.bottom < 0 ||
> +            rect.left > viewport_width || rect.top > viewport_height)
> +            return false;
> +
> +        return true;

Instead of an if-condition you can simplify by just returning
the statement.
Attachment #8927452 - Flags: review?(ato) → review+
Comment on attachment 8927452 [details]
Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement.

https://reviewboard.mozilla.org/r/198756/#review208460

> Can you please explain more? I don't get what you are referring to here. Thanks.

I had a re-read of it and it is fine.
Andreas, please have a look at comment 46 if you haven't had the time yet or simply missed it. If you are fine with this approach I can update the webdriver spec issue. Not sure if this is something we should consider to implement yet, but it would fix all of our missing cases for keyboard-interactability checks.
Flags: needinfo?(ato)
Sorry, I missed this needinfo request.

(In reply to Henrik Skupin (:whimboo) from comment #47)

> (In reply to Henrik Skupin (:whimboo) from comment #46)
> 
> > Also I was able to use the following code to get all elements
> > identified correctly as keyboard-interactable. Well except for
> > `<input type=file>` which actually cannot be focused directly,
> > but only reached via Tab.
> 
> Please scratch that. I was checking the focus event, which indeed
> is not firing for `input type=file` elements. But checking for the
> active element it works great. It means I do not see any downside
> why we cannot use focus for the following purposes:
> 
> 1) automatically let it scroll the element into view 2) set the
> element as the active element
> 
> This is all based on
> https://html.spec.whatwg.org/multipage/interaction.html#focus-mana
> gement- apis and should be fine to be taken into the spec.
> 
> Andreas, what do you think? I would vote strongly to let the
> keyboard-interactability check be performed by the browser itself.

Yes, I think this makes a lot of sense.  We might have to update the
specification to not scroll the element into view first, but that
seems like a minor obstacle.

Do you want to land this changeset as it is first, and then follow
up?

I don’t think that the specification definition of a
keyboard-interactable element needs to change because it is still
correct, but I think it is worthwhile to add an example or a note to
explain how it can be implemented using existing platform APIs.
Flags: needinfo?(ato)
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 0ff75d3c58f5 -d dbc85c882639: rebasing 436824:0ff75d3c58f5 "Bug 1414322 - Refactor keyboard and visibility tests. r=ato"
rebasing 436825:a79ed36c527b "Bug 1414322 - Update Marionette accessibility tests for non keyboard-interactable elements. r=yzen"
rebasing 436826:08fb0b66d210 "Bug 1414322 - Refactor sendKeysToElement methods. r=ato"
merging testing/marionette/event.js
merging testing/marionette/listener.js
rebasing 436827:9282679f14c3 "Bug 1414322 - Use WebDriver conformant interactability checks for sendKeysToElement. r=ato" (tip)
merging testing/marionette/listener.js
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6d4860fcd0a
Refactor keyboard and visibility tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/3eec27a60959
Update Marionette accessibility tests for non keyboard-interactable elements. r=yzen
https://hg.mozilla.org/integration/autoland/rev/35625701a924
Refactor sendKeysToElement methods. r=ato
https://hg.mozilla.org/integration/autoland/rev/3cbe8afd86c7
Use WebDriver conformant interactability checks for sendKeysToElement. r=ato
So far we haven't seen any test failure nor regression. I will keep this patch on central only over the weekend, and request an uplift on Monday.
Test harness related improvement which we would like to enable together with click() by default for Firefox 58. Please get it uplifted. Thanks.
Whiteboard: [spec][17/11] → [spec][17/11][checkin-needed-beta]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: