Closed Bug 1468110 Opened 7 years ago Closed 7 years ago

Add attributes to AccessibleNode

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dora.tokio, Assigned: dora.tokio)

References

Details

Attachments

(4 files, 23 obsolete files)

7.76 KB, patch
yzen
: review+
qdot
: review+
Details | Diff | Splinter Review
9.49 KB, patch
qdot
: review+
Details | Diff | Splinter Review
11.94 KB, patch
Details | Diff | Splinter Review
8.20 KB, patch
Details | Diff | Splinter Review
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.
Blocks: weba11y
(In reply to dora.tokio from comment #0) > Steps to reproduce: It's a mistake.
Attached patch part1: boolean attributes (obsolete) — Splinter Review
Attachment #8985565 - Flags: review?(surkov.alexander)
Attachment #8985565 - Attachment description: patch → part1: boolean attributes
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)
Attachment #8985565 - Flags: review?(bugs)
(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 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?
(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
Attached patch part1: boolean attributes (obsolete) — Splinter Review
Attachment #8985565 - Attachment is obsolete: true
Attachment #8985565 - Flags: review?(surkov.alexander)
Attachment #8985565 - Flags: review?(bugs)
Attachment #8985785 - Flags: review?(surkov.alexander)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 on attachment 8985785 [details] [diff] [review] part1: boolean attributes Review of attachment 8985785 [details] [diff] [review]: ----------------------------------------------------------------- cancelling request until comments are addressed
Attachment #8985785 - Flags: review?(surkov.alexander)
Attached patch part1: boolean attributes (obsolete) — Splinter Review
(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.
Attachment #8985785 - Attachment is obsolete: true
Attachment #8986702 - Flags: review?(surkov.alexander)
(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?
Flags: needinfo?(bugs)
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 :)
Flags: needinfo?(bugs)
Attached patch part1: add boolean attributes (obsolete) — Splinter Review
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.
Attachment #8986702 - Attachment is obsolete: true
Attachment #8986702 - Flags: review?(surkov.alexander)
Attachment #8987680 - Flags: review?(surkov.alexander)
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
Attachment #8987680 - Flags: review?(surkov.alexander)
Attachment #8987680 - Flags: review+
Attachment #8987680 - Flags: feedback?(bugs)
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.
Attachment #8987680 - Flags: feedback?(bugs)
(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?
Priority: -- → P2
Attached patch patch1: boolean attributes (obsolete) — Splinter Review
Attachment #8987680 - Attachment is obsolete: true
Attachment #8988999 - Flags: review?(surkov.alexander)
Attached patch patch2: unsigned long attributes (obsolete) — Splinter Review
Attachment #8989000 - Flags: review?(surkov.alexander)
Attached patch patch3: long & double attributes (obsolete) — Splinter Review
Attachment #8989002 - Flags: review?(surkov.alexander)
(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.
Attachment #8988999 - Flags: review?(surkov.alexander) → review?(yzenevich)
Attachment #8989000 - Flags: review?(surkov.alexander) → review?(yzenevich)
Attachment #8989002 - Flags: review?(surkov.alexander) → review?(yzenevich)
Attached patch patch4: string attributes (obsolete) — Splinter Review
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.
Attachment #8989039 - Flags: review?(yzenevich)
It makes sense to ni? Olli on comment #20 to check if he'ok with that
Flags: needinfo?(bugs)
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.
Attachment #8989039 - Flags: review?(yzenevich) → review+
(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'?
Why any Hashtable usage? Hashtables are slow. Sure, they are O(1), but with quite high constant time.
Flags: needinfo?(bugs)
Attached patch patch1 (obsolete) — Splinter Review
(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.
Attachment #8988999 - Attachment is obsolete: true
Attachment #8988999 - Flags: review?(yzenevich)
Attachment #8989172 - Flags: review?(yzenevich)
Attached patch patch2: unsigned long attributes (obsolete) — Splinter Review
Attachment #8989000 - Attachment is obsolete: true
Attachment #8989000 - Flags: review?(yzenevich)
Attachment #8989185 - Flags: review?(yzenevich)
Attached patch patch3: long & double attributes (obsolete) — Splinter Review
Attachment #8989002 - Attachment is obsolete: true
Attachment #8989002 - Flags: review?(yzenevich)
Attachment #8989186 - Flags: review?(yzenevich)
Attached patch patch4: string attributes (obsolete) — Splinter Review
Attachment #8989039 - Attachment is obsolete: true
Attachment #8989187 - Flags: review?(yzenevich)
(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.
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.
Attachment #8989825 - Flags: review?(yzenevich)
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?
Attachment #8989172 - Flags: review?(yzenevich)
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
Attachment #8989185 - Flags: review?(yzenevich)
(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
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).
Attached patch patch1: boolean attributes (obsolete) — Splinter Review
Attachment #8989172 - Attachment is obsolete: true
Attachment #8990258 - Flags: review?(yzenevich)
Attached patch patch2: unsigned long attributes (obsolete) — Splinter Review
Attachment #8989185 - Attachment is obsolete: true
Attachment #8990259 - Flags: review?(yzenevich)
Attached patch patch3: long & double attributes (obsolete) — Splinter Review
Attachment #8989186 - Attachment is obsolete: true
Attachment #8989186 - Flags: review?(yzenevich)
Attachment #8990260 - Flags: review?(yzenevich)
Attached patch patch4: string attributes (obsolete) — Splinter Review
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'.
Attachment #8989187 - Attachment is obsolete: true
Attachment #8989187 - Flags: review?(yzenevich)
Attachment #8990262 - Flags: review?(yzenevich)
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.
Attachment #8989825 - Attachment is obsolete: true
Attachment #8989825 - Flags: review?(yzenevich)
Attachment #8990266 - Flags: review?(yzenevich)
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
Attachment #8990258 - Flags: review?(yzenevich) → review+
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 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).
Attachment #8990259 - Flags: review?(yzenevich) → review+
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 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
Attachment #8990260 - Flags: review?(yzenevich) → review+
(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.
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.
Flags: needinfo?(yzenevich)
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.
Flags: needinfo?(yzenevich)
Just realized that your comments in webidl are taken from the AOM spec. Feel free to leave them the way you wrote them.
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
Attachment #8990266 - Flags: review?(yzenevich) → review+
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
Attachment #8990262 - Flags: review?(yzenevich) → review+
Attachment #8990258 - Attachment is obsolete: true
Attachment #8991885 - Flags: review?(yzenevich)
Attachment #8990259 - Attachment is obsolete: true
Attachment #8991886 - Flags: review?(yzenevich)
Attached patch patch3: DOMString attributes (obsolete) — Splinter Review
I integrated part2 and part3 because I newly wrote a macro used for part2 & 3, and part3 became too short for one patch.
Attachment #8990260 - Attachment is obsolete: true
Attachment #8990262 - Attachment is obsolete: true
Attachment #8991889 - Flags: review?(yzenevich)
Comment on attachment 8991885 [details] [diff] [review] patch1: boolean attributes Review of attachment 8991885 [details] [diff] [review]: ----------------------------------------------------------------- Great, Thanks!
Attachment #8991885 - Flags: review?(yzenevich) → review+
Comment on attachment 8991886 [details] [diff] [review] patch2: unsigned long, long & double attributes Review of attachment 8991886 [details] [diff] [review]: ----------------------------------------------------------------- Thanks
Attachment #8991886 - Flags: review?(yzenevich) → review+
Attachment #8991889 - Flags: review?(yzenevich) → review+
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.
(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.
(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.
Attached patch patch3: DOMString attributes (obsolete) — Splinter Review
fix some indentation
Attachment #8991889 - Attachment is obsolete: true
Attachment #8992490 - Flags: review?(surkov.alexander)
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)
Attachment #8990266 - Attachment is obsolete: true
Attachment #8992492 - Flags: review?(surkov.alexander)
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.
Flags: needinfo?(yzenevich)
I don't think we have any choice, so I think it's fine to just rearrange. Alex?
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
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
Attachment #8992490 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8992492 [details] [diff] [review] patch4: AccessibleNode attributes ># HG changeset patch ># User Tokio Kajitsuka <kajitsuka-tokio@g.ecc.u-tokyo.ac.jp> ># 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;
Attachment #8992492 - Attachment is patch: true
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.
Attachment #8992492 - Flags: review?(surkov.alexander) → review+
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"
Flags: needinfo?(yzenevich)
Alex? Is that for Olli?
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
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?
Attachment #8991885 - Flags: review?(continuation)
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.
Attachment #8991885 - Flags: review?(continuation) → review?(kyle)
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.
Attachment #8991885 - Flags: review?(kyle) → review+
Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7bd2bfe551 add AccessibleNode boolean attributes, r=yzen, qdot
(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
Flags: needinfo?(surkov.alexander)
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]
Keywords: leave-open
Flags: needinfo?(dora.tokio)
Attached patch patch2: unsigned long attributes (obsolete) — Splinter Review
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.
Attachment #8991886 - Attachment is obsolete: true
Flags: needinfo?(dora.tokio)
Attachment #8993952 - Flags: review?(surkov.alexander)
Attachment #8993952 - Attachment is obsolete: true
Attachment #8993952 - Flags: review?(surkov.alexander)
Attachment #8993953 - Flags: review?(surkov.alexander)
(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.
(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.
(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 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
Attachment #8993953 - Flags: review?(surkov.alexander) → review?(kyle)
Attachment #8992490 - Flags: review?(kyle)
Attachment #8992492 - Flags: review?(kyle)
Attachment #8992490 - Flags: review?(kyle) → review+
Attachment #8992492 - Flags: review?(kyle) → review+
Attachment #8993953 - Flags: review?(kyle) → review+
Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8cf9e6f7d6 add AccessibleNode unsigned long, long & double attributes attributes, r=yzen, qdot
Tokio, patch3 is no longer clearly applied. Could you update it please?
Flags: needinfo?(dora.tokio)
(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.
Attachment #8992490 - Attachment is obsolete: true
Flags: needinfo?(dora.tokio)
Attachment #8994450 - Flags: review?(surkov.alexander)
Attachment #8992492 - Attachment is obsolete: true
Attachment #8994451 - Flags: review?(surkov.alexander)
Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc49c426b6b add AccessibleNode string attributes, r=yzen, qdot
If you were not busy, could you take a look at the patch4 of AccessibleNode attributes?
Flags: needinfo?(surkov.alexander)
Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/020682cafeff add AccessibleNode relation attributes, r=yzen, qdot
Flags: needinfo?(surkov.alexander)
Attachment #8994450 - Flags: review?(surkov.alexander)
(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?
Flags: needinfo?(dora.tokio)
Keywords: leave-open
(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)
Flags: needinfo?(dora.tokio)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #8994451 - Flags: review?(surkov.alexander)
(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.
Assignee: nobody → dora.tokio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: