Rename raisesAccessibilityExceptions capability

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Version 3
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
The raiseAccessibilityCapability in Marionette is not conforming to the extension capability naming scheme laid out by the WebDriver specification.
Assignee

Updated

3 years ago
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Summary: Rename raiseAccessibilityExceptions capability → Rename raisesAccessibilityExceptions capability
Comment hidden (mozreview-request)

Comment 2

3 years ago
mozreview-review
Comment on attachment 8820006 [details]
Bug 1324529 - Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks;

https://reviewboard.mozilla.org/r/99556/#review99992

looks good to me.
Attachment #8820006 - Flags: review?(yzenevich) → review+

Comment 3

3 years ago
mozreview-review
Comment on attachment 8820006 [details]
Bug 1324529 - Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks;

https://reviewboard.mozilla.org/r/99556/#review100002

::: testing/marionette/driver.js:187
(Diff revision 1)
>  
>    this.actions = new action.Chain();
>  };
>  
> +Object.defineProperty(GeckoDriver.prototype, "a11yChecks", {
> +  get: function () { return this.sessionCapabilities["moz:accessibilityChecks"]; }

Can we use `() => this.sessionCapabilities["moz:accessibilityChecks"];` here? Or isn't that not supported at this level?

::: testing/marionette/listener.js:638
(Diff revision 1)
>    let visible = element.isVisible(el, corx, cory);
>    if (!visible) {
>      throw new ElementNotVisibleError("Element is not currently visible and may not be manipulated");
>    }
>  
> -  let a11y = accessibility.get(capabilities.raisesAccessibilityExceptions);
> +  let a11y = accessibility.get(capabilities["moz:accessibilityChecks"]);

Is this a variable we could store globally in that file so that we don't have to access the capabilities each time? Keep in mind that other capability access code doesn't use get() and could fail if it is not present.
Assignee

Comment 4

3 years ago
mozreview-review-reply
Comment on attachment 8820006 [details]
Bug 1324529 - Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks;

https://reviewboard.mozilla.org/r/99556/#review100002

> Can we use `() => this.sessionCapabilities["moz:accessibilityChecks"];` here? Or isn't that not supported at this level?

We can’t use that here because arrow functions doesn’t bind `this`.  At some point I will convert `GeckoDriver` to be an ES6 class, however, and we can remove this monstrosity.

> Is this a variable we could store globally in that file so that we don't have to access the capabilities each time? Keep in mind that other capability access code doesn't use get() and could fail if it is not present.

Well, it will return `undefined` if the value is missing from the dictionary.  When I get around to converting listener.js to an ES6 class, we can follow the same pattern as we are in driver.js.

I don’t want to store it in a global variable because we might forget cleaning it up when the capabilities change.

Comment 5

3 years ago
mozreview-review-reply
Comment on attachment 8820006 [details]
Bug 1324529 - Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks;

https://reviewboard.mozilla.org/r/99556/#review100002

> Well, it will return `undefined` if the value is missing from the dictionary.  When I get around to converting listener.js to an ES6 class, we can follow the same pattern as we are in driver.js.
> 
> I don’t want to store it in a global variable because we might forget cleaning it up when the capabilities change.

I see. Lets go with the current code then. Thanks.

Comment 6

3 years ago
mozreview-review
Comment on attachment 8820006 [details]
Bug 1324529 - Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks;

https://reviewboard.mozilla.org/r/99556/#review100206
Attachment #8820006 - Flags: review?(hskupin) → review+

Comment 7

3 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce04296e8331
Rename Marionette capability raisesAccessibilityExceptions to moz:accessibilityChecks; r=whimboo,yzen

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce04296e8331
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Comment 9

3 years ago
Sheriffs: Can you please do a test-only uplift of this?
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]

Comment 10

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4543f401f05f
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
seems this have problems on beta 

Andreas could you take a look,

merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
Assignee

Comment 12

3 years ago
I’m going to cancel this uplift as it requires a long list of other uplifts and modifications to apply.
Flags: needinfo?(ato)
You need to log in before you can comment on or make changes to this bug.