1245064 - Implement element interactability algorithm from WebDriver

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Now that bug 1164427 has landed, it is possible for us to implement the element interactability concept from the W3C WebDriver specification:

    http://w3c.github.io/webdriver/webdriver-spec.html#element-interactability

Although it has not yet been verified if this approach is backwards compatible with the existing algorithm, it would eventually allow us to move off the dependency of an additional Selenium atom.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

Review commit: https://reviewboard.mozilla.org/r/38247/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38247/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Implements the WebDriver pointer-interactability algorithm described in
http://w3c.github.io/webdriver/webdriver-spec.html#dfn-interactable-element.

The specification compatible behaviour is enabled only when the client
requests the capability specificationLevel >= 0.

Review commit: https://reviewboard.mozilla.org/r/38249/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38249/

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: fixup! Bug 1245064 - Implement element pointer-interactability; r?automatedtester

Review commit: https://reviewboard.mozilla.org/r/38251/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38251/

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Marionette is not yet compatible with the WebDriver specification, and
we indicate this by lowering the specificationLevel capability to 0.
This lets us "gate" specification-compatible features, such as the new
element interactability algorithm.

The new interactability algorithm can be enabled by requesting the
capability specificationLevel 1.

Review commit: https://reviewboard.mozilla.org/r/38253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38253/

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Pass all capabilities to listener; r?automatedtester

Review commit: https://reviewboard.mozilla.org/r/38255/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38255/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r?automatedtester

Review commit: https://reviewboard.mozilla.org/r/38257/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38257/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r?automatedtester

Review commit: https://reviewboard.mozilla.org/r/38259/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38259/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/1-2/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/1-2/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726838 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38255/diff/1-2/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726839 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38257/diff/1-2/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726840 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38259/diff/1-2/

Comment 13

[David Burns :automatedtester]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

https://reviewboard.mozilla.org/r/38247/#review35171

::: testing/marionette/accessibility.js:309
(Diff revision 1)
> +    let explorable = win.getComputedStyle(element)

it feels like automatic semicolon injection would make the `getPropertyValue(...)` not get called.

Comment 14

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/38247/#review35171

> it feels like automatic semicolon injection would make the `getPropertyValue(...)` not get called.

What is “automatic semicolon injection”?

Comment 15

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/38247/#review35171

> What is “automatic semicolon injection”?

in JS if you had

```
 return 
   a + b
```

The JS Compiler would change it to 

```
  return;
    a + b;
```

Because it wants to be "helpful". There are rules where it doesnt automatically insert so I am open to correction.

Comment 16

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/38247/#review35171

> in JS if you had
> 
> ```
>  return 
>    a + b
> ```
> 
> The JS Compiler would change it to 
> 
> ```
>   return;
>     a + b;
> ```
> 
> Because it wants to be "helpful". There are rules where it doesnt automatically insert so I am open to correction.

How does that affect this statement?

Comment 17

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/38247/#review35171

> How does that affect this statement?

I was wondering if 

```
let explorable = win.getComputedStyle(element)
        .getPropertyValue("pointer-events") !== "none";
```
   
results in 
```
    let explorable = win.getComputedStyle(element);
        .getPropertyValue("pointer-events") !== "none";
```

Comment 18

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/38247/#review35171

> I was wondering if 
> 
> ```
> let explorable = win.getComputedStyle(element)
>         .getPropertyValue("pointer-events") !== "none";
> ```
>    
> results in 
> ```
>     let explorable = win.getComputedStyle(element);
>         .getPropertyValue("pointer-events") !== "none";
> ```

I think it maybe be specific to the return statement, because JS is otherwise uses C-like syntax where semicolons denote the end of an expression.

```
» a = {b: 0}
← Object { b: 0 }
» c = a
»   .b;
← 0
```

Comment 19

[David Burns :automatedtester]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

https://reviewboard.mozilla.org/r/38249/#review35341

::: testing/marionette/element.js:883
(Diff revision 2)
> +  // TODO

I know in the spec we use this to return early but step 7 handles this for us already. If its not in the same document then we return `[]` as per CSSOM.

Comment 20

[David Burns :automatedtester]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

https://reviewboard.mozilla.org/r/38247/#review35345

Comment 21

[David Burns :automatedtester]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

https://reviewboard.mozilla.org/r/38253/#review35349

Comment 22

[David Burns :automatedtester]

Comment on attachment 8726838 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r?automatedtester

https://reviewboard.mozilla.org/r/38255/#review35351

Comment 23

[David Burns :automatedtester]

Comment on attachment 8726839 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r?automatedtester

https://reviewboard.mozilla.org/r/38257/#review35353

