1468110 - Add attributes to AccessibleNode

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[dora.tokio]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.79 Safari/537.36

Steps to reproduce:

Bug 1462465 had AccessibleNode cached in Node.



Expected results:

Next, attributes are added to AccessibleNode.

Comment 1

[dora.tokio]

(In reply to dora.tokio from comment #0)
> Steps to reproduce:

It's a mistake.

Comment 2

[dora.tokio]

Attachment: part1: boolean attributes

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8985565 [details] [diff] [review]
part1: boolean attributes

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

Could you please add tests as well? that may do nothing as there's no connection to accessibility engine yet, but you could do something like 
is(anode.prop, undefined, 'Bla');
anode.prop = true;
is(anode.prop, true, 'Bla');
anode.prop = false;
is(anode.prop, false, 'Bla');

Also it makes sense to have a thumb-up from Olli

::: accessible/aom/AccessibleNode.h
@@ +70,5 @@
> +  // Widget states
> +  dom::Nullable<bool> GetExpanded();
> +  void SetExpanded(const dom::Nullable<bool>& aExpanded);
> +  dom::Nullable<bool> GetDisabled();
> +  void SetDisabled(const dom::Nullable<bool>& aDisabled);

all these should be inline, also you may want macroses to easily define methods:

BOOL_PROP(Hidden)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 8985565 [details] [diff] [review]
> part1: boolean attributes
> 
> Review of attachment 8985565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you please add tests as well? that may do nothing as there's no
> connection to accessibility engine yet, but you could do something like 
> is(anode.prop, undefined, 'Bla');
sure it shouldn't be undefined, but null

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985565 [details] [diff] [review]
part1: boolean attributes

> 
>+  // Widget properties
>+  dom::Nullable<bool> GetHidden();
>+  void SetHidden(const dom::Nullable<bool>& aHidden);
>+  dom::Nullable<bool> GetModal();
>+  void SetModal(const dom::Nullable<bool>& aModal);
>+  dom::Nullable<bool> GetMultiline();
>+  void SetMultiline(const dom::Nullable<bool>& aMultiline);
>+  dom::Nullable<bool> GetMultiselectable();
>+  void SetMultiselectable(const dom::Nullable<bool>& aMultiselectable);
>+  dom::Nullable<bool> GetReadOnly();
>+  void SetReadOnly(const dom::Nullable<bool>& aReadOnly);
>+  dom::Nullable<bool> GetRequired();
>+  void SetRequired(const dom::Nullable<bool>& aRequired);
>+  dom::Nullable<bool> GetSelected();
>+  void SetSelected(const dom::Nullable<bool>& aSelected);
empty line after Get/Set* methods would make this easier to read

>+  enum class AOMBooleanProperty {
{ should be on its own line.

>+    Atomic,
>+    Busy,
>+    Disabled,
>+    Expanded,
>+    Hidden,
>+    Modal,
>+    Multiline,
>+    Multiselectable,
>+    ReadOnly,
>+    Required,
>+    Selected
enum values should start with lowercase e. eAtomic etc.
(I know we aren't always consistent with that)



>+  // Widget properties
>+  attribute boolean? hidden;
>+  attribute boolean? modal;
>+  attribute boolean? multiline;
>+  attribute boolean? multiselectable;
>+  attribute boolean? readOnly;
>+  attribute boolean? required;
>+  attribute boolean? selected;
>+
>+  // Widget states
>+  attribute boolean? expanded;
>+  attribute boolean? disabled;
>+
>+  // Live regions
>+  attribute boolean? atomic;
>+  attribute boolean? busy;
Hmm, which spec should I be looking at for these?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #5)

> >+  attribute boolean? atomic;
> >+  attribute boolean? busy;
> Hmm, which spec should I be looking at for these?

These are basically copied from ARIA [1]. Also these properties are present in AOM ARIA reflection document [2], prefixed by 'aria', since they are hosted on Element interface. And also these properties will moved into virtual accessible node (this part is yet to be written). It was used to be not empty, when we started this project, but the spec is still quite fickle.

Anyway, a long story short, we keep orienting to existing ARIA properties, putting them into the interface. You may also want to leave this part to me, if you like.

[1] https://www.w3.org/WAI/PF/aria/states_and_properties
[2] https://wicg.github.io/aom/spec/aria-reflection.html
[3] https://wicg.github.io/aom/spec/virtual-accessibility-nodes.html

Comment 7

[dora.tokio]

Attachment: part1: boolean attributes

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8985785 [details] [diff] [review]
part1: boolean attributes

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

::: accessible/aom/AccessibleNode.h
@@ +27,5 @@
>  struct ParentObject;
>  
> +#define ANODE_DECL_BOOL_PROP(_name)                     \
> +  dom::Nullable<bool> Get##_name();                     \
> +  void Set##_name(const dom::Nullable<bool>& a##_name);

can you combine the method declaration and its implementation to keep methods inline

@@ +89,5 @@
> +    eMultiselectable,
> +    eReadOnly,
> +    eRequired,
> +    eSelected
> +  };

mm, curious if these can be autogenerated somehow too

::: accessible/tests/mochitest/aom/test_general.html
@@ +104,5 @@
> +    is(anode.hidden, null, "anode.hidden is null");
> +    anode.hidden = true;
> +    is(anode.hidden, true, "anode.hidden was assigned true");
> +    anode.hidden = null;
> +    is(anode.hidden, null, "anode.hidden was assigned null");

this can be wrapped nicely in a helper function like:

function test_bool_prop(anode, prop)
{
  is(anode[prop], null, `anode.${prop} should be null`);
  ...
}
and then
test_bool_prop(anode, 'hidden');

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8985785 [details] [diff] [review]
part1: boolean attributes

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

cancelling request until comments are addressed

Comment 10

[dora.tokio]

Attachment: part1: boolean attributes

(In reply to alexander :surkov from comment #8)
> Comment on attachment 8985785 [details] [diff] [review]
> part1: boolean attributes
> 
> Review of attachment 8985785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/aom/AccessibleNode.h
> @@ +27,5 @@
> >  struct ParentObject;
> >  
> > +#define ANODE_DECL_BOOL_PROP(_name)                     \
> > +  dom::Nullable<bool> Get##_name();                     \
> > +  void Set##_name(const dom::Nullable<bool>& a##_name);
> 
> can you combine the method declaration and its implementation to keep
> methods inline

I moved the definitions of Set/GetBooleanProperty to header file too.


> @@ +89,5 @@
> > +    eMultiselectable,
> > +    eReadOnly,
> > +    eRequired,
> > +    eSelected
> > +  };
> 
> mm, curious if these can be autogenerated somehow too

I can't come up with any simple idea to autogenerate an enum class.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #10)

> > @@ +89,5 @@
> > > +    eMultiselectable,
> > > +    eReadOnly,
> > > +    eRequired,
> > > +    eSelected
> > > +  };
> > 
> > mm, curious if these can be autogenerated somehow too
> 
> I can't come up with any simple idea to autogenerate an enum class.

you could try to use MOZ_FOREACH macros (https://dxr.mozilla.org/mozilla-central/source/mfbt/MacroForEach.h#19), something like this:

#define ENUM_BOOL(arg) \
  e##arg

#define FUNC_BOOL(arg) \
  // func def goes here

#define ENUM_FOR_EACH(arg) \
  ENUM_##arg

#define FUNC_FOR_EACH(arg) \
  FUNC_##arg

#define DEFINE_PROPS(...) \
enum { \
  MOZ_FOREACH(ENUM_FOR_EACH, (), __VA_ARGS__)
}; \
MOZ_FOREACH(FUNC_FOR_EACH, (), __VA_ARGS__)

DEFINE_PROPS(
  BOOL(Hidden),
  BOOL(Selected)
)

Olli, how does it sound?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

If using macros makes code easier to maintain, sounds good.
I haven't used MOZ_FOREACH, and looks like it is rarely used, so be careful :)

Comment 13

[dora.tokio]

Attachment: part1: add boolean attributes

I tried using MOZ_FOR_EACH, but this patch is a little different from surkov's sketch in that not all properties are defined in the same enum, but they are separated by their types. I feel this would be more suitable semantically.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8987680 [details] [diff] [review]
part1: add boolean attributes

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

looks good with me, Olli, may we have your thumb-up on the approach and stuff?

::: accessible/aom/AccessibleNode.h
@@ +97,5 @@
> +      return Nullable<bool>(data);
> +    }
> +    return Nullable<bool>();
> +  }
> +  

nit: whitespace

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8987680 [details] [diff] [review]
part1: add boolean attributes

Using HashTable to store boolean values seems rather heavy. Hashtable accesses aren't too fast.
Using Nullable<bool> as member variable might be ok.
If that takes too much memory, using 3 or 4 bits for each attribute
(null, false, true, (+unused)) might be one option.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8987680 [details] [diff] [review]
> part1: add boolean attributes
> 
> Using HashTable to store boolean values seems rather heavy. Hashtable
> accesses aren't too fast.

Would we use hashtables to store integer and string types? It's about 40+ properties, and presumably only several of them will be used at time. If so, would it be benefical if to deal with booleans separately, or it's ok to go with hashtables for code simplicity?

Comment 17

[dora.tokio]

Attachment: patch1: boolean attributes

Comment 18

[dora.tokio]

Attachment: patch2: unsigned long attributes

Comment 19

[dora.tokio]

Attachment: patch3: long & double attributes

Comment 20

[dora.tokio]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8987680 [details] [diff] [review]
> part1: add boolean attributes
> 
> Using HashTable to store boolean values seems rather heavy. Hashtable
> accesses aren't too fast.
> Using Nullable<bool> as member variable might be ok.
> If that takes too much memory, using 3 or 4 bits for each attribute
> (null, false, true, (+unused)) might be one option.

I wrote new patches about unsigned long, long and double attributes. If the way of implementing boolean attributes has some problem, I'll rewrite it without Hashtable and send the patch again.

Comment 21

[dora.tokio]

Attachment: patch4: string attributes

The attribute 'role' had already been defined, and its name conflicted with that of DOMString attribute 'role' I added in this patch. So I temporarily commented out the code related to the previous one.

Comment 22

[alexander :surkov (:asurkov)]

It makes sense to ni? Olli on comment #20 to check if he'ok with that

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 8989039 [details] [diff] [review]
patch4: string attributes

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

It is unfortunate you have to disable computed role thing, but agreed you dont have other options. If you choose to comment out the method, then please provide a good comment for this.

Comment 24

[dora.tokio]

(In reply to alexander :surkov from comment #23)
> Comment on attachment 8989039 [details] [diff] [review]
> patch4: string attributes
> 
> Review of attachment 8989039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It is unfortunate you have to disable computed role thing, but agreed you
> dont have other options. If you choose to comment out the method, then
> please provide a good comment for this.

How about renaming the previous 'role' attribute to something like 'computedRole'?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Why any Hashtable usage? Hashtables are slow. Sure, they are O(1), but with quite high constant time.

Comment 26

[dora.tokio]

Attachment: patch1

(In reply to Olli Pettay [:smaug] from comment #25)
> Why any Hashtable usage? Hashtables are slow. Sure, they are O(1), but with
> quite high constant time.

I rewrote the patch of boolean attributes into the boolean operation version, instead of Hashtable.

Comment 27

[dora.tokio]

Attachment: patch2: unsigned long attributes

Comment 28

[dora.tokio]

Attachment: patch3: long & double attributes

Comment 29

[dora.tokio]

Attachment: patch4: string attributes

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #25)
> Why any Hashtable usage? Hashtables are slow. Sure, they are O(1), but with
> quite high constant time.

What I like about maps is they are not hungry for memory. If only a few properties are used, which is a typical usecase I think,then it should be more memory savvy, than other approaches if I dont miss anything. Also it's sort of an easy way for prototyping.

Comment 31

[dora.tokio]

Attachment: patch5: AccessibleNode attributes

I wrote a patch about AccessibleNode attributes temporarily, but I'm not sure this implementation conforms to the spec of AOM. Would you check it?
And I'm glad if you'd give me advice on what kind of test should I write.

Comment 32

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8989172 [details] [diff] [review]
patch1

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

This looks good, I added some comments that need to be addressed first. removing the r? for now since this will affect the other patches.

::: accessible/aom/AccessibleNode.cpp
@@ +41,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(AccessibleNode)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(AccessibleNode)
>  
> +AccessibleNode::AccessibleNode(nsINode* aNode)

nit: lets follow general indentation as follows (also alphabetical):

AccessibleNode::AccessibleNode(nsINode* aNode) :
  mBooleanProperties(0),
  mDOMNode(aNode)
...

::: accessible/aom/AccessibleNode.h
@@ +6,5 @@
>  
>  #ifndef A11Y_AOM_ACCESSIBLENODE_H
>  #define A11Y_AOM_ACCESSIBLENODE_H
>  
> +#include "nsDataHashtable.h"

This is not needed any more, as far as I can tell?

@@ +25,5 @@
>  
>  class DOMStringList;
>  struct ParentObject;
>  
> +#define ANODE_ENUM_FOR_EACH(_name) \

nit here and below, to be consistent with the way macros are defined elsewhere in the module, lets drop _ in argument names.

@@ +28,5 @@
>  
> +#define ANODE_ENUM_FOR_EACH(_name) \
> +    e##_name,
> +
> +#define ANODE_FUNC_FOR_EACH(_name, _type, _prop)           \

_name is confusing in this context, lets call it typeName?

Also _prop can be then renamed into _name and it will be consistent the macro above.

@@ +39,5 @@
> +  {                                                        \
> +    SetProperty(AOM##_name##Property::e##_prop, a##_prop); \
> +  }                                                        \
> +
> +#define ANODE_DEFINE_BOOL_PROPS(...)                                 \

I think you can make it more generic (useful for the other patches so you would not have to define a macro for each type):

#define ANODE_PROPS(typeName, type, ...)                            \
enum class AOM##typeName##Property {                                \
  MOZ_FOR_EACH(ANODE_ENUM_FOR_EACH, (), (__VA_ARGS__))              \
};                                                                  \
MOZ_FOR_EACH(ANODE_FUNC_FOR_EACH, (typeName, type,), (__VA_ARGS__)) \

then call 

ANODE_PROPS(Boolean, bool,
  Atomic,
  ...
)

@@ +70,5 @@
>             ErrorResult& aRv);
>  
>    static bool IsAOMEnabled(JSContext*, JSObject*);
>  
> +  ANODE_DEFINE_BOOL_PROPS(

nit: here and in the declaration - drop the DEFINE

::: accessible/tests/mochitest/aom/test_general.html
@@ +40,5 @@
>        document.body.appendChild(iframe);
>      });
>    }
>  
> +  function test_bool_prop(anode, prop) {

nit: test_bool_prop -> testBoolProp (I realize Alex mentioned that name, but this file has a different convention).

@@ +110,5 @@
>        ok(attrs.includes(anode.attributes[i]),
>           `${anode.attributes[i]} attribute is expected and found`);
>      }
>  
> +    let boolProps = ["hidden", "modal", "multiline", "multiselectable", "readOnly",

nit: const instead of let

For readability it would be great to split properties and states as well as have them ordered alphabetically.

::: dom/webidl/AccessibleNode.webidl
@@ +17,5 @@
>    boolean has(DOMString... attributes);
>    [Throws]
>    any get(DOMString attribute);
> +
> +  // Widget properties

Here and in other comments, "widget" is a little too specific, lets use "Accessible"

Other property that seems to be missing: haspopup. Is that intentional?

@@ +18,5 @@
>    [Throws]
>    any get(DOMString attribute);
> +
> +  // Widget properties
> +  attribute boolean? hidden;

hidden is a state, not a property.

@@ +22,5 @@
> +  attribute boolean? hidden;
> +  attribute boolean? modal;
> +  attribute boolean? multiline;
> +  attribute boolean? multiselectable;
> +  attribute boolean? readOnly;

Shouldn't this be "readonly" and not "readOnly" (it's consistently all lower case everywhere)?

