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)
Tracking
(firefox57 wontfix, firefox58 fixed, firefox59 fixed)
RESOLVED
FIXED
mozilla59
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [spec][17/11])
Attachments
(4 files)
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 4•7 years ago
|
||
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
|
|
Assignee | |
Updated•7 years ago
|
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 8•7 years ago
|
||
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 20•7 years ago
|
||
A try build of the current state of the patch can be found here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec743f53e34c3299967c58b1748e4400d2f7ac5a
|
|
Assignee | |
Updated•7 years ago
|
Whiteboard: [spec][17/11]
|
||
Comment 21•7 years ago
|
||
| mozreview-review | ||
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.
|
|
Assignee | |
Comment 22•7 years ago
|
||
| mozreview-review-reply | ||
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 23•7 years ago
|
||
| mozreview-review | ||
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 24•7 years ago
|
||
| mozreview-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 25•7 years ago
|
||
| mozreview-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 26•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 27•7 years ago
|
||
| mozreview-review-reply | ||
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 28•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 29•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 30•7 years ago
|
||
| mozreview-review-reply | ||
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
|
|
Assignee | |
Comment 31•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 32•7 years ago
|
||
| mozreview-review-reply | ||
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 33•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 34•7 years ago
|
||
| mozreview-review-reply | ||
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 42•7 years ago
|
||
| mozreview-review-reply | ||
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 43•7 years ago
|
||
| mozreview-review | ||
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+
|
|
Assignee | |
Comment 44•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 45•7 years ago
|
||
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.
|
|
Assignee | |
Comment 46•7 years ago
|
||
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.
|
|
Assignee | |
Comment 47•7 years ago
|
||
(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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 52•7 years ago
|
||
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 53•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8927451 [details]
Bug 1414322 - Refactor sendKeysToElement methods.
https://reviewboard.mozilla.org/r/198754/#review209782
Still looks good.
|
|
||
Updated•7 years ago
|
Flags: needinfo?(ato)
Attachment #8927451 -
Flags: review?(ato)
|
|
||
Comment 54•7 years ago
|
||
| mozreview-review-reply | ||
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 55•7 years ago
|
||
| mozreview-review | ||
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 56•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 57•7 years ago
|
||
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)
|
|
||
Comment 58•7 years ago
|
||
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)
| Comment hidden (mozreview-request) |
|
||
Comment 60•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 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)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 65•7 years ago
|
||
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
|
||
Comment 66•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/c6d4860fcd0a
https://hg.mozilla.org/mozilla-central/rev/3eec27a60959
https://hg.mozilla.org/mozilla-central/rev/35625701a924
https://hg.mozilla.org/mozilla-central/rev/3cbe8afd86c7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
|
|
Assignee | |
Comment 67•7 years ago
|
||
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.
|
|
Assignee | |
Comment 68•7 years ago
|
||
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]
|
||
Comment 69•7 years ago
|
||
| bugherder uplift | ||
https://hg.mozilla.org/releases/mozilla-beta/rev/1ae1021cec72
https://hg.mozilla.org/releases/mozilla-beta/rev/9a6aa369b0d6
https://hg.mozilla.org/releases/mozilla-beta/rev/13dcd46fb286
https://hg.mozilla.org/releases/mozilla-beta/rev/e171f519ec08
Whiteboard: [spec][17/11][checkin-needed-beta] → [spec][17/11]
|
||
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•