[AccessFu] Add states to Constants module

RESOLVED FIXED in mozilla29

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8364665 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15573

In bug #937466 we introduced the Constants module. We should extend it to do states as well.
(Assignee)

Comment 1

4 years ago
Created attachment 8364677 [details] [diff] [review]
Add states to Constants and unify states and extended states.
Attachment #8364677 - Flags: review?(yzenevich)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8364665 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15573

I don't know how this got here..
Attachment #8364665 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Comment on attachment 8364677 [details] [diff] [review]
Add states to Constants and unify states and extended states.

Apparently js does not support 64 bit integers :P you learn something new every day.
Attachment #8364677 - Attachment is obsolete: true
Attachment #8364677 - Flags: review?(yzenevich)
I wonder if keeping extended states separately will be less confusing ?
(Assignee)

Comment 5

4 years ago
Created attachment 8365355 [details] [diff] [review]
Add states to Constants and unify states and extended states.

This should work better..
Attachment #8365355 - Flags: review?(yzenevich)
Comment on attachment 8365355 [details] [diff] [review]
Add states to Constants and unify states and extended states.

Review of attachment 8365355 [details] [diff] [review]:
-----------------------------------------------------------------

Looks so much nicer :) . r=me with comments addressed.

::: accessible/src/jsat/Constants.jsm
@@ +9,4 @@
>    let offset = aPrefix.length;
>    for (var name in aObject) {
>      if (name.indexOf(aPrefix) === 0) {
> +      (aMap || this)[name.slice(offset)] = aModifier ?

Lets add |extend| method instead of using aMap || this.

@@ +43,5 @@
> +  this, 'States',
> +  function() {
> +    let statesMap = new ConstantsMap(Ci.nsIAccessibleStates, 'STATE_', null,
> +                                     (val) => { return { base: val, extended: 0 }; });
> +    ConstantsMap(Ci.nsIAccessibleStates, 'EXT_STATE_', statesMap,

This is really scary to me :) - the constructor without the |new|, i would suggest we use the |extend| method proposed above.

::: accessible/src/jsat/EventManager.jsm
@@ +182,2 @@
>          let editState = {
> +          editing: !!(state.contains(States.EDITABLE)),

Here and on the next line, contains returns Boolean already.

::: accessible/src/jsat/OutputGenerator.jsm
@@ +359,5 @@
>        return this.objectOutputFunctions.defaultFunc.apply(this, arguments);
>      },
>  
> +    entry: function entry(aAccessible, aRoleStr, aState, aFlags) {
> +      let rolestr = (aState.contains(States.MULTI_LINE)) ?

Nit: unnecessary ().

@@ +622,5 @@
>      // This is because we expose the checked information on the node itself.
>      // XXX: this means the checked state is always appended to the end, regardless
>      // of the utterance ordering preference.
> +    if (Utils.AndroidSdkVersion < 16 && aState.contains(States.CHECKABLE)) {
> +      let statetr = (aState.contains(States.CHECKED)) ?

Nit: unnecessary (). and a couple lines lower.

::: accessible/src/jsat/Utils.jsm
@@ +20,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, 'Relations',
>    'resource://gre/modules/accessibility/Constants.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, 'States',
> +  'resource://gre/modules/accessibility/Constants.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, 'State',

Can't find |State| in the Constants.jsm, looks like a typo.

@@ +201,5 @@
> +
> +      this.toString = () => {
> +        let stateStrings = Utils.AccRetrieval.
> +          getStringStates(this.base, this.extended);
> +        return '[' + [stateStrings.item(i) for (i of Array(stateStrings.length))].join('|') + ']';

I think you can do this simpler, like this (unless you can't iterate stateStrings directly):
return '[' + [stateStrings.item(i) for ([i, ss] of stateStrings)].join('|') + ']';
Attachment #8365355 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/a16b224233d9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.