@@ +24,5 @@
> +  attribute boolean? multiline;
> +  attribute boolean? multiselectable;
> +  attribute boolean? readOnly;
> +  attribute boolean? required;
> +  attribute boolean? selected;

selected is a state, not a property.

@@ +26,5 @@
> +  attribute boolean? readOnly;
> +  attribute boolean? required;
> +  attribute boolean? selected;
> +
> +  // Widget states

Not sure if there's a reason to not include states such as busy, expanded, grabbed?

Comment 33

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8989185 [details] [diff] [review]
patch2: unsigned long attributes

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

Looks good, with comments addressed. Ill hold off on other reviews until patch1 and patch2 are updated but I think you get the idea. Thanks!

::: accessible/aom/AccessibleNode.h
@@ +45,5 @@
>      MOZ_FOR_EACH(ANODE_ENUM_FOR_EACH, (), (__VA_ARGS__))             \
>    };                                                                 \
>    MOZ_FOR_EACH(ANODE_FUNC_FOR_EACH, (Boolean, bool,), (__VA_ARGS__)) \
>  
> +#define ANODE_DEFINE_UINT_PROPS(...)                                  \

This function will not be needed with comments requested for patch 1

@@ +90,5 @@
>      Required,
>      Selected
>    )
>  
> +  ANODE_DEFINE_UINT_PROPS(

This will need to be updated to match changes requested in patch1

@@ +150,4 @@
>    // The 2k'th bit indicates whether the k'th boolean property is used(1) or not(0)
>    // and 2k+1'th bit contains the property's value(1:true, 0:false)
>    uint32_t mBooleanProperties;
> +  nsDataHashtable<nsUint32HashKey, uint32_t> mUIntProperties;

It's probably a good idea to come up with the initial size for mUIntProperties and initialize it in the constructor with that size. E.g. if there are only 6 UInt properties, then that should be it.

::: accessible/tests/mochitest/aom/test_general.html
@@ +125,5 @@
>      for (let boolProp of boolProps) {
>        test_bool_prop(anode, boolProp);
>      }
>  
> +    let uintProps = ["colIndex", "colSpan", "posInSet", "rowIndex",

const

Comment 34

[dora.tokio]

(In reply to Yura Zenevich [:yzen] from comment #32)
> Comment on attachment 8989172 [details] [diff] [review]
> patch1
> 
> Review of attachment 8989172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/AccessibleNode.webidl
> @@ +17,5 @@
> >    boolean has(DOMString... attributes);
> >    [Throws]
> >    any get(DOMString attribute);
> > +
> > +  // Widget properties
> 
> Here and in other comments, "widget" is a little too specific, lets use
> "Accessible"
> 
> Other property that seems to be missing: haspopup. Is that intentional?
> 
> @@ +18,5 @@
> >    [Throws]
> >    any get(DOMString attribute);
> > +
> > +  // Widget properties
> > +  attribute boolean? hidden;
> 
> hidden is a state, not a property.
> 
> @@ +22,5 @@
> > +  attribute boolean? hidden;
> > +  attribute boolean? modal;
> > +  attribute boolean? multiline;
> > +  attribute boolean? multiselectable;
> > +  attribute boolean? readOnly;
> 
> Shouldn't this be "readonly" and not "readOnly" (it's consistently all lower
> case everywhere)?
> 
> @@ +24,5 @@
> > +  attribute boolean? multiline;
> > +  attribute boolean? multiselectable;
> > +  attribute boolean? readOnly;
> > +  attribute boolean? required;
> > +  attribute boolean? selected;
> 
> selected is a state, not a property.
> 
> @@ +26,5 @@t 
> > +  attribute boolean? readOnly;
> > +  attribute boolean? required;
> > +  attribute boolean? selected;
> > +
> > +  // Widget states
> 
> Not sure if there's a reason to not include states such as busy, expanded,
> grabbed?

I basically implement properties based on the proposal, which is a copy of an unofficial AOM draft at the point of March. [1]
For example, in this proposal hasPopUp is categorized as DOMString. Which definition would it be better to adopt?

[1] https://docs.google.com/document/d/e/2PACX-1vQKV8G9lUyI2yNxsMcJsSojQFOtZCOa4kNu2mDDfKOFZNXyun39E8FEuLgzAFkrSASTUBD0Xe75X8jX/pub

Comment 35

[Yura Zenevich [:yzen] (please use "needinfo")]

hasPopup is interesting because it is boolean in ARIA 1.0 but a token in ARIA 1.1 so I think it's fine to not include it as boolean attributes, I was really looking at the 1.0 spec. 

Just checked grabbed and it is deprecated in 1.1, so no need to add it.

Also please ignore my readonly vs readOnly comment (I thought I removed it) and the busy comment (I missed it in the aria-live section).

Comment 36

[dora.tokio]

Attachment: patch1: boolean attributes

Comment 37

[dora.tokio]

Attachment: patch2: unsigned long attributes

Comment 38

[dora.tokio]

Attachment: patch3: long & double attributes

Comment 39

[dora.tokio]

Attachment: patch4: string attributes

The previous role attribute and GetRole() has been commented out temporarily because its name is conflicted with the newly added attribute in this patch. I think it'd be good to rename it to something like 'computedRole'.

Comment 40

[dora.tokio]

Attachment: patch5: AccessibleNode attributes

Note that the test code of AccessibleNode attributes is not added in this patch because I'm not sure how to write it. I'm glad if you would give me some advice.

Comment 41

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990258 [details] [diff] [review]
patch1: boolean attributes

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

This is great, thanks, please address the nits before requesting checkin.

::: accessible/aom/AccessibleNode.cpp
@@ +41,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(AccessibleNode)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(AccessibleNode)
>  
> +AccessibleNode::AccessibleNode(nsINode* aNode)

please use : on the same line as AccessibleNode::AccessibleNode(nsINode* aNode)

::: accessible/aom/AccessibleNode.h
@@ +24,5 @@
>  
>  class DOMStringList;
>  struct ParentObject;
>  
> +#define ANODE_ENUM_FOR_EACH(name) \

Just noticed this, this should be called ANODE_ENUM_FOR or ANODE_ENUM

@@ +27,5 @@
>  
> +#define ANODE_ENUM_FOR_EACH(name) \
> +    e##name,
> +
> +#define ANODE_FUNC_FOR_EACH(typeName, type, name)           \

Should also be renamed to ANODE_FUNC_FOR or ANODE_FUNC or something like ANODE_PROP_ACCESSOR_MUTATOR

::: dom/webidl/AccessibleNode.webidl
@@ +17,5 @@
>    boolean has(DOMString... attributes);
>    [Throws]
>    any get(DOMString attribute);
> +
> +  // Widget properties

Please rename widget into Accessible or Accessible node

@@ +18,5 @@
>    [Throws]
>    any get(DOMString attribute);
> +
> +  // Widget properties
> +  attribute boolean? hidden;

Please move hidden into states section.

@@ +24,5 @@
> +  attribute boolean? multiline;
> +  attribute boolean? multiselectable;
> +  attribute boolean? readOnly;
> +  attribute boolean? required;
> +  attribute boolean? selected;

Please move selected into states section.

@@ +26,5 @@
> +  attribute boolean? readOnly;
> +  attribute boolean? required;
> +  attribute boolean? selected;
> +
> +  // Widget states

Please rename widget into Accessible or Accessible node

Comment 42

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990258 [details] [diff] [review]
patch1: boolean attributes

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

::: accessible/aom/AccessibleNode.h
@@ +99,5 @@
> +    return dom::Nullable<bool>();
> +  }
> +
> +  void SetProperty(AOMBooleanProperty aProperty,
> +                          const dom::Nullable<bool>& aValue)

also nit: indentation here

Comment 43

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990259 [details] [diff] [review]
patch2: unsigned long attributes

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

Looks good with nits addressed

::: accessible/aom/AccessibleNode.h
@@ +131,5 @@
> +    return dom::Nullable<uint32_t>();
> +  }
> +
> +  void SetProperty(AOMUIntProperty aProperty,
> +                          const dom::Nullable<uint32_t>& aValue)

