Closed Bug 1333014 Opened 7 years ago Closed 7 years ago

Return ‘element click intercepted’ error when clicking obscured element

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 4 obsolete files)

59 bytes, text/x-review-board-request
whimboo
: review+
Details
59 bytes, text/x-review-board-request
whimboo
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
Details
59 bytes, text/x-review-board-request
impossibus
: review+
Details
59 bytes, text/x-review-board-request
impossibus
: review+
Details
59 bytes, text/x-review-board-request
impossibus
: review+
Details
59 bytes, text/x-review-board-request
impossibus
: review+
Details
The WebDriver specification recently changed to say that we should return an ‘element click intercepted’ error when clicking an element, which in-view centre point is obscured by another element.

(In part reported at https://github.com/mozilla/geckodriver/issues/243.)
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Note that the patch for this will only work when the `specificationLevel` capability is set to 1 and we are using the WebDriver-conforming element interactability check.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

::: testing/marionette/driver.js:2504
(Diff revision 1)
>    this._checkIfAlertIsPresent();
>  
>    // see toolkit/components/prompts/content/commonDialog.js
>    let {loginContainer, loginTextbox} = this.dialog.ui;
>    if (loginContainer.hidden) {
> -    throw new ElementNotVisibleError("This prompt does not accept text input");
> +    throw new ElementNotInteractable("This prompt does not accept text input");

Try jobs are busted mainly due to s/ElementNotInteractable/ElementNotInteractableError/

::: testing/marionette/error.js:237
(Diff revision 1)
> + *     Point to click.
> + */
> +this.ElementClickInterceptedError = function (obscuredEl, coords) {
> +  let msg;
> +
> +  if (obscuredEl && coords) {

Shouldn't we force a valid element at least? I don't see how this can be optional. If coords could be optional, please mark as such in the docstring.

::: testing/marionette/error.js:242
(Diff revision 1)
> +  if (obscuredEl && coords) {
> +    let doc = obscuredEl.ownerDocument;
> +    let overlayingEl = doc.elementFromPoint(coords.x, coords.y);
> +    msg = error.pprint`Element ${obscuredEl} is not clickable ` +
> +      `at point (${coords.x}, ${coords.y}) ` +
> +      error.pprint`because another element ${overlayingEl} ` +

Why the extra `error.pprint` here?

::: testing/marionette/test_error.js:156
(Diff revision 1)
>  
> -add_test(function test_ElementNotVisibleError() {
> -  let err = new ElementNotVisibleError("foo");
> -  equal("ElementNotVisibleError", err.name);
> +add_test(function test_ElementNotInteractableError() {
> +  let err = new ElementNotInteractableError("foo");
> +  equal("ElementNotInteractableError", err.name);
>    equal("foo", err.message);
>    equal("element not visible", err.status);

This should also fail due to the wrong status line.
Attachment #8833444 - Flags: review?(hskupin) → review-
Comment on attachment 8833445 [details]
Bug 1333014 - Remove unused element.isInteractable API;

https://reviewboard.mozilla.org/r/109678/#review111006
Attachment #8833445 - Flags: review?(hskupin) → review+
Comment on attachment 8833446 [details]
Bug 1333014 - Introduce new exceptions to Python client;

https://reviewboard.mozilla.org/r/109680/#review111008

::: testing/marionette/client/marionette_driver/errors.py:99
(Diff revision 1)
>  class ScriptTimeoutException(MarionetteException):
>      status = "script timeout"
>  
>  
>  class ElementNotVisibleException(MarionetteException):
> +    """Deprecated"""

As done by the last deprecation changes lets also add the version when it can be removed.
Attachment #8833446 - Flags: review?(hskupin) → review+
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

::: testing/marionette/element.js:1017
(Diff revision 1)
>  element.isPointerInteractable = function (el) {
> -  let tree = element.getInteractableElementTree(el, el.ownerDocument);
> +  const doc = el.ownerDocument;
> +  const win = doc.defaultView;
> +
> +  let tree = element.getPointerInteractablePaintTree(el, doc)
> +      .filter(el => win.getComputedStyle(el).opacity === "1");

So what about values of 0 < x < 1? The element should also be interactable for those values, right?

Also I think you shoudl use .getPropertyValue('opacity').

::: testing/marionette/element.js:1062
(Diff revision 1)
>   * of any rendered scrollbars.
>   *
>   * @param {DOMElement} el
>   *     Element to determine if is pointer-interactable.
>   * @param {DOMDocument} doc
>   *     Current browsing context's active document.

Please remove.

::: testing/marionette/element.js:1065
(Diff revision 1)
>   *     Element to determine if is pointer-interactable.
>   * @param {DOMDocument} doc
>   *     Current browsing context's active document.
>   *
>   * @return {Array.<DOMElement>}
>   *     Sequence of non-opaque elements in paint order.

non-transparent? See my question above.

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:206
(Diff revision 1)
>          self.marionette.find_element(By.TAG_NAME, "div").click()
> +
> +    def test_input_file(self):
> +        self.marionette.navigate(inline("<input type=file>"))
> +        with self.assertRaises(errors.InvalidArgumentException):
> +            self.marionette.find_element(By.TAG_NAME, "input").click()

Please also add a non-container test case.

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:229
(Diff revision 1)
> +            <script>
> +            window.clicked = false;
> +
> +            let link = document.querySelector("#obscured");
> +            link.addEventListener("click", () => window.clicked = true);
> +            </script>"""))

Can we make the HTML code in test_click_element_obscured_by_absolute_positioned_element re-usable?

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:237
(Diff revision 1)
> +        obscured = self.marionette.find_element(By.ID, "obscured")
> +
> +        overlay.click()
> +        with self.assertRaises(errors.ElementClickInterceptedException):
> +            obscured.click()
> +        self.assertFalse(self.marionette.execute_script("return window.clicked", sandbox=None))

This needs a Wait().until() or an async script call. Otherwise we would have a race condition here.

::: testing/marionette/interaction.js:89
(Diff revision 1)
>   * The element is scrolled into view before visibility- or interactability
>   * checks are performed.
>   *
>   * Selenium-style visibility checks will be performed if |specCompat|
>   * is false (default).  Otherwise pointer-interactability checks will be
> - * performed.  If either of these fail an {@code ElementNotVisibleError}
> + * performed.  If either of these fail an {@code ElementNotInteractable}

s/ElementNotInteractable/ElementNotInteractableError/

::: testing/marionette/interaction.js:106
(Diff revision 1)
>   * @param {boolean=} strict
>   *     Enforce strict accessibility tests.
>   * @param {boolean=} specCompat
>   *     Use WebDriver specification compatible interactability definition.
>   *
> - * @throws {ElementNotVisibleError}
> + * @throws {ElementNotInteractable}

s/ElementNotInteractable/ElementNotInteractableError/

::: testing/marionette/interaction.js:115
(Diff revision 1)
>   *     If |strict| is true and element is not accessible.
>   * @throws {InvalidElementStateError}
>   *     If |el| is not enabled.
>   */
>  interaction.clickElement = function*(el, strict = false, specCompat = false) {
> -  let win = getWindow(el);
> +  const win = getWindow(el);

You are not using getWindow() in the other added code lines of this patch. Is there a reason for?

::: testing/marionette/interaction.js:120
(Diff revision 1)
> -  let win = getWindow(el);
> -  let a11y = accessibility.get(strict);
> +  const win = getWindow(el);
> +  const doc = win.document;
> +  const a11y = accessibility.get(strict);
>  
> -  let visibilityCheckEl  = el;
> -  if (el.localName == "option") {
> +  // step 3
> +  if (el.localName == "input" && el.type == "file") {

As by the spec you also have to check for a valid container. If there is none the same error has to be thrown.

::: testing/marionette/interaction.js:131
(Diff revision 1)
> +
> +  let inView = false;
>    if (specCompat) {
> -    if (!element.isPointerInteractable(visibilityCheckEl)) {
> +    // step 4
> +    if (!element.isInView(containerEl)) {
>        element.scrollIntoView(el);

If I read the spec correctly the container has to be scrolled into view and not the element itself.

::: testing/marionette/interaction.js:135
(Diff revision 1)
> +    if (!element.isInView(containerEl)) {
>        element.scrollIntoView(el);
>      }
> -    interactable = element.isPointerInteractable(visibilityCheckEl);
> +
> +    // step 5
> +    // TODO(ato): wait for containerEl to be in view

When will this be done? Do we expect races in the interim?

::: testing/marionette/interaction.js:144
(Diff revision 1)
> -    interactable = element.isVisible(visibilityCheckEl);
> +    inView = element.isVisible(containerEl);
> +  }
> +
> +  // step 6
> +  if (!inView) {
> +    throw new ElementNotInteractableError();

Lets add a comment to make the failure more clear.

::: testing/marionette/interaction.js:157
(Diff revision 1)
>    }
>  
> +  // step 8
>    if (!atom.isElementEnabled(el)) {
>      throw new InvalidElementStateError("Element is not enabled");
>    }

That's not part of the spec. Does it have to be added?

::: testing/marionette/interaction.js:175
(Diff revision 1)
>        el.click();
>      }
>  
>    // content elements
>    } else {
> +    // step 9

This is still step 8.

::: testing/marionette/interaction.js:186
(Diff revision 1)
>    }
> +
> +  // step 10
> +  yield interaction.flushEventLoop(win);
> +
> +  // step 11

This is step 9.

::: testing/marionette/interaction.js:269
(Diff revision 1)
> + *     Promise is accepted once event queue is flushed, or rejected if
> + *     |win| is unloaded before the queue can be flushed.
> + */
> +interaction.flushEventLoop = function* (win) {
> +  return new Promise((resolve, reject) => {
> +    win.addEventListener("unload", reject);

Missing once option. Also we have to get it removed before calling resolve.

Btw. I don't see how this is related to any `unload` events happening. With a frame rate of 60 or higher the chances to catch an unload event is kinda low.
Attachment #8833447 - Flags: review?(hskupin) → review-
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

> Shouldn't we force a valid element at least? I don't see how this can be optional. If coords could be optional, please mark as such in the docstring.

This needs to be optional because we reconstruct errors from JSON when they are passed from the listener to the chrome.  But you’re right they should be marked as optional in the API documentation.

> Why the extra `error.pprint` here?

It annotates `overlayingEl` with some additional information.  See testing/marionette/error.js.

> This should also fail due to the wrong status line.

My initial patch reused the `element not visible` error in the Python client, but I later changed it to use a new error type.  This explains why this line was left untouched.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> So what about values of 0 < x < 1? The element should also be interactable for those values, right?
> 
> Also I think you shoudl use .getPropertyValue('opacity').

The element should not be interactable if its opacity is less than 1.0.

Fixed the call to `getPropertyValue`.

> non-transparent? See my question above.

Because the spec says so.  However, what is now returned from this function does include opaque elements, so I’ve addressed that.

> Please also add a non-container test case.

All the tests are non-container tests, actually.  But I’ve now added a container test (clicking `<option>` in `<select>`).

However, as part of this I discovered that the specification is wrong about how to handle `<datalist><option>foo</option></datalist>` is wrong.  I filed https://github.com/w3c/webdriver/issues/735 about this.

> Can we make the HTML code in test_click_element_obscured_by_absolute_positioned_element re-usable?

I have made the change.

> This needs a Wait().until() or an async script call. Otherwise we would have a race condition here.

I don’t think we do: `overlay.click()` is supposed to be synchronous.

> You are not using getWindow() in the other added code lines of this patch. Is there a reason for?

`getWindow` is used throughout testing/marionette/interaction.js.  I think you mean why it is not useed in testing/marionette/element.js?  That’s because it’s an internal helper function in interaction.js.  Maybe we should consider making it available in element.js as a public API in the future.

> As by the spec you also have to check for a valid container. If there is none the same error has to be thrown.

I recently fixed this in the spec: https://github.com/w3c/webdriver/commit/962944cd92341e77f30a784821b3c7105a3fbc9d

> If I read the spec correctly the container has to be scrolled into view and not the element itself.

This is true.  Fixed.

> When will this be done? Do we expect races in the interim?

I expect we can fix this once the generalised wait utility in https://bugzilla.mozilla.org/show_bug.cgi?id=1319237 lands.

I don’t expect it will cause many issues not to have this.  Its purpose is to wait for the scroll to complete.

> That's not part of the spec. Does it have to be added?

It does, I recently added this to the specification: https://github.com/w3c/webdriver/commit/ca7998731684a8b4505c93d1ea255733781f7b6e

> This is still step 8.

The commits https://github.com/w3c/webdriver/commit/962944cd92341e77f30a784821b3c7105a3fbc9d, https://github.com/w3c/webdriver/commit/36131dbc1cd85bf2da3c72d7225399c2f63571c4, and https://github.com/w3c/webdriver/commit/ca7998731684a8b4505c93d1ea255733781f7b6e caused the step numbers to be correct.

> This is step 9.

The commits https://github.com/w3c/webdriver/commit/962944cd92341e77f30a784821b3c7105a3fbc9d, https://github.com/w3c/webdriver/commit/36131dbc1cd85bf2da3c72d7225399c2f63571c4, and https://github.com/w3c/webdriver/commit/ca7998731684a8b4505c93d1ea255733781f7b6e caused the step numbers to be correct.

> Missing once option. Also we have to get it removed before calling resolve.
> 
> Btw. I don't see how this is related to any `unload` events happening. With a frame rate of 60 or higher the chances to catch an unload event is kinda low.

Good point about removing it if `requestAnimationFrame` resolves the promise and about adding `{once: true}`.

I didn’t find a particularly clean way to remove the event apart from this:

```js
interaction.flushEventLoop = function* (win) {
  let unloadEv;
  return new Promise((resolve, reject) => {
    unloadEv = reject;
    win.addEventListener("unload", unloadEv, {once: true});
    win.requestAnimationFrame(resolve);
  }).then(() => {
    win.removeEventListener("unload", unloadEv);
  });
};
```

Ideas?

The unload event does trigger in two of our unit tests, so I’d say the chance an event like this occurring before `requestAnimationFrame` is quite high.  It seems to trigger when we click something that takes us away from the current document.  In these cases, `requestAnimationFrame` calls are blocked.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

> This needs to be optional because we reconstruct errors from JSON when they are passed from the listener to the chrome.  But you’re right they should be marked as optional in the API documentation.

How does JSON play into account here? Also for driver commands we have to decode from JSON and have required parameters. Especially for this error type I cannot see that the underlying element can be optional.

> It annotates `overlayingEl` with some additional information.  See testing/marionette/error.js.

But you don't use it in the line before too where placeholders are getting replaced by variable values. What's the difference between those two lines?
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> The element should not be interactable if its opacity is less than 1.0.
> 
> Fixed the call to `getPropertyValue`.

I checked the spec again and I feel there is something clearly wrong:

> A pointer-interactable element is defined to be the first non-transparent element, defined by the paint order found at the center point of its rectangle that is inside the viewport, excluding the size of any rendered scrollbars. 

and

> An element is considered transparent if its opacity style property has the computed value of "1". 

Why is opacity=1 transparent? This should be full opaque. Instead 0 is fully transparent.

Also why should translucent elements not be interactable? As the first quote says: "first non-transparent element". As I read this it includes any value > 0.0.

> All the tests are non-container tests, actually.  But I’ve now added a container test (clicking `<option>` in `<select>`).
> 
> However, as part of this I discovered that the specification is wrong about how to handle `<datalist><option>foo</option></datalist>` is wrong.  I filed https://github.com/w3c/webdriver/issues/735 about this.

Great finding!

> I don’t think we do: `overlay.click()` is supposed to be synchronous.

It means that all attached event handlers are processed before the click() command returns?

> It does, I recently added this to the specification: https://github.com/w3c/webdriver/commit/ca7998731684a8b4505c93d1ea255733781f7b6e

Hm, this landed 4 days ago. Maybe I missed to refresh the web page. Thanks for pointing that out. Btw. how long will the issue be shown in the spec? Does it need a signoff?

> The commits https://github.com/w3c/webdriver/commit/962944cd92341e77f30a784821b3c7105a3fbc9d, https://github.com/w3c/webdriver/commit/36131dbc1cd85bf2da3c72d7225399c2f63571c4, and https://github.com/w3c/webdriver/commit/ca7998731684a8b4505c93d1ea255733781f7b6e caused the step numbers to be correct.

Then it should be moved up before the chrome handling part.

> Good point about removing it if `requestAnimationFrame` resolves the promise and about adding `{once: true}`.
> 
> I didn’t find a particularly clean way to remove the event apart from this:
> 
> ```js
> interaction.flushEventLoop = function* (win) {
>   let unloadEv;
>   return new Promise((resolve, reject) => {
>     unloadEv = reject;
>     win.addEventListener("unload", unloadEv, {once: true});
>     win.requestAnimationFrame(resolve);
>   }).then(() => {
>     win.removeEventListener("unload", unloadEv);
>   });
> };
> ```
> 
> Ideas?
> 
> The unload event does trigger in two of our unit tests, so I’d say the chance an event like this occurring before `requestAnimationFrame` is quite high.  It seems to trigger when we click something that takes us away from the current document.  In these cases, `requestAnimationFrame` calls are blocked.

Thinking more about this we would still have a race condition here. Right now we are doing the click before calling this method. It means nothing guaranties that that unload event has not happened yet. As such we would miss it, and would not detect a page load.

What we would need is to register the unload handler before we actually click the element. We could do that by passing in a callback to `flushEventLoop()` and calling it inside the new promise. That way we are flexible and could extend the coverage later for other actions too.

Beside that we should make sure to pass the reject along, so that the calling code knows when an unload event has been fired, and can react accordingly.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

> How does JSON play into account here? Also for driver commands we have to decode from JSON and have required parameters. Especially for this error type I cannot see that the underlying element can be optional.

When an error is thrown in the content frame script we serialise it to JSON before passing it via IPC message to chrome.  We then deserialise it by constructing a new error instance and assigning the relevant properties.  To be able to construct the blank error instance, `obscuredEl` and `coords` need to be optional since the deserialisation only assigns the properties `message` and `stack`.

> But you don't use it in the line before too where placeholders are getting replaced by variable values. What's the difference between those two lines?

The intention is to sugar `obscuredEl` and `overlayingEl` so it is easier to identify the elements in the error message.  We don’t need to include type information for the coordinates.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> I checked the spec again and I feel there is something clearly wrong:
> 
> > A pointer-interactable element is defined to be the first non-transparent element, defined by the paint order found at the center point of its rectangle that is inside the viewport, excluding the size of any rendered scrollbars. 
> 
> and
> 
> > An element is considered transparent if its opacity style property has the computed value of "1". 
> 
> Why is opacity=1 transparent? This should be full opaque. Instead 0 is fully transparent.
> 
> Also why should translucent elements not be interactable? As the first quote says: "first non-transparent element". As I read this it includes any value > 0.0.

The second quote is wrong, I think.  It should say that an element is considered transparent if its opacity style property _does not have_ the computed value of "1".

> Also why should translucent elements not be interactable? As the first quote says: "first non-transparent element". As I read this it includes any value > 0.0.

The definition is anything that is not 1.0, which is made clear from your first quote.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> The second quote is wrong, I think.  It should say that an element is considered transparent if its opacity style property _does not have_ the computed value of "1".
> 
> > Also why should translucent elements not be interactable? As the first quote says: "first non-transparent element". As I read this it includes any value > 0.0.
> 
> The definition is anything that is not 1.0, which is made clear from your first quote.

Filed https://github.com/w3c/webdriver/issues/738 about the negation of the ‘transparent’ definition.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> It means that all attached event handlers are processed before the click() command returns?

Yes.  This is why we flush the event queue by calling `window.requestAnimationFrame`.

> Hm, this landed 4 days ago. Maybe I missed to refresh the web page. Thanks for pointing that out. Btw. how long will the issue be shown in the spec? Does it need a signoff?

Until someone patches it.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> Then it should be moved up before the chrome handling part.

Why?

> Thinking more about this we would still have a race condition here. Right now we are doing the click before calling this method. It means nothing guaranties that that unload event has not happened yet. As such we would miss it, and would not detect a page load.
> 
> What we would need is to register the unload handler before we actually click the element. We could do that by passing in a callback to `flushEventLoop()` and calling it inside the new promise. That way we are flexible and could extend the coverage later for other actions too.
> 
> Beside that we should make sure to pass the reject along, so that the calling code knows when an unload event has been fired, and can react accordingly.

> Right now we are doing the click before calling this method. It means nothing guaranties that that unload event has not happened yet. As such we would miss it, and would not detect a page load.

This is theoretically true.  I say that because it works in practice.

What we could do is to register the event handlers when calling `interaction.flushEventLoop` and then resolve the promise when yielding it.  We follow this pattern in a few other places.  I will make the change.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> > Right now we are doing the click before calling this method. It means nothing guaranties that that unload event has not happened yet. As such we would miss it, and would not detect a page load.
> 
> This is theoretically true.  I say that because it works in practice.
> 
> What we could do is to register the event handlers when calling `interaction.flushEventLoop` and then resolve the promise when yielding it.  We follow this pattern in a few other places.  I will make the change.

I’m redacting my comment above.  Thinking more carefully about this, the unload event is fine to add inside `flushEventLoop` since we only need it to break out of the promise, once it starts, if `win.requestAnimationFrame` does not resolve.  We don’t actually need to make the click depending on an unload event happening.  This is just an escape route in case an unload event occurs whilst we are waiting for `requestAnimationFrame`.
Attachment #8833447 - Flags: review?(mjzffr)
Attachment #8833443 - Flags: review+
Attachment #8833444 - Flags: review?(hskupin)
Attachment #8833445 - Flags: review+
Attachment #8833446 - Flags: review+
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

I found some substantive and structural issues with these patches.  Unsetting the r?s for now.  Sorry for the noise.
Attachment #8833447 - Flags: review?(hskupin)
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

> When an error is thrown in the content frame script we serialise it to JSON before passing it via IPC message to chrome.  We then deserialise it by constructing a new error instance and assigning the relevant properties.  To be able to construct the blank error instance, `obscuredEl` and `coords` need to be optional since the deserialisation only assigns the properties `message` and `stack`.

Hm, sounds suboptimal. It would be better to know the type of exception before-hand and being able to call the constructor of that exact exception class.

> The intention is to sugar `obscuredEl` and `overlayingEl` so it is easier to identify the elements in the error message.  We don’t need to include type information for the coordinates.

Ah, I see now. Thanks.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> Filed https://github.com/w3c/webdriver/issues/738 about the negation of the ‘transparent’ definition.

> The definition is anything that is not 1.0, which is made clear from your first quote.

And I think this is wrong in the spec. You can clearly interact with elements which are transparent. Even when they have `opacity: 0;`. To really make an element non-interactable you have to use eg. `display: none`.

> Yes.  This is why we flush the event queue by calling `window.requestAnimationFrame`.

Is this somewhere described? What I get is that we simply request an animation frame which will be inserted as last item into to the queue. And as such once processed all other events as popped out before have been processed. What happens if other code eg. in Firefox also added such a request but before ours? Do we wait for the one we requested, or do we return early?

> Why?

This is just about the comment which also applies to the chrome part some lines above where the selectedness gets changes.

> I’m redacting my comment above.  Thinking more carefully about this, the unload event is fine to add inside `flushEventLoop` since we only need it to break out of the promise, once it starts, if `win.requestAnimationFrame` does not resolve.  We don’t actually need to make the click depending on an unload event happening.  This is just an escape route in case an unload event occurs whilst we are waiting for `requestAnimationFrame`.

I didn't say that we should move it out of the above proposed promise code. But we still would have to register it before we are issuing the click (or any other command which could trigger an unload event). As such the flushEventLoop method would need a callback which contains the triggering of the action. Maybe it would also have to be renamed.

Something like:

```
interaction.flushEventLoop = function* (win, trigger_callback) {
  let unloadEv;
  return new Promise((resolve, reject) => {
    unloadEv = reject;
    win.addEventListener("unload", unloadEv, {once: true});
    trigger_callback();
    win.requestAnimationFrame(resolve);
  }).then(() => {
    win.removeEventListener("unload", unloadEv);
  });
};
```

I don't see another way to not have race conditions, and making it so that other commands can also make use of it.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> > The definition is anything that is not 1.0, which is made clear from your first quote.
> 
> And I think this is wrong in the spec. You can clearly interact with elements which are transparent. Even when they have `opacity: 0;`. To really make an element non-interactable you have to use eg. `display: none`.

Right, but this is what the WebDriver WG has decided upon.

> Is this somewhere described? What I get is that we simply request an animation frame which will be inserted as last item into to the queue. And as such once processed all other events as popped out before have been processed. What happens if other code eg. in Firefox also added such a request but before ours? Do we wait for the one we requested, or do we return early?

This is why we give `requestAnimationFrame` a callback, which isn’t called until that event is processed.

> I didn't say that we should move it out of the above proposed promise code. But we still would have to register it before we are issuing the click (or any other command which could trigger an unload event). As such the flushEventLoop method would need a callback which contains the triggering of the action. Maybe it would also have to be renamed.
> 
> Something like:
> 
> ```
> interaction.flushEventLoop = function* (win, trigger_callback) {
>   let unloadEv;
>   return new Promise((resolve, reject) => {
>     unloadEv = reject;
>     win.addEventListener("unload", unloadEv, {once: true});
>     trigger_callback();
>     win.requestAnimationFrame(resolve);
>   }).then(() => {
>     win.removeEventListener("unload", unloadEv);
>   });
> };
> ```
> 
> I don't see another way to not have race conditions, and making it so that other commands can also make use of it.

We don’t need to register before the click, because we only need the unload event when we’re inside the promise as an escape hatch.  There is no normative requirement to wait for an unload event.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> Right, but this is what the WebDriver WG has decided upon.

As we decided and meanwhile has been implemented... the extra check for opacity has been gone from the spec.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review111004

> Hm, sounds suboptimal. It would be better to know the type of exception before-hand and being able to call the constructor of that exact exception class.

I also forgot to mention that we currently construct every error type on loading in testing/marionette/error.js to populate the status code lookup table.  I have another patch in progress that addresses this by using a static lookup table.

I don’t think it’s suboptimal to let error types be constructed without the correct arguments with the knowledge that their data properties are correct.  Again, I have a concurrent patch which is up for review that implements `toJSON` and `fromJSON` methods on all error types to make this operation safer, but at some point you will have to construct the types, and since we cannot deduce the input arguments of each individual, unique type, assigning their data properties is the only option.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review111010

> As we decided and meanwhile has been implemented... the extra check for opacity has been gone from the spec.

I have updated the patch to reflect that transparency has been removed from the specification.
Attachment #8833446 - Flags: review?(hskupin)
Attachment #8833444 - Flags: review?(hskupin)
Attachment #8833443 - Flags: review?(mjzffr)
Comment on attachment 8833443 [details]
Bug 1333014 - Pretty-print HTML elements;

https://reviewboard.mozilla.org/r/109674/#review113706

::: testing/marionette/error.js:88
(Diff revisions 1 - 4)
>  
>  /**
> + * Wraps an Error prototype in a WebDriverError.  If the given error is
> + * already a WebDriverError, this is effectively a no-op.
> + */
> +error.wrap = function (err) {

error.wrap is defined twice. I know it the duplication was fixed elsewhere, so maybe you just need to rebase?

::: testing/marionette/error.js:182
(Diff revisions 1 - 4)
> -      if (s && s.length > 0) {
> -        res.push(" ");
> -        res.push(s);
> +      if (val && val.nodeType === 1) {
> +        s = prettyElement(val);
> +      } else if (typ == "[object Object]") {
> +        s = prettyObject(val);
> +      } else {
> +        s = typ;

So this pretty prints the value 2 as [object Number]. I think just '2' would be more helpful. Call JSON.stringify in the else clause, perhaps.

::: testing/marionette/test_error.js:90
(Diff revision 4)
> +    id: "foo",
> +    classList: {length: 1},
> +    className: "bar baz",
> +  };
> +  equal('<input id="foo" class="bar baz">', error.pprint`${el}`);
> +

Add test for pretty printing primitive type.
Attachment #8833443 - Flags: review?(mjzffr) → review-
Comment on attachment 8833445 [details]
Bug 1333014 - Remove unused element.isInteractable API;

https://reviewboard.mozilla.org/r/109678/#review113744
Attachment #8833445 - Flags: review?(mjzffr) → review+
Comment on attachment 8836769 [details]
Bug 1333014 - Change element.isDisconnected to take container;

https://reviewboard.mozilla.org/r/112100/#review113748

::: testing/marionette/element.js
(Diff revision 2)
> -    let wrappedFrame = new XPCNativeWrapper(container.frame);
> -    let wrappedShadowRoot;
> -    if (container.shadowRoot) {
> -      wrappedShadowRoot = new XPCNativeWrapper(container.shadowRoot);
> -    }
> -
> -    let wrappedEl = new XPCNativeWrapper(el);

I'm not familiar with this bit: why stop using XPCNativeWrapper?
Attachment #8836769 - Flags: review?(mjzffr) → review+
Comment on attachment 8836770 [details]
Bug 1333014 - Lint testing/marionette/element.js;

https://reviewboard.mozilla.org/r/112102/#review113752
Attachment #8836770 - Flags: review?(mjzffr) → review+
Depends on: 1336214
Comment on attachment 8833443 [details]
Bug 1333014 - Pretty-print HTML elements;

https://reviewboard.mozilla.org/r/109674/#review113706

> error.wrap is defined twice. I know it the duplication was fixed elsewhere, so maybe you just need to rebase?

Yes, this will be fixed on rebase after https://bugzilla.mozilla.org/show_bug.cgi?id=1336214 lands on central.  Marking the bug as dependent.

> So this pretty prints the value 2 as [object Number]. I think just '2' would be more helpful. Call JSON.stringify in the else clause, perhaps.

This will print "[object Number] 2", which is what we do already (see examples in testing/marionette/test_assert.js).  You would prefer primitives (null, undefined, number, boolean, string) to not contain type information?

> Add test for pretty printing primitive type.

Thanks, this actually helped me find some bugs.  I also added a few more tests.
Comment on attachment 8836769 [details]
Bug 1333014 - Change element.isDisconnected to take container;

https://reviewboard.mozilla.org/r/112100/#review113748

> I'm not familiar with this bit: why stop using XPCNativeWrapper?

XPCNativeWrapper was needed on B2G because we were using elements from content in chrome and it was needed to make the calls safe.  But after some consideration I think I will drop this patch, although we should really be wrapping elements in XPCNativeWrapper much earlier than every time we do comparisons.
Comment on attachment 8833443 [details]
Bug 1333014 - Pretty-print HTML elements;

https://reviewboard.mozilla.org/r/109674/#review113706

> This will print "[object Number] 2", which is what we do already (see examples in testing/marionette/test_assert.js).  You would prefer primitives (null, undefined, number, boolean, string) to not contain type information?

Nope, "[object Number] 2" is good. My gripe was with *just* printing "[object Number]".
Comment on attachment 8836769 [details]
Bug 1333014 - Change element.isDisconnected to take container;

https://reviewboard.mozilla.org/r/112100/#review113748

> XPCNativeWrapper was needed on B2G because we were using elements from content in chrome and it was needed to make the calls safe.  But after some consideration I think I will drop this patch, although we should really be wrapping elements in XPCNativeWrapper much earlier than every time we do comparisons.

Hm, does it mean that we are still trying to access content elements from chrome for Firefox? We should really stop that and make a call to the frame script, which should give us the correct reply.
Comment on attachment 8836769 [details]
Bug 1333014 - Change element.isDisconnected to take container;

https://reviewboard.mozilla.org/r/112100/#review113748

> Hm, does it mean that we are still trying to access content elements from chrome for Firefox? We should really stop that and make a call to the frame script, which should give us the correct reply.

Not that I’m aware of.  We have enabled unsafe CPOW checks in Marionette, which should crash Firefox if we attempt to do so.

The Firefox UI tests broke when I removed these, so I suspect `XPCNativeWrapper` also has some bearing for these.  I didn’t investigate.

In any case, I have reverted the removal of `XPCNativeWrapper` in the latest revision.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review114136

Please make sure to solve any possible merge conflicts from bug 1336214. Also I do not see that you update the usage of ElementNotVisibleError in interaction.js. You want to fix that here. Also what's up with atom.js? Would that also need an update?
Attachment #8833444 - Flags: review?(hskupin) → review-
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

::: testing/marionette/element.js:250
(Diff revisions 1 - 5)
>    } else {
>      searchFn = findElement.bind(this);
>    }
>  
>    return new Promise((resolve, reject) => {
> -    let findElements = implicitlyWaitFor(
> +    let findElements = wait.until((resolve, reject) => {

Great usage of wait.until() and that we can get rid of the implicitlyWaitFor() method.

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:228
(Diff revisions 1 - 5)
>      def test_input_file(self):
>          self.marionette.navigate(inline("<input type=file>"))
>          with self.assertRaises(errors.InvalidArgumentException):
>              self.marionette.find_element(By.TAG_NAME, "input").click()
>  
> -    def test_obscured_element(self):
> +    def test_container_element(self):

I miss a test when we have to move the container into view.

::: testing/marionette/interaction.js:160
(Diff revisions 1 - 5)
>      throw new ElementClickInterceptedError(containerEl, clickPoint);
>    }
>  
>    // step 8
>    if (!atom.isElementEnabled(el)) {
>      throw new InvalidElementStateError("Element is not enabled");

The spec is not clear about the correct error to throw yet, so we might have to revisit later once it has been defined.

::: testing/marionette/interaction.js:166
(Diff revisions 1 - 5)
>    }
>  
>    yield a11y.getAccessible(el, true).then(acc => {
> -    a11y.assertVisible(acc, el, inView);
> +    a11y.assertVisible(acc, el, true);
>      a11y.assertEnabled(acc, el, true);
>      a11y.assertActionable(acc, el);

Does it require accessibility to be turned on all the time. Don't we have cases when this is not the case?

::: testing/marionette/interaction.js:179
(Diff revisions 1 - 5)
>        el.click();
>      }
>  
>    // content elements
>    } else {
>      // step 9

Again, please move this comment up before line 169.

::: testing/marionette/interaction.js:195
(Diff revisions 1 - 5)
>    // step 11
>    // TODO(ato): if the click causes navigation,
>    // run post-navigation checks
> +}
> +
> +function* seleniumClickElement (el, a11y) {

Should we move this out to a legacy module?

I don't know this code, so maybe you get David to review that.

::: testing/marionette/interaction.js:249
(Diff revisions 1 - 5)
>   *
>   * @return {Object.<string, number>}
>   *     X- and Y-position.
>   */
>  interaction.calculateCentreCoords = function (el) {
> -  let rects = el.getClientRects();
> +

Hm, maybe the interdiff is shown wrongly to me but it looks to be an empty method now. Removing it?

::: testing/marionette/interaction.js:318
(Diff revisions 1 - 5)
> -    win.addEventListener("unload", reject);
> +    unloadEv = reject;
> +    win.addEventListener("unload", unloadEv, {once: true});
>      win.requestAnimationFrame(resolve);
> +  }).then(() => {
> +    win.removeEventListener("unload", unloadEv);
>    });

Just to raise my concern again from another issue you have already closed. 

The click or whatever we do on an element has already happened BEFORE you call flushEventLoop(). As such a possible unload event could have already happened.

Looking forward to step 11 which we haven't implemented yet, it would be of great benefit to also catch this event. Otherwise it wouldn't be clear if an ongoing page load event is happening.

But maybe we can do that on a follow-up bug. I just wanted to express it again, so you can understand my point.
Attachment #8833447 - Flags: review?(hskupin) → review-
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

> I miss a test when we have to move the container into view.

I’ve added a test, `test_container_element_outside_view`, which tries to click an `<option>` element in a `<select>` element that is outside the viewport so that the container has to be scrolled into view.

> The spec is not clear about the correct error to throw yet, so we might have to revisit later once it has been defined.

Indeed, but this is the correct behaviour; it just hasn’t made it into the specification yet.

> Does it require accessibility to be turned on all the time. Don't we have cases when this is not the case?

`a11y.getAccessible` will not run this branch if `strict` is false.  Unless you start the session with the `moz:accessibilityChecks` capability set to true, these assertions will not run.

> Again, please move this comment up before line 169.

That would be incorrect, as step 9 in the specification does not apply to XUL elements.

> Should we move this out to a legacy module?
> 
> I don't know this code, so maybe you get David to review that.

If you compare this line-by-line with the old `interaction.clickElement` implementation, this hasn’t functionally changed at all.

I thought it would provide clearer code, despite some duplication, to have one function with the old behaviour and one with the new when you use the spec-conforming element interaction branch.  If you look at `interaction.clickElement`, it takes a `specCompat` argument, which if false, will call `seleniumClickElement`.

In my previous iteration of this patch, `seleniumClickElement` and `webdriverClickElement` were one function and used a series of if-conditions to determine if the user wanted the spec conforming behaviour or not.  I dare say that the refactored version with two functions makes it easier to read.

But I will request review of this patch from David if you think it is needed.  Again, as requested, I will not resolve the issue.

I don’t think this change necessitates a new JS module in Marionette since the non-conforming element click implementation is related to interaction, but I will let David be the judge of that.

> Hm, maybe the interdiff is shown wrongly to me but it looks to be an empty method now. Removing it?

The intention was to remove `interaction.calculateCentreCoords` entirely in favour of `element.getInViewCentrePoint`.  I’ve updated the patch to reflect that.

> Just to raise my concern again from another issue you have already closed. 
> 
> The click or whatever we do on an element has already happened BEFORE you call flushEventLoop(). As such a possible unload event could have already happened.
> 
> Looking forward to step 11 which we haven't implemented yet, it would be of great benefit to also catch this event. Otherwise it wouldn't be clear if an ongoing page load event is happening.
> 
> But maybe we can do that on a follow-up bug. I just wanted to express it again, so you can understand my point.

It is correct that an unload event may have been fired before going into this promise, but if an unload event _does_ fire while we are inside this promise we need to escape it in order not to hang.  As I also said before, listening for the unload event is in itself not the goal here: but _should_ it fire we need an escape route.

> Looking forward to step 11 which we haven't implemented yet, it would be of great benefit to also catch this event. Otherwise it wouldn't be clear if an ongoing page load event is happening.

You are correct that waiting for the page to finish loading would presumably need to listen for some document unload event or web progress update before returning.  It is also true that _those_ events need to be registered before clicking the element.  But that is an entirely different scenario than ensuring `interaction.flushEventLoop` always returns.

Since a repurposable page load algorithm has not landed on central yet, I don’t see how I can anticipate this change for step 11.
Attachment #8833443 - Attachment is obsolete: true
Attachment #8833445 - Attachment is obsolete: true
Attachment #8836769 - Attachment is obsolete: true
Attachment #8836770 - Attachment is obsolete: true
Comment on attachment 8838579 [details]
Bug 1333014 - Remove unused element.isInteractable API;

https://reviewboard.mozilla.org/r/113446/#review114930
Attachment #8838579 - Flags: review?(mjzffr) → review+
Comment on attachment 8838580 [details]
Bug 1333014 - Change element.isDisconnected to take container;

https://reviewboard.mozilla.org/r/113448/#review114932
Attachment #8838580 - Flags: review?(mjzffr) → review+
Comment on attachment 8838581 [details]
Bug 1333014 - Lint testing/marionette/element.js;

https://reviewboard.mozilla.org/r/113450/#review114934
Attachment #8838581 - Flags: review?(mjzffr) → review+
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114926

::: testing/marionette/interaction.js:106
(Diff revision 8)
>   * @param {boolean=} strict
>   *     Enforce strict accessibility tests.
>   * @param {boolean=} specCompat
>   *     Use WebDriver specification compatible interactability definition.
>   *
> - * @throws {ElementNotVisibleError}
> + * @throws {ElementNotInteractableError}

`ElementClickInterceptedError` is not documented
Attachment #8833447 - Flags: review?(dburns) → review+
David, would you please provide your feedback on https://reviewboard.mozilla.org/r/109682/#comment148080?
Flags: needinfo?(dburns)
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114926

> `ElementClickInterceptedError` is not documented

I have added a docstring for that error to this method, and also updated the first patch to include a docstring for the `ElementClickInterceptedError` itself since it can receive custom arguments that are not familiar to other descendants of `WebDriverError`.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

> If you compare this line-by-line with the old `interaction.clickElement` implementation, this hasn’t functionally changed at all.
> 
> I thought it would provide clearer code, despite some duplication, to have one function with the old behaviour and one with the new when you use the spec-conforming element interaction branch.  If you look at `interaction.clickElement`, it takes a `specCompat` argument, which if false, will call `seleniumClickElement`.
> 
> In my previous iteration of this patch, `seleniumClickElement` and `webdriverClickElement` were one function and used a series of if-conditions to determine if the user wanted the spec conforming behaviour or not.  I dare say that the refactored version with two functions makes it easier to read.
> 
> But I will request review of this patch from David if you think it is needed.  Again, as requested, I will not resolve the issue.
> 
> I don’t think this change necessitates a new JS module in Marionette since the non-conforming element click implementation is related to interaction, but I will let David be the judge of that.

I don't think this warrants a separate module, at least not for this patch series. If we want to move other things out we can do that under another bug when there is more than 1 method to go out.
Flags: needinfo?(dburns)
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

> That would be incorrect, as step 9 in the specification does not apply to XUL elements.

But it's about selecting the element. It's not covered by the spec for sure but why should it make it harder for us to read our code?

Btw as it looks like the spec has been drastically changed here. Are you going to include the recent changes, which will also make it step 8 - and needs changes for the other steps.

> I don't think this warrants a separate module, at least not for this patch series. If we want to move other things out we can do that under another bug when there is more than 1 method to go out.

Sounds fine. So lets discard this issue.

> It is correct that an unload event may have been fired before going into this promise, but if an unload event _does_ fire while we are inside this promise we need to escape it in order not to hang.  As I also said before, listening for the unload event is in itself not the goal here: but _should_ it fire we need an escape route.
> 
> > Looking forward to step 11 which we haven't implemented yet, it would be of great benefit to also catch this event. Otherwise it wouldn't be clear if an ongoing page load event is happening.
> 
> You are correct that waiting for the page to finish loading would presumably need to listen for some document unload event or web progress update before returning.  It is also true that _those_ events need to be registered before clicking the element.  But that is an entirely different scenario than ensuring `interaction.flushEventLoop` always returns.
> 
> Since a repurposable page load algorithm has not landed on central yet, I don’t see how I can anticipate this change for step 11.

Ok, so lets defer this changes and get them implemented as part of bug 1335778.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review117140

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:239
(Diff revisions 5 - 10)
>          option.click()
> +        self.assertTrue(option.get_property("selected"))
> +
> +    def test_container_element_outside_view(self):
> +        self.marionette.navigate(inline("""
> +            <select style="margin-top: 100vh">

Can we assert that the container is really outside the viewport? Isn't 100vh just the maximum value for the visible area? Would it make sense to use a value like 110vh?
Attachment #8833447 - Flags: review?(hskupin)
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review117150

Hm, not sure what happened with a comment I gave earlier for this commit. But I still miss the changes in interaction.js which is still using ElementNotVisibleError. Also what about atom.js which is also using ElementNotVisibleError?
Attachment #8833444 - Flags: review?(hskupin) → review-
Comment on attachment 8833446 [details]
Bug 1333014 - Introduce new exceptions to Python client;

Hm, not sure why mozreview didn't update the attachment's review status. Doing it manually now.
Attachment #8833446 - Flags: review?(hskupin) → review+
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review117150

> But I still miss the changes in interaction.js which is still using ElementNotVisibleError.

Where does testing/marionette/interaction.js use `ElementNotVisibleError`?  I think I have moved all uses of it to `ElementNotInteractableError`.

> Also what about atom.js which is also using ElementNotVisibleError?

It is true that testing/marionette/atom.js uses an `ElementNotVisibleError` type internally, but it is not the type from testing/marionette/error.js.  The atom is also only used for the element enabled check as part of `interaction.clickElement`.

Because the internal testing/marionette/atom.js error types use number-based status codes (as Selenium does), they will will not propagate properly to the local end.  In fact, any error currently raised from inside this module will be return as an `WebDriverError` because Marionette is unable to determine what type it is.

There are several open bugs on removing the dependency on testing/marionette/atom.js.  These patches implements spec-conforming element click behaviour, which moves us in that direction.
Attachment #8833444 - Flags: review- → review?(hskupin)
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

> But it's about selecting the element. It's not covered by the spec for sure but why should it make it harder for us to read our code?
> 
> Btw as it looks like the spec has been drastically changed here. Are you going to include the recent changes, which will also make it step 8 - and needs changes for the other steps.

> But it's about selecting the element. It's not covered by the spec for sure but why should it make it harder for us to read our code?

I don’t quite follow: How does it make it harder to read our code?  The comment “step 9” is associated with the lines of code needed to produce the expected outcome of the specification.  Moving it to :169, it would arguably not be.

> Btw as it looks like the spec has been drastically changed here. Are you going to include the recent changes, which will also make it step 8 - and needs changes for the other steps.

The spec hasn’t really changed in any noteworthy capacity.  Some of the steps have been folded together by abstraction: the ‘scroll into view’ definition for example takes into account not to scroll into view if the element is already in view, and the event order produced when clicking `<option>` has been filled out.

I’ve gone through the patch and made sure each step comment is aligned with the spec, however.  That’s quite important.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114138

> > But it's about selecting the element. It's not covered by the spec for sure but why should it make it harder for us to read our code?
> 
> I don’t quite follow: How does it make it harder to read our code?  The comment “step 9” is associated with the lines of code needed to produce the expected outcome of the specification.  Moving it to :169, it would arguably not be.
> 
> > Btw as it looks like the spec has been drastically changed here. Are you going to include the recent changes, which will also make it step 8 - and needs changes for the other steps.
> 
> The spec hasn’t really changed in any noteworthy capacity.  Some of the steps have been folded together by abstraction: the ‘scroll into view’ definition for example takes into account not to scroll into view if the element is already in view, and the event order produced when clicking `<option>` has been filled out.
> 
> I’ve gone through the patch and made sure each step comment is aligned with the spec, however.  That’s quite important.

I think I’ve taken care of both these concerns now.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review117140

> Can we assert that the container is really outside the viewport? Isn't 100vh just the maximum value for the visible area? Would it make sense to use a value like 110vh?

100vh means ‘100% of the viewport’, so `<select style="margin-top: 100vh">` is definitely out of the viewport.  See https://sny.no/e/100vh for an example.

I don’t think it’s the place of Mn to test that CSS is rendered correctly.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review117150

> Where does testing/marionette/interaction.js use ElementNotVisibleError?  I think I have moved all uses of it to ElementNotInteractableError.

This commit doesn't contain any changes to interaction.js. 3 updates are necessary:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/interaction.js#89,106,133

Thank you for clarifying the question about atom.js.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review117140

> 100vh means ‘100% of the viewport’, so `<select style="margin-top: 100vh">` is definitely out of the viewport.  See https://sny.no/e/100vh for an example.
> 
> I don’t think it’s the place of Mn to test that CSS is rendered correctly.

I see. In this case it's fine then. Thanks.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review117588
Attachment #8833447 - Flags: review?(hskupin) → review+
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

Looks like mozreview missed again to move the flag on Bugzilla.

Please address the comments before requesting review again. Thanks.
Attachment #8833444 - Flags: review?(hskupin)
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review121052
Attachment #8833447 - Flags: review?(mjzffr)
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review117150

This was actually fixed in a later commit, but you’re right it makes sense to change it here.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114926

> I have added a docstring for that error to this method, and also updated the first patch to include a docstring for the `ElementClickInterceptedError` itself since it can receive custom arguments that are not familiar to other descendants of `WebDriverError`.

whimboo: It would be good if you could confirm if you are satisfied with the docstring so I can land this patch.  Not resolving any issues per your request, and this was fixed over three weeks ago.
I suspect that these patches causes some test failures on the Firefox UI test jobs.  Triggered a new try job and will investigate once it finishes.
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114926

> whimboo: It would be good if you could confirm if you are satisfied with the docstring so I can land this patch.  Not resolving any issues per your request, and this was fixed over three weeks ago.

This is not mine issue. It's from David. So I cannot close it.
Comment on attachment 8833444 [details]
Bug 1333014 - Align element interaction errors with spec;

https://reviewboard.mozilla.org/r/109676/#review122046
Attachment #8833444 - Flags: review?(hskupin) → review+
Comment on attachment 8833447 [details]
Bug 1333014 - Support intercepted clicks and align with spec;

https://reviewboard.mozilla.org/r/109682/#review114926

> This is not mine issue. It's from David. So I cannot close it.

Sorry, my mistake.  I have closed it.
What are we waiting on for this bug?
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #137)
> I suspect that these patches causes some test failures on the Firefox UI
> test jobs.  Triggered a new try job and will investigate once it finishes.

It looks like that was not the case in the last try run:

	https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e993425bda2&group_state=expanded

I will go ahead and push it to inbound.
Flags: needinfo?(ato)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1fb6e323c00
Align element interaction errors with spec; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/3034d2532b88
Introduce new exceptions to Python client; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/49173c7e06f4
Support intercepted clicks and align with spec; r=automatedtester,whimboo
https://hg.mozilla.org/integration/autoland/rev/40569184236e
Pretty-print HTML elements; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/99a7a0a0b08b
Remove unused element.isInteractable API; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/74c2ae42b2c0
Change element.isDisconnected to take container; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/08b46c736a62
Lint testing/marionette/element.js; r=maja_zf
Sheriffs: Please uplift to Aurora and Beta as test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
I think nothing speaks against an uplift to esr52 here. The reason why I request this now is that it will block uplifts of other important patches, or at least cause lots of merge conflict work.

So can we please uplift this test-only patch to esr52? Please note that bug 1150527 needs to be uplifted first.
Whiteboard: [checkin-needed-esr52][uplift patch on bug 1150527 first]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: