Centralize interactions to it's own file and have listener/driver import that

RESOLVED FIXED in Firefox 46

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 obsolete attachments)

(Assignee)

Description

2 years ago
In order to be able to do additional checks such as accessibility checks when performing interactions both in chrome and content contexts we should try making interaction bits common between the two and then loading them into both contexts.
(Assignee)

Comment 1

2 years ago
Created attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

---
 testing/marionette/accessibility.js | 308 ++++++++++++++++++++++++++++++++++
 testing/marionette/driver.js        |  42 ++---
 testing/marionette/elements.js      | 197 +---------------------
 testing/marionette/interactions.js  | 319 ++++++++++++++++++++++++++++++++++++
 testing/marionette/jar.mn           |   2 +
 testing/marionette/listener.js      | 178 +++-----------------
 testing/marionette/sendkeys.js      |   6 +-
 7 files changed, 669 insertions(+), 383 deletions(-)
 create mode 100644 testing/marionette/accessibility.js
 create mode 100644 testing/marionette/interactions.js

Review commit: https://reviewboard.mozilla.org/r/30957/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30957/
Attachment #8708059 - Flags: review?(dburns)
https://reviewboard.mozilla.org/r/30957/#review27847

This is good!

I am especially happy that this patch moves us in the direction of moving all the listener commands to use the new dispatching technique (synchronous sendkeys.js was a blocker).

I also very much like the design of the Interactions API and how it hides and integrates accessibility checks as a natural component of that.