nit: indenation

::: dom/webidl/AccessibleNode.webidl
@@ +34,5 @@
>    // Live regions
>    attribute boolean? atomic;
>    attribute boolean? busy;
> +
> +  // Collections.

Make sure these are alphabetic (e.g. level).

Comment 44

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990259 [details] [diff] [review]
patch2: unsigned long attributes

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

Looks good with nits addressed

::: accessible/aom/AccessibleNode.h
@@ +121,5 @@
>                                             : mBooleanProperties & ~(1U << (2 * num + 1)));
>      }
>    }
>  
> +  dom::Nullable<uint32_t> GetProperty(AOMUIntProperty aProperty)

I just looked at the remaining patches and we have a lot of repetition for attributes that are saved in hashtables (int, uint, double, anode props) we can probably write another macro for GetProperty and SetProperty methods for those types?

@@ +131,5 @@
> +    return dom::Nullable<uint32_t>();
> +  }
> +
> +  void SetProperty(AOMUIntProperty aProperty,
> +                          const dom::Nullable<uint32_t>& aValue)

nit: indenation

::: dom/webidl/AccessibleNode.webidl
@@ +34,5 @@
>    // Live regions
>    attribute boolean? atomic;
>    attribute boolean? busy;
> +
> +  // Collections.

Make sure these are alphabetic (e.g. level).

Comment 45

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990260 [details] [diff] [review]
patch3: long & double attributes

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

Looks good, please see my comment in previous patch about a macro for GetProperty and SetProperty for attributes stored in hashtables.

::: dom/webidl/AccessibleNode.webidl
@@ +26,5 @@
>    attribute boolean? readOnly;
>    attribute boolean? required;
>    attribute boolean? selected;
>  
> +  // Control values

Control -> Range

Comment 46

[dora.tokio]

(In reply to Yura Zenevich [:yzen] from comment #44)
> Comment on attachment 8990259 [details] [diff] [review]
> patch2: unsigned long attributes
> 
> Review of attachment 8990259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good with nits addressed
> 
> ::: accessible/aom/AccessibleNode.h
> @@ +121,5 @@
> >                                             : mBooleanProperties & ~(1U << (2 * num + 1)));
> >      }
> >    }
> >  
> > +  dom::Nullable<uint32_t> GetProperty(AOMUIntProperty aProperty)
> 
> I just looked at the remaining patches and we have a lot of repetition for
> attributes that are saved in hashtables (int, uint, double, anode props) we
> can probably write another macro for GetProperty and SetProperty methods for
> those types?

Because each Get/SetProperty methods are slightly different from one another, and also it is not necessary to duplicate these by MOZ_FOR_EACH, I wrote these methods directly without macros. But if it'd be better to use a macro, I'll rewrite it.

