Closed
Bug 1468110
Opened 7 years ago
Closed 7 years ago
Add attributes to AccessibleNode
Categories
(Core :: Disability Access APIs, defect, P2)
Core
Disability Access APIs
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.
Assignee | ||
Comment 1•7 years ago
|
||
(In reply to dora.tokio from comment #0)
> Steps to reproduce:
It's a mistake.
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8985565 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•7 years ago
|
Attachment #8985565 -
Attachment description: patch → part1: boolean attributes
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
(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
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8985565 -
Attachment is obsolete: true
Attachment #8985565 -
Flags: review?(surkov.alexander)
Attachment #8985565 -
Flags: review?(bugs)
Attachment #8985785 -
Flags: review?(surkov.alexander)
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
(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?
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
(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
Assignee | ||
Comment 17•7 years ago
|
||
Attachment #8987680 -
Attachment is obsolete: true
Attachment #8988999 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8989000 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8989002 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 20•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8988999 -
Flags: review?(surkov.alexander) → review?(yzenevich)
Assignee | ||
Updated•7 years ago
|
Attachment #8989000 -
Flags: review?(surkov.alexander) → review?(yzenevich)
Assignee | ||
Updated•7 years ago
|
Attachment #8989002 -
Flags: review?(surkov.alexander) → review?(yzenevich)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
It makes sense to ni? Olli on comment #20 to check if he'ok with that
Flags: needinfo?(bugs)
Comment 23•7 years ago
|
||
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+
Assignee | ||
Comment 24•7 years ago
|
||
(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•7 years ago
|
||
Why any Hashtable usage? Hashtables are slow. Sure, they are O(1), but with quite high constant time.
Flags: needinfo?(bugs)
Assignee | ||
Comment 26•7 years ago
|
||
(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)
Assignee | ||
Comment 27•7 years ago
|
||
Attachment #8989000 -
Attachment is obsolete: true
Attachment #8989000 -
Flags: review?(yzenevich)
Attachment #8989185 -
Flags: review?(yzenevich)
Assignee | ||
Comment 28•7 years ago
|
||
Attachment #8989002 -
Attachment is obsolete: true
Attachment #8989002 -
Flags: review?(yzenevich)
Attachment #8989186 -
Flags: review?(yzenevich)
Assignee | ||
Comment 29•7 years ago
|
||
Attachment #8989039 -
Attachment is obsolete: true
Attachment #8989187 -
Flags: review?(yzenevich)
Comment 30•7 years ago
|
||
(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.
Assignee | ||
Comment 31•7 years ago
|
||
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 32•7 years ago
|
||
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 33•7 years ago
|
||
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)
Assignee | ||
Comment 34•7 years ago
|
||
(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•7 years ago
|
||
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).
Assignee | ||
Comment 36•7 years ago
|
||
Attachment #8989172 -
Attachment is obsolete: true
Attachment #8990258 -
Flags: review?(yzenevich)
Assignee | ||
Comment 37•7 years ago
|
||
Attachment #8989185 -
Attachment is obsolete: true
Attachment #8990259 -
Flags: review?(yzenevich)
Assignee | ||
Comment 38•7 years ago
|
||
Attachment #8989186 -
Attachment is obsolete: true
Attachment #8989186 -
Flags: review?(yzenevich)
Attachment #8990260 -
Flags: review?(yzenevich)
Assignee | ||
Comment 39•7 years ago
|
||
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)
Assignee | ||
Comment 40•7 years ago
|
||
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 41•7 years ago
|
||
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 42•7 years ago
|
||
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•7 years ago
|
||
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 44•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 46•7 years ago
|
||
(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.
Assignee | ||
Comment 47•7 years ago
|
||
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)
Comment 48•7 years ago
|
||
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)
Comment 49•7 years ago
|
||
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•7 years ago
|
||
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 51•7 years ago
|
||
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+
Assignee | ||
Comment 52•7 years ago
|
||
Attachment #8990258 -
Attachment is obsolete: true
Attachment #8991885 -
Flags: review?(yzenevich)
Assignee | ||
Comment 53•7 years ago
|
||
Attachment #8990259 -
Attachment is obsolete: true
Attachment #8991886 -
Flags: review?(yzenevich)
Assignee | ||
Comment 54•7 years ago
|
||
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 55•7 years ago
|
||
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 56•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8991889 -
Flags: review?(yzenevich) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 57•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 59•7 years ago
|
||
(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.
Assignee | ||
Comment 60•7 years ago
|
||
fix some indentation
Attachment #8991889 -
Attachment is obsolete: true
Attachment #8992490 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 61•7 years ago
|
||
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)
Assignee | ||
Comment 62•7 years ago
|
||
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)
Comment 63•7 years ago
|
||
I don't think we have any choice, so I think it's fine to just rearrange. Alex?
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
Comment 64•7 years ago
|
||
(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 65•7 years ago
|
||
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 66•7 years ago
|
||
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 67•7 years ago
|
||
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+
Comment 68•7 years ago
|
||
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)
Updated•7 years ago
|
Keywords: checkin-needed
Comment 69•7 years ago
|
||
Alex? Is that for Olli?
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
Comment 70•7 years ago
|
||
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 71•7 years ago
|
||
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 72•7 years ago
|
||
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+
Comment 73•7 years ago
|
||
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7bd2bfe551
add AccessibleNode boolean attributes, r=yzen, qdot
Comment 74•7 years ago
|
||
(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)
Comment 75•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(dora.tokio)
Comment 76•7 years ago
|
||
bugherder |
Assignee | ||
Comment 77•7 years ago
|
||
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)
Assignee | ||
Comment 78•7 years ago
|
||
Attachment #8993952 -
Attachment is obsolete: true
Attachment #8993952 -
Flags: review?(surkov.alexander)
Attachment #8993953 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 79•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8992490 -
Flags: review?(kyle)
Updated•7 years ago
|
Attachment #8992492 -
Flags: review?(kyle)
Updated•7 years ago
|
Attachment #8992490 -
Flags: review?(kyle) → review+
Updated•7 years ago
|
Attachment #8992492 -
Flags: review?(kyle) → review+
Updated•7 years ago
|
Attachment #8993953 -
Flags: review?(kyle) → review+
Comment 83•7 years ago
|
||
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
Comment 84•7 years ago
|
||
Tokio, patch3 is no longer clearly applied. Could you update it please?
Flags: needinfo?(dora.tokio)
Comment 85•7 years ago
|
||
bugherder |
Assignee | ||
Comment 86•7 years ago
|
||
(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)
Assignee | ||
Comment 87•7 years ago
|
||
Attachment #8992492 -
Attachment is obsolete: true
Attachment #8994451 -
Flags: review?(surkov.alexander)
Comment 88•7 years ago
|
||
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc49c426b6b
add AccessibleNode string attributes, r=yzen, qdot
![]() |
||
Comment 89•7 years ago
|
||
bugherder |
Assignee | ||
Comment 90•7 years ago
|
||
If you were not busy, could you take a look at the patch4 of AccessibleNode attributes?
Flags: needinfo?(surkov.alexander)
Comment 91•7 years ago
|
||
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/020682cafeff
add AccessibleNode relation attributes, r=yzen, qdot
Updated•7 years ago
|
Flags: needinfo?(surkov.alexander)
Attachment #8994450 -
Flags: review?(surkov.alexander)
Comment 92•7 years ago
|
||
(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
Assignee | ||
Comment 93•7 years ago
|
||
(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)
Comment 94•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•7 years ago
|
Attachment #8994451 -
Flags: review?(surkov.alexander)
Comment 95•7 years ago
|
||
(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.
Updated•7 years ago
|
Assignee: nobody → dora.tokio
You need to log in
before you can comment on or make changes to this bug.
Description
•