::: testing/marionette/accessibility.js:27
(Diff revision 1)
> +  Object.defineProperty(this, 'accessibleRetrieval', {

It’s already obvious this prototype is about accessibility, so the “accessible” in “accessibleRetrieval” is kind of superfluous.

::: testing/marionette/accessibility.js:31
(Diff revision 1)
> +      this.accessibleRetrieval = Components.classes[
> +        '@mozilla.org/accessibleRetrieval;1'].getService(
> +          Components.interfaces.nsIAccessibleRetrieval);

Expose as Cc and Ci from Components to shorten.

::: testing/marionette/accessibility.js:60
(Diff revision 1)
> +  actionableRoles: new Set([

If this set is not supposed to grow, you could indicate that by using upper-casing for its name.

::: testing/marionette/accessibility.js:151
(Diff revision 1)
> +    let hidden;

Should default to `false`, not `undefined`?

::: testing/marionette/accessibility.js:152
(Diff revision 1)
> +    try {
> +      hidden = accessible.attributes.getStringProperty('hidden');
> +    } finally {
> +      // If the property is missing, exception will be thrown.
> +      return hidden && hidden === 'true';
> +    }

Is there a way to tell if the property is present?  Relying on throws to direct code flow should be avoided if we can.

::: testing/marionette/accessibility.js:167
(Diff revision 1)
> +    let stateToMatch = Components.interfaces.nsIAccessibleStates[stateName];

Shorten to Ci

::: testing/marionette/accessibility.js:193
(Diff revision 1)
> +  handleErrorMessage(message, element) {

Nit: Personally I would shorten this to just “error” or “handleError”.

::: testing/marionette/accessibility.js:203
(Diff revision 1)
> +    dump(Date.now() + ' Marionette ' + message);

Debug that should be removed?

::: testing/marionette/accessibility.js:242
(Diff revision 1)
> +      accessible, 'STATE_UNAVAILABLE');

Make "STATE_UNAVAILABLE" string a constant.

::: testing/marionette/accessibility.js:244
(Diff revision 1)
> +      element, null).getPropertyValue('pointer-events') !== 'none';

Is the `null` required?

::: testing/marionette/accessibility.js:292
(Diff revision 1)
> +    if (!this.matchState(accessible, 'STATE_SELECTABLE')) {
> +      // Element is not selectable via the accessibility API
> +      return;
> +    }
> +
> +    let selectedAccessibility = this.matchState(accessible, 'STATE_SELECTED');

Make "STATE_SELECTABLE", "STATE_SELECTED", "STATE_FOCUSABLE", and "STATE_UNAVAILABLE" an enum.

Something like:

```
const State = {
  Selected: 0
  Selectable: 1,
  Focusable: 2,
  Unavailable: 3,
};
```

::: testing/marionette/interactions.js:17
(Diff revision 1)
> + * @type {Array}

I think this type annotation is a bit pointless, but not raising an issue about it.

::: testing/marionette/interactions.js:19
(Diff revision 1)
> +let DISABLED_ATTRIBUTE_SUPPORTED_XUL = [

const

::: testing/marionette/interactions.js:53
(Diff revision 1)
> +let CHECKED_PROPERTY_SUPPORTED_XUL = [

const

::: testing/marionette/interactions.js:60
(Diff revision 1)
> +let SELECTED_PROPERTY_SUPPORTED_XUL = [

const

::: testing/marionette/interactions.js:78
(Diff revision 1)
> +  if (x == null) {
> +    x = box.width / 2;
> +  }
> +  if (y == null) {
> +    y = box.height / 2;
> +  }

When comparing to `null`, please use `===`.

::: testing/marionette/interactions.js:84
(Diff revision 1)
> +  let coords = {};
> +  coords.x = box.left + x;
> +  coords.y = box.top + y;

Do this in a single expression:

```
let coords = {
  x: box.left + x,
  y: box.top + y,
};
return coords;
```

::: testing/marionette/interactions.js:100
(Diff revision 1)
> +  /**
> +   * Update session capabilities.
> +   * @param {JSON} capabilities session capabilities
> +   */
> +  updateCapabilities(capabilities) {
> +    this.capabilities = capabilities;
> +    // Set accessibility strict flag.
> +    this.accessibility.strict = capabilities.raisesAccessibilityExceptions;
> +  },

It might be a good idea to give Interactions a closure that returns the current capabilities.  This avoids forgetting to update this when the capabilities change.

I could imagine this would work like this:

```
let i = new Interactions(utils, () => caps);
```

Then `this.accessibility.strict` could be a getter function that returned `this.capabilities.raisesAccessibilityExceptions`.

::: testing/marionette/interactions.js:116
(Diff revision 1)
> +   * @param Object elementManager

s/Object/ElementManager/

::: testing/marionette/interactions.js:135
(Diff revision 1)
> +          this.utils.synthesizeMouseAtCenter(el, {},
> +            el.ownerDocument.defaultView);

Just so you’re aware, this might need rebasing after bug 1239307 landed with this change: https://hg.mozilla.org/mozilla-central/rev/40da34cef572

::: testing/marionette/interactions.js:139
(Diff revision 1)
> +        throw new InvalidElementStateError('Element is not Enabled');

s/Enabled/enabled/

::: testing/marionette/interactions.js:247
(Diff revision 1)
> +      if (CHECKED_PROPERTY_SUPPORTED_XUL.indexOf(tagName) >= 0) {
> +        selected = el.checked;
> +      }
> +      if (SELECTED_PROPERTY_SUPPORTED_XUL.indexOf(tagName) >= 0) {
> +        selected = el.selected;
> +      }

I generally find `tagName in ARRAY` to be more readable, but this is fine.

::: testing/marionette/interactions.js:309
(Diff revision 1)
> +    let frame = container.frame;

Nit: Assign this to `win` to be clear that this is a browsing context, i.e. a `Window` object.

::: testing/marionette/interactions.js:310
(Diff revision 1)
> +    let viewPort = {top: frame.pageYOffset,
> +                    left: frame.pageXOffset,
> +                    bottom: (frame.pageYOffset + frame.innerHeight),
> +                    right:(frame.pageXOffset + frame.innerWidth)};

Nit: Break after { to not rely no arbitrary number of spaces for indentation:

```
let vp = {
  foo: …
  bar: …
}
```

Also, a space after "right:".
Since ato has done a lot of the review, and it's things that I agree with can you carry on the review with him.
(Assignee)

Comment 5

2 years ago
(In reply to Andreas Tolfsen (:ato) from comment #3)
> https://reviewboard.mozilla.org/r/30957/#review27847
> 
> ::: testing/marionette/accessibility.js:152
> (Diff revision 1)
> > +    try {
> > +      hidden = accessible.attributes.getStringProperty('hidden');
> > +    } finally {
> > +      // If the property is missing, exception will be thrown.
> > +      return hidden && hidden === 'true';
> > +    }
> 
> Is there a way to tell if the property is present?  Relying on throws to
> direct code flow should be avoided if we can.

Sorry in this case it is not reliable as it can fail whenever the object we are accessing becomes defunct.
(Assignee)

Comment 6

2 years ago
(In reply to Andreas Tolfsen (:ato) from comment #3)
> https://reviewboard.mozilla.org/r/30957/#review27847
> ::: testing/marionette/accessibility.js:203
> (Diff revision 1)
> > +    dump(Date.now() + ' Marionette ' + message);
> 
> Debug that should be removed?
> 
This is intentional. We still want to log accessibility issues even if we do not raise exceptions. This is helpful for looking at integration tests that are failing if the a11y exceptions are turned on. We use this information for Gij in gaia
(Assignee)

Updated

2 years ago
Attachment #8708059 - Attachment description: MozReview Request: Bug 1238744 - centralizing interactions into its own file. r?automatedtester → MozReview Request: Bug 1238744 - centralizing interactions into its own file. r?ato
Attachment #8708059 - Flags: review?(dburns) → review?(ato)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/1-2/
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

https://reviewboard.mozilla.org/r/30957/#review27875

Looks good to me now.  Please fix the two remaining issues, and consider it an r+.

::: testing/marionette/accessibility.js:28
(Diff revision 2)
> +this.Accessibility = function Accessibility(getCapabilies = () => {}) {

I had no idea you could do this.  Cool.

::: testing/marionette/accessibility.js:28
(Diff revision 2)
> +this.Accessibility = function Accessibility(getCapabilies = () => {}) {

Can I also trouble you to add some documentation to the Accessibility prototype?  Sorry for not pointing this out earlier.

::: testing/marionette/accessibility.js:215
(Diff revision 2)
> +    dump(Date.now() + ' Marionette ' + message);

Of course it’s fine to log, but could we use the Log.jsm "Marionette" instance for this?
Attachment #8708059 - Flags: review?(ato) → review+
Please also see the remaining issues in MozReview and mark them as “fixed” when you have.
(Assignee)

Comment 10

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/2-3/
Attachment #8708059 - Attachment description: MozReview Request: Bug 1238744 - centralizing interactions into its own file. r?ato → MozReview Request: Bug 1238744 - centralizing interactions into its own file. r=ato
https://reviewboard.mozilla.org/r/30957/#review27899

::: testing/marionette/accessibility.js:336
(Diff revisions 2 - 3)
> +logger.debug('loaded listener.js');

Is this supposed to be here?
(Assignee)

Comment 12

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/3-4/
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/30957/#review27847

> Is there a way to tell if the property is present?  Relying on throws to direct code flow should be avoided if we can.

Replied in bugzilla: Sorry in this case it is not reliable as it can fail whenever the object we are accessing becomes defunct.
(Assignee)

Comment 14

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/4-5/
(Assignee)

Comment 15

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/5-6/
(Assignee)

Comment 16

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/577e4acaa223
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

2 years ago
Attachment #8708059 - Attachment description: MozReview Request: Bug 1238744 - centralizing interactions into its own file. r=ato → MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester
Attachment #8708059 - Flags: review?(dburns)
(Assignee)

Comment 19

2 years ago
Comment on attachment 8708059 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30957/diff/7-8/
yzen, I think with MozReview it’s generally recommended to start a new bug when you do a new patch.
(Assignee)

Comment 21

2 years ago
https://reviewboard.mozilla.org/r/30957/#review28527

Closing, will open a separate bug
(Assignee)

Updated

2 years ago
Attachment #8708059 - Attachment is obsolete: true
Attachment #8708059 - Flags: review?(dburns)
(Assignee)

Comment 22

2 years ago
Created attachment 8710572 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=ato

---
 testing/marionette/accessibility.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Review commit: https://reviewboard.mozilla.org/r/31795/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31795/
Attachment #8710572 - Flags: review?(ato)
Attachment #8710572 - Flags: review?(ato) → review+
Comment on attachment 8710572 [details]
MozReview Request: Bug 1238744 - replacing logger error with debug when logging accessibility issues. r=ato

https://reviewboard.mozilla.org/r/31795/#review28529
(Assignee)

Updated

2 years ago
Attachment #8710572 - Attachment is obsolete: true
Depends on: 1244837
This patch caused a massive performance slow down. Please see bug 1244837.
You need to log in before you can comment on or make changes to this bug.