Comment 47

[dora.tokio]

Would it be better to rewrite Get/SetProperty (of UInt, Int and Double) methods? I think other methods are not suitable for being converted into macros.

Comment 48

[Yura Zenevich [:yzen] (please use "needinfo")]

Yes, sorry I should've specified that, indeed, these only makes sense in a macro for the ones you mentioned. There are only a few variable things in those methods that could be taken out as arguments: property type, data type and the default value. I guess the string has the refptr that does not suite the pattern very well.

Comment 49

[Yura Zenevich [:yzen] (please use "needinfo")]

Just realized that your comments in webidl are taken from the AOM spec. Feel free to leave them the way you wrote them.

Comment 50

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990266 [details] [diff] [review]
patch5: AccessibleNode attributes

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

thanks, please see nits and comments.

::: accessible/aom/AccessibleNode.h
@@ +53,5 @@
>  
> +#define ANODE_RELATION_FUNC_FOR_EACH(name)              \
> +  already_AddRefed<AccessibleNode> Get##name()          \
> +  {                                                     \
> +    RefPtr<AccessibleNode> data;                        \

Could you assign data right away?

@@ +278,5 @@
> +    return nullptr;
> +  }
> +
> +  void SetProperty(AOMRelationProperty aProperty,
> +                           AccessibleNode* aValue)

nit: indentation