::: testing/marionette/driver.js:645
(Diff revision 2)
> -    if (Object.keys(errors).length === 0) {
> +    if (Object.keys(errors).length == 0) {

TIL: Linters prefer `==` for length conditionals

Comment 24

[David Burns :automatedtester]

Comment on attachment 8726840 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r?automatedtester

https://reviewboard.mozilla.org/r/38259/#review35359

::: testing/marionette/harness/marionette/tests/unit/test_click.py:64
(Diff revision 2)
> +        TestLegacyClick.test_clicking_an_element_that_is_not_displayed_raises(self)

Add a `pass` statement please

Comment 25

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/38249/#review35341

> I know in the spec we use this to return early but step 7 handles this for us already. If its not in the same document then we return `[]` as per CSSOM.

The more I think about this, the more I don't think we really need anything. Since we have the element we have already checked in it is in the DOM and is not stale. If it is not in the `document` then we can return []. If the element is good then carry on through the steps. This is just me putting my thoughts down more than anything else.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38247/diff/1-2/

Comment 27

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/2-3/

Comment 28

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/2-3/

Comment 29

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38247/diff/2-3/

Comment 30

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/3-4/

Comment 31

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/3-4/

Comment 32

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Pass all capabilities to listener; r=automatedtester

Review commit: https://reviewboard.mozilla.org/r/38705/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38705/

Comment 33

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r=automatedtester

Review commit: https://reviewboard.mozilla.org/r/38707/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38707/

Comment 34

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

Review commit: https://reviewboard.mozilla.org/r/38709/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38709/

Comment 35

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727953 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38709/diff/1-2/

Comment 36

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/4-5/

Comment 37

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/4-5/

Comment 38

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727950 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38705/diff/1-2/

Comment 39

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727951 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38707/diff/1-2/

Comment 40

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727953 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38709/diff/2-3/

Comment 41

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38247/diff/3-4/

Comment 42

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/5-6/

Comment 43

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/5-6/

Comment 44

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727950 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38705/diff/2-3/

Comment 45

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727951 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38707/diff/2-3/

Comment 46

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727953 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38709/diff/3-4/

Comment 47

[David Burns :automatedtester]

Comment on attachment 8727953 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

https://reviewboard.mozilla.org/r/38709/#review35549

Comment 48

[David Burns :automatedtester]

Comment on attachment 8727951 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r=automatedtester

https://reviewboard.mozilla.org/r/38707/#review35551

Comment 49

[David Burns :automatedtester]

Comment on attachment 8727950 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r=automatedtester

https://reviewboard.mozilla.org/r/38705/#review35553

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dec8d87c105
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd0bcc1c45e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb92135b84d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2483596f62ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4589c2cf47
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d915f9df91

Comment 51

[Carsten Book [:Tomcat]]

backed out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=23342503&repo=mozilla-inbound

Comment 52

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/580ae95e5206
https://hg.mozilla.org/integration/mozilla-inbound/rev/da2b75c86700
https://hg.mozilla.org/integration/mozilla-inbound/rev/73fa8e17f405
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74b387a975e
https://hg.mozilla.org/integration/mozilla-inbound/rev/785f1cb39173
https://hg.mozilla.org/integration/mozilla-inbound/rev/3529a9febf36

Comment 53

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726834 [details]
MozReview Request: Bug 1245064 - Refactor interaction- and accessibility API; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38247/diff/4-5/

Comment 54

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726835 [details]
MozReview Request: Bug 1245064 - Implement element pointer-interactability; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38249/diff/6-7/

Comment 55

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8726837 [details]
MozReview Request: Bug 1245064 - Lower specificationLevel to 0; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38253/diff/6-7/

Comment 56

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727950 [details]
MozReview Request: Bug 1245064 - Pass all capabilities to listener; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38705/diff/3-4/

Comment 57

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727951 [details]
MozReview Request: Bug 1245064 - Lint GeckoDriver#setSessionCapabilities; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38707/diff/3-4/

Comment 58

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8727953 [details]
MozReview Request: Bug 1245064 - Run click tests with new interactability algorithm; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38709/diff/4-5/

Comment 59

[Andreas Tolfsen ❲:ato❳]

Fixed failure and triggered autoland again.

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f862d5425a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a7cc13eab3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3983d5fd33f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2203b0881e7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b794dc66a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/15680b451181

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f862d5425a3
https://hg.mozilla.org/mozilla-central/rev/18a7cc13eab3
https://hg.mozilla.org/mozilla-central/rev/3983d5fd33f6
https://hg.mozilla.org/mozilla-central/rev/2203b0881e7f
https://hg.mozilla.org/mozilla-central/rev/f6b794dc66a8
https://hg.mozilla.org/mozilla-central/rev/15680b451181

Comment 62

[David Burns :automatedtester]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77141d52b884

Comment 63

[David Burns :automatedtester]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4857cfc29b00
https://hg.mozilla.org/releases/mozilla-aurora/rev/a09a88275612
https://hg.mozilla.org/releases/mozilla-aurora/rev/4598a6110c42
https://hg.mozilla.org/releases/mozilla-aurora/rev/0da3648e2523
https://hg.mozilla.org/releases/mozilla-aurora/rev/57e85125326d
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bb19c9730af