@@ +280,5 @@
> +
> +  void SetProperty(AOMRelationProperty aProperty,
> +                           AccessibleNode* aValue)
> +  {
> +    if (!aValue) {

nit: could you flip this so we check if (aValue) instead

Comment 51

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8990262 [details] [diff] [review]
patch4: string attributes

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

thanks

::: accessible/aom/AccessibleNode.cpp
@@ +76,5 @@
>  {
>    return mDOMNode->GetParentObject();
>  }
>  
> +/* void

Feel free to leave and rename to computedRole

::: accessible/aom/AccessibleNode.h
@@ +40,5 @@
>      SetProperty(AOM##typeName##Property::e##name, a##name); \
>    }                                                         \
>  
> +#define ANODE_STRING_FUNC_FOR_EACH(name)                     \
> +  void Get##name(nsAString & a##name)                        \

nit: nsAString & -> nsAString&

::: accessible/tests/mochitest/aom/test_general.html
@@ +88,5 @@
>    function checkImplementation(ifrDoc) {
>      let anode = ifrDoc.accessibleNode;
>      ok(anode, "DOM document has accessible node");
>  
> +    // is(anode.role, "document", "correct role of a document accessible node");

If you decide to leave and rename to computedRole, keep this test

::: dom/webidl/AccessibleNode.webidl
@@ +5,5 @@
>   */
>  
>  [Func="mozilla::dom::AccessibleNode::IsAOMEnabled"]
>  interface AccessibleNode {
> +  // readonly attribute DOMString role;

I don't mind leaving this for now and renaming into computedRole

@@ +50,3 @@
>  
>    // Control values
> +  attribute DOMString? valueText;

I know this is the order in the spec, but lets make this alphabetical

Comment 52

[dora.tokio]

Attachment: patch1: boolean attributes

Comment 53

[dora.tokio]

Attachment: patch2: unsigned long, long & double attributes

Comment 54

[dora.tokio]

Attachment: patch3: DOMString attributes

I integrated part2 and part3 because I newly wrote a macro used for part2 & 3, and part3 became too short for one patch.

Comment 55

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8991885 [details] [diff] [review]
patch1: boolean attributes

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

Great, Thanks!

Comment 56

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8991886 [details] [diff] [review]
patch2: unsigned long, long & double attributes

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

Thanks

Comment 57

[dora.tokio]

I'm afraid to say but the part5 patch about AccessibleNode doesn't include the test for AccessibleNode attributes because I would write it after having the implementation checked and getting some advice about how to write a test.

Comment 58

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #57)
> I'm afraid to say but the part5 patch about AccessibleNode doesn't include
> the test for AccessibleNode attributes because I would write it after having
> the implementation checked and getting some advice about how to write a test.

Let's fire a new bug on this. I think we can deal with it separately.

Comment 59

[dora.tokio]

(In reply to alexander :surkov from comment #58)
> (In reply to dora.tokio from comment #57)
> Let's fire a new bug on this. I think we can deal with it separately.

I think the part5 is not consistent with part1 ~ 3 and the merge won't succeed. I'll rewrite it now.

Comment 60

[dora.tokio]

Attachment: patch3: DOMString attributes

fix some indentation

Comment 61

[dora.tokio]

Attachment: patch4: AccessibleNode attributes

I fix the consistency and added a simple test for AccessibleNode attributes (I'm not sure this suffices to check the behavior of those attributes, but let's keep it simple for now)

Comment 62

[dora.tokio]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1d8c204d15bcab25b7e4e90ccd82d5dab58b100&selectedJob=188477552

Do I have to fix these errors? To fix this, I must re-arrange the properties in AccessibleNode.h in alphabetical order, but that won't look good.

Comment 63

[Yura Zenevich [:yzen] (please use "needinfo")]

I don't think we have any choice, so I think it's fine to just rearrange. Alex?

Comment 64

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #62)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f1d8c204d15bcab25b7e4e90ccd82d5dab58b100&selectedJob=1
> 88477552
> 
> Do I have to fix these errors? To fix this, I must re-arrange the properties
> in AccessibleNode.h in alphabetical order, but that won't look good.

It's not about alphabetical order, it's about mismatch between declaration and initialization order.

Also all these hashes especially separates ones to handle uint/int look weird. I think it's ok to go with these now, as it will be ok for ideas prototyping, but please file a new bug, where we can find out a nicer solution.

Comment 65

[alexander :surkov (:asurkov)]

Comment on attachment 8992490 [details] [diff] [review]
patch3: DOMString attributes

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

::: accessible/aom/AccessibleNode.cpp
@@ +77,5 @@
>    return mDOMNode->GetParentObject();
>  }
>  
>  void
> +AccessibleNode::GetComputedRole(nsAString& aRole)

inconsistent naming, but it's ok for now, please file a new bug for this

Comment 66

[alexander :surkov (:asurkov)]

Comment on attachment 8992492 [details] [diff] [review]
patch4: AccessibleNode attributes

># HG changeset patch
># User Tokio Kajitsuka <[e-mail]>
># Date 1531784257 -32400
>#      Tue Jul 17 08:37:37 2018 +0900
># Node ID b67be9e697211924836f4f9e70e822144c27c8ba
># Parent  7398ef9a0288903088e1dc7f4ef8de0f6f309a2c
>Bug 1468110 - part4: AccessibleNode attributes
>Add AccessibleNode attributes to AccessibleNode
>
>diff --git a/accessible/aom/AccessibleNode.cpp b/accessible/aom/AccessibleNode.cpp
>--- a/accessible/aom/AccessibleNode.cpp
>+++ b/accessible/aom/AccessibleNode.cpp
>@@ -42,16 +42,17 @@ NS_INTERFACE_MAP_END
> NS_IMPL_CYCLE_COLLECTING_ADDREF(AccessibleNode)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(AccessibleNode)
> 
> AccessibleNode::AccessibleNode(nsINode* aNode) :
>     mBooleanProperties(0),
>     mDoubleProperties(3),
>     mDOMNode(aNode),
>     mIntProperties(3),
>+    mRelationProperties(3),
>     mStringProperties(16),
>     mUIntProperties(6)
> {
>   nsAccessibilityService* accService = GetOrCreateAccService();
>   if (!accService) {
>     return;
>   }
> 
>diff --git a/accessible/aom/AccessibleNode.h b/accessible/aom/AccessibleNode.h
>--- a/accessible/aom/AccessibleNode.h
>+++ b/accessible/aom/AccessibleNode.h
>@@ -46,28 +46,45 @@ struct ParentObject;
>     return GetProperty(AOMStringProperty::e##name, a##name); \
>   }                                                          \
>                                                              \
>   void Set##name(const nsAString& a##name)                   \
>   {                                                          \
>     SetProperty(AOMStringProperty::e##name, a##name);        \
>   }                                                          \
> 
>+#define ANODE_RELATION_FUNC(name)                       \
>+  already_AddRefed<AccessibleNode> Get##name()          \
>+  {                                                     \
>+    return GetProperty(AOMRelationProperty::e##name);   \
>+  }                                                     \
>+                                                        \
>+  void Set##name(AccessibleNode* a##name)               \
>+  {                                                     \
>+    SetProperty(AOMRelationProperty::e##name, a##name); \
>+  }                                                     \
>+
> #define ANODE_PROPS(typeName, type, ...)                     \
>   enum class AOM##typeName##Property {                       \
>     MOZ_FOR_EACH(ANODE_ENUM, (), (__VA_ARGS__))              \
>   };                                                         \
>   MOZ_FOR_EACH(ANODE_FUNC, (typeName, type,), (__VA_ARGS__)) \
> 
> #define ANODE_STRING_PROPS(...)                               \
>   enum class AOMStringProperty {                              \
>     MOZ_FOR_EACH(ANODE_ENUM, (), (__VA_ARGS__))               \
>   };                                                          \
>   MOZ_FOR_EACH(ANODE_STRING_FUNC, (), (__VA_ARGS__))          \
> 
>+#define ANODE_RELATION_PROPS(...)                               \
>+  enum class AOMRelationProperty {                              \
>+    MOZ_FOR_EACH(ANODE_ENUM, (), (__VA_ARGS__))                 \
>+  };                                                            \
>+  MOZ_FOR_EACH(ANODE_RELATION_FUNC, (), (__VA_ARGS__))          \
>+
> #define ANODE_ACCESSOR_MUTATOR(typeName, type, defVal)                          \
>   nsDataHashtable<nsUint32HashKey, type> m##typeName##Properties;               \
>                                                                                 \
>   dom::Nullable<type> GetProperty(AOM##typeName##Property aProperty)            \
>   {                                                                             \
>     type value = defVal;                                                        \
>     if (m##typeName##Properties.Get(static_cast<int>(aProperty), &value)) {     \
>       return dom::Nullable<type>(value);                                        \
>@@ -159,16 +176,22 @@ public:
>   )
> 
>   ANODE_PROPS(Double, double,
>     ValueMax,
>     ValueMin,
>     ValueNow
>   )
> 
>+  ANODE_RELATION_PROPS(
>+    ActiveDescendant,
>+    Details,
>+    ErrorMessage
>+  )
>+
> protected:
>   AccessibleNode(const AccessibleNode& aCopy) = delete;
>   AccessibleNode& operator=(const AccessibleNode& aCopy) = delete;
>   virtual ~AccessibleNode();
> 
>   void GetProperty(AOMStringProperty aProperty, nsAString& aRetval) {
>     nsString data;
>     if (!mStringProperties.Get(static_cast<int>(aProperty), &data)) {
>@@ -208,23 +231,43 @@ protected:
>                                            : mBooleanProperties & ~(1U << (2 * num + 1)));
>     }
>   }
> 
>   ANODE_ACCESSOR_MUTATOR(Double, double, 0.0)
>   ANODE_ACCESSOR_MUTATOR(Int, int32_t, 0)
>   ANODE_ACCESSOR_MUTATOR(UInt, uint32_t, 0)
> 
>+  already_AddRefed<AccessibleNode> GetProperty(AOMRelationProperty aProperty)
>+  {
>+    RefPtr<AccessibleNode> data;
>+    if (mRelationProperties.Get(static_cast<int>(aProperty), &data)) {
>+      return data.forget();
>+    }
>+    return nullptr;
>+  }
>+
>+  void SetProperty(AOMRelationProperty aProperty,
>+                   AccessibleNode* aValue)
>+  {
>+    if (!aValue) {
>+      mRelationProperties.Remove(static_cast<int>(aProperty));
>+    } else {
>+      mRelationProperties.Put(static_cast<int>(aProperty), aValue);
>+    }
>+  }
>+
>   nsDataHashtable<nsUint32HashKey, nsString> mStringProperties;
>   // The 2k'th bit indicates whether the k'th boolean property is used(1) or not(0)
>   // and 2k+1'th bit contains the property's value(1:true, 0:false)
>   uint32_t mBooleanProperties;
> 
>   RefPtr<a11y::Accessible> mIntl;
>   RefPtr<nsINode> mDOMNode;
>   RefPtr<dom::DOMStringList> mStates;
>+  nsDataHashtable<nsUint32HashKey, RefPtr<AccessibleNode> > mRelationProperties;
> };
> 
> } // dom
> } // mozilla
> 
> 
> #endif // A11Y_JSAPI_ACCESSIBLENODE
>diff --git a/accessible/tests/mochitest/aom/test_general.html b/accessible/tests/mochitest/aom/test_general.html
>--- a/accessible/tests/mochitest/aom/test_general.html
>+++ b/accessible/tests/mochitest/aom/test_general.html
>@@ -79,16 +79,23 @@
>   function testUIntProp(anode, prop) {
>     is(anode[prop], null, `anode.${prop} should be null`);
>     anode[prop] = 4294967295;
>     is(anode[prop], 4294967295, `anode.${prop} was assigned 4294967295`);
>     anode[prop] = null;
>     is(anode[prop], null, `anode.${prop} was assigned null`);
>   }
> 
>+  function testRelationProp(anode, node, prop) {
>+    is(anode[prop], null, `anode.${prop} should be null`);
>+    anode[prop] = node.accessibleNode;
>+    is(anode[prop], node.accessibleNode, `anode.${prop} was assigned AccessibleNode`);
>+    anode[prop] = null;
>+    is(anode[prop], null, `anode.${prop} was assigned null`);
>+  }
>   // Check that the WebIDL is as expected.
>   function checkImplementation(ifrDoc) {
>     let anode = ifrDoc.accessibleNode;
>     ok(anode, "DOM document has accessible node");
> 
>     is(anode.computedRole, "document", "correct role of a document accessible node");
>     is(anode.DOMNode, ifrDoc, "correct DOM Node of a document accessible node");
> 
>@@ -183,12 +190,18 @@
>     anode = node.accessibleNode;
>     is(anode, node.accessibleNode, "an AccessibleNode is properly cached");
> 
>     // Adopting node to another document doesn't change .accessibleNode
>     let anotherDoc = document.implementation.createDocument("", "", null);
>     let adopted_node = anotherDoc.adoptNode(node);
>     is(anode, adopted_node.accessibleNode, "adopting node to another document doesn't change node.accessibleNode");
> 
>+    const relationProps = ["activeDescendant", "details", "errorMessage"];
>+
>+    for (const relationProp of relationProps) {
>+      testRelationProp(anode, node, relationProp);
>+    }
>+
>     finish();
>   }
>   </script>
> </head>
>diff --git a/dom/webidl/AccessibleNode.webidl b/dom/webidl/AccessibleNode.webidl
>--- a/dom/webidl/AccessibleNode.webidl
>+++ b/dom/webidl/AccessibleNode.webidl
>@@ -56,16 +56,21 @@ interface AccessibleNode {
>   attribute boolean? selected;
> 
>   // Live regions
>   attribute boolean? atomic;
>   attribute boolean? busy;
>   attribute DOMString? live;
>   attribute DOMString? relevant;
> 
>+  // Other relationships
>+  attribute AccessibleNode? activeDescendant;
>+  attribute AccessibleNode? details;
>+  attribute AccessibleNode? errorMessage;
>+
>   // Collections.
>   attribute long? colCount;
>   attribute unsigned long? colIndex;
>   attribute unsigned long? colSpan;
>   attribute unsigned long? level;
>   attribute unsigned long? posInSet;
>   attribute long? rowCount;
>   attribute unsigned long? rowIndex;

Comment 67

[alexander :surkov (:asurkov)]

Comment on attachment 8992492 [details] [diff] [review]
patch4: AccessibleNode attributes

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

my understanding that Yura reviewed it already, so rs from me until you look a review from me for a particular reason

::: accessible/aom/AccessibleNode.h
@@ +262,5 @@
>  
>    RefPtr<a11y::Accessible> mIntl;
>    RefPtr<nsINode> mDOMNode;
>    RefPtr<dom::DOMStringList> mStates;
> +  nsDataHashtable<nsUint32HashKey, RefPtr<AccessibleNode> > mRelationProperties;

it's sort of weird to use hash to 3 properties, I'm curious if we could use a single hash for all of them, but again let's go with it for now.

Comment 68

[Andreea Pavel [:apavel]]

I've tried to land this, but for the first patch i receive transaction abort:  "Changeset b2b67e4792d2 alters WebIDL file(s) without DOM peer review:
remote: dom/webidl/AccessibleNode.webidl"

Comment 69

[Yura Zenevich [:yzen] (please use "needinfo")]

Alex? Is that for Olli?

Comment 70

[alexander :surkov (:asurkov)]

Comment on attachment 8991885 [details] [diff] [review]
patch1: boolean attributes

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

this is Olli's area for sure, but he's on vacations. Maybe Andrew could approve these?

Comment 71

[Andrew McCreight [:mccr8]]

Comment on attachment 8991885 [details] [diff] [review]
patch1: boolean attributes

It looks like this isn't exposed to the web (it is behind a pref), so it should be ok, but Kyle might have more useful feedback than me.

Comment 72

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8991885 [details] [diff] [review]
patch1: boolean attributes

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

LGTM, though it might be nice to throw the various spec info from Comment 6 in as a comment, since we usually try to have the defining spec in our WebIDLs. I realize that's rather complicated here since this pulls from multiple specs, so whatever you feel is best there.

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7bd2bfe551
add AccessibleNode boolean attributes, r=yzen, qdot

Comment 74

[alexander :surkov (:asurkov)]

(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #72)
> Comment on attachment 8991885 [details] [diff] [review]
> patch1: boolean attributes
> 
> Review of attachment 8991885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, though it might be nice to throw the various spec info from Comment 6
> in as a comment, since we usually try to have the defining spec in our
> WebIDLs. I realize that's rather complicated here since this pulls from
> multiple specs, so whatever you feel is best there.

makes sense, thank you, landed the first patch with updated comment

Comment 75

[alexander :surkov (:asurkov)]

started a try build for 2nd patch (https://treeherder.mozilla.org/#/jobs?repo=try&revision=29a145ad28cf2484c022ec6d811f295907dae3a6&selectedJob=189208159):

builds/worker/workspace/build/src/accessible/aom/AccessibleNode.h:165:12: error: 'mozilla::dom::AccessibleNode::mBooleanProperties' will be initialized after [-Werror=reorder]
/builds/worker/workspace/build/src/accessible/aom/AccessibleNode.h:50:42: error: 'nsDataHashtable<nsUint32HashKey, double> mozilla::dom::AccessibleNode::mDoubleProperties' [-Werror=reorder]
/builds/worker/workspace/build/src/accessible/aom/AccessibleNode.cpp:45:1: error: when initialized here [-Werror=reorder]
/builds/worker/workspace/build/src/accessible/aom/AccessibleNode.h:168:19: error: 'mozilla::dom::AccessibleNode::mDOMNode' will be initialized after [-Werror=reorder]
/builds/worker/workspace/build/src/accessible/aom/AccessibleNode.h:50:42: error: 'nsDataHashtable<nsUint32HashKey, int> mozilla::dom::AccessibleNode::mIntProperties' [-Werror=reorder]
/builds/worker/workspace/build/src/accessible/aom/AccessibleNode.cpp:45:1: error: when initialized here [-Werror=reorder]

Comment 76

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0e7bd2bfe551

Comment 77

[dora.tokio]

Attachment: patch2: unsigned long attributes

After all we have to rearrange the properties in the same order as the declarations?

I made the initialization order correspond with the declarations, though this contradicts what I was asked in commend #32.

Comment 78

[dora.tokio]

Attachment: patch2: unsigned long attributes

Comment 79

[dora.tokio]

(In reply to alexander :surkov (:asurkov) from comment #65)
> Comment on attachment 8992490 [details] [diff] [review]
> patch3: DOMString attributes
> 
> Review of attachment 8992490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/aom/AccessibleNode.cpp
> @@ +77,5 @@
> >    return mDOMNode->GetParentObject();
> >  }
> >  
> >  void
> > +AccessibleNode::GetComputedRole(nsAString& aRole)
> 
> inconsistent naming, but it's ok for now, please file a new bug for this

If what should be fixed is only renaming ComputedRole, could you tell me the proper name? I'll fix it in the next patch.

Comment 80

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #77)
> Created attachment 8993952 [details] [diff] [review]
> patch2: unsigned long attributes
> 
> After all we have to rearrange the properties in the same order as the
> declarations?
> 
> I made the initialization order correspond with the declarations, though
> this contradicts what I was asked in commend #32.

do you mean this alphabetical concern?

(In reply to Yura Zenevich [:yzen] from comment #32)
> ::: accessible/aom/AccessibleNode.cpp
> @@ +41,5 @@
> >  
> >  NS_IMPL_CYCLE_COLLECTING_ADDREF(AccessibleNode)
> >  NS_IMPL_CYCLE_COLLECTING_RELEASE(AccessibleNode)
> >  
> > +AccessibleNode::AccessibleNode(nsINode* aNode)
> 
> nit: lets follow general indentation as follows (also alphabetical):
>
> AccessibleNode::AccessibleNode(nsINode* aNode) :
>   mBooleanProperties(0),
>   mDOMNode(aNode)
> ...

usually members are not ordered alphabetically, but they are grouped by a type for memory allocation. So I'd say you can keep them the way you do, if Yura is fine with that.

Comment 81

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #79)
> (In reply to alexander :surkov (:asurkov) from comment #65)
> > >  void
> > > +AccessibleNode::GetComputedRole(nsAString& aRole)
> > 
> > inconsistent naming, but it's ok for now, please file a new bug for this
> 
> If what should be fixed is only renaming ComputedRole, could you tell me the
> proper name? I'll fix it in the next patch.

I don't know yet. So AccessibleNode operates on computed properties like role, states, is(), has(), get(), and then we add here a bunch of conflicting setters/getters. We could have a separate interface or we could make new properties to return a computed value. Thus it makes sense to have a new bug where we can figure out solution.

Comment 82

[alexander :surkov (:asurkov)]

Comment on attachment 8993953 [details] [diff] [review]
patch2: unsigned long attributes

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

you need to have Kyle's thumb up on this one as well

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8cf9e6f7d6
add AccessibleNode unsigned long, long & double attributes attributes, r=yzen, qdot

Comment 84

[alexander :surkov (:asurkov)]

Tokio, patch3 is no longer clearly applied. Could you update it please?

Comment 85

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4b8cf9e6f7d6

Comment 86

[dora.tokio]

Attachment: patch3: DOMString Attributes

(In reply to alexander :surkov (:asurkov) from comment #81)
> I don't know yet. So AccessibleNode operates on computed properties like
> role, states, is(), has(), get(), and then we add here a bunch of
> conflicting setters/getters. We could have a separate interface or we could
> make new properties to return a computed value. Thus it makes sense to have
> a new bug where we can figure out solution.

I got it, and left the name of computedRole as it is.

Comment 87

[dora.tokio]

Attachment: patch4: AccessibleNode attributes

Comment 88

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc49c426b6b
add AccessibleNode string attributes, r=yzen, qdot

Comment 89

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9cc49c426b6b

Comment 90

[dora.tokio]

If you were not busy, could you take a look at the patch4 of AccessibleNode attributes?

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/020682cafeff
add AccessibleNode relation attributes, r=yzen, qdot

Comment 92

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #90)
> If you were not busy, could you take a look at the patch4 of AccessibleNode
> attributes?

sure, just bunch of unfortunate consequences, landed, we can close this one. Could you file follow-up please?

Comment 93

[dora.tokio]

(In reply to alexander :surkov (:asurkov) from comment #92)
> (In reply to dora.tokio from comment #90)
> > If you were not busy, could you take a look at the patch4 of AccessibleNode
> > attributes?
> 
> sure, just bunch of unfortunate consequences, landed, we can close this one.
> Could you file follow-up please?

https://bugzilla.mozilla.org/show_bug.cgi?id=1479177
https://bugzilla.mozilla.org/show_bug.cgi?id=1479178

I filed this two bugs for now. Is there any bug which should be filed?

By the way, would you tell me what NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE is? (a macro appearing in the line 53 of AccessibleNode.cpp)

Comment 94

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/020682cafeff

Comment 95

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #93)
> (In reply to alexander :surkov (:asurkov) from comment #92)
> > (In reply to dora.tokio from comment #90)
> > > If you were not busy, could you take a look at the patch4 of AccessibleNode
> > > attributes?
> > 
> > sure, just bunch of unfortunate consequences, landed, we can close this one.
> > Could you file follow-up please?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1479177
> https://bugzilla.mozilla.org/show_bug.cgi?id=1479178
> 
> I filed this two bugs for now. Is there any bug which should be filed?

only the next one, where the work will be continued :)

> 
> By the way, would you tell me what NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE is?
> (a macro appearing in the line 53 of AccessibleNode.cpp)

not sure about wrappercache postfix, it's better to ask Olli on this one, but this is a macros that implements cycle collection for this class.