make ARIA state map extensible

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 611462 [details] [diff] [review]
patch
Attachment #611462 - Flags: review?(trev.saunders)
(Assignee)

Updated

6 years ago
Blocks: 740851
Comment on attachment 611462 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/ARIAStateMap.cpp b/accessible/src/base/ARIAStateMap.cpp
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/base/ARIAStateMap.cpp
>@@ -0,0 +1,356 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public

editor lines :)

>+  nsIAtom* mAttrName;

any reason not to make all these things const? afaict you never modify one of these after making it.

>+
>+  // States if the attribute value is matched to the enum value.
>+  const char* mValue1;

I wonder how using atoms instead would effect performance, I suspect they'd atleast make things nicer.

>+static void MapEnumType(dom::Element* aElement, PRUint64* aState,
>+                        const EnumTypeData& aData);

if it were me I'd probably keep the function prototypes together if I could.

>+  nsIAtom* mAttrName;

make these const too?

>+aria::MapToState(EStateRule aRule, dom::Element* aElement, PRUint64* aState)


I think I might like having the aria:: here for clarity, but I don't think you should need it.

I'm not a huge fan of this function returning true or false, afaik we check in two places one being UniversalStatesFor() which is sort of reasonable, but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy would it be to assert we were not called with a crazy  rule, and check type is not eNone at the call site?

>+{
>+  switch (aRule) {
>+    case eARIAAutoComplete:
>+    {

I can't decide if I like this switch because its explicit or want to shorten it.

>+      static EnumTypeData data(
>+        nsGkAtoms::aria_autocomplete,
>+        "inline", states::SUPPORTS_AUTOCOMPLETION,
>+        "list", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION,
>+        "both", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION);

static const?

>+void

the prototype is static, for C style functions I think you should probably make the definition static as well although I'm not sure exactly what the spec says here.

>+      if (aData.mState1)
>+        *aState |= aData.mState1;

I think just *aState |= aData.mState1 would be clearer.

>+void

static here as well.

>+++ b/accessible/src/base/ARIAStateMap.h
>@@ -0,0 +1,60 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public

editor lines again.

>+#include <prtypes.h>

<prtypes.h> is weird any reason?

> #include "nsIAccessibleRole.h"

still needed?

> #include "nsIContent.h"

same

>+  if (aria::MapToState(mRoleMapEntry->attributeMap1, element, aState) &&
>+      aria::MapToState(mRoleMapEntry->attributeMap2, element, aState))
>+    aria::MapToState(mRoleMapEntry->attributeMap3, element, aState);
> }

that's better than it was but still funny.

> nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> {
>   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> 
>-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
>+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);

are you sure that's valid? I'm fairly sure for html where we have real objects, but doesn't xul use a lot of funny xml / js stuffs?
(Assignee)

Comment 2

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> >+  nsIAtom* mAttrName;
> 
> any reason not to make all these things const? afaict you never modify one
> of these after making it.

which const, object, pointer or both? they are sort of internal objects without any wide usage so what is the best practice for those? i.e should we really care or the point is follow a good style?

> >+  // States if the attribute value is matched to the enum value.
> >+  const char* mValue1;
> 
> I wonder how using atoms instead would effect performance, I suspect they'd
> atleast make things nicer.

ok, done

> >+static void MapEnumType(dom::Element* aElement, PRUint64* aState,
> >+                        const EnumTypeData& aData);
> 
> if it were me I'd probably keep the function prototypes together if I could.

ok, if you like

> >+aria::MapToState(EStateRule aRule, dom::Element* aElement, PRUint64* aState)
> 
> 
> I think I might like having the aria:: here for clarity, but I don't think
> you should need it.

iirc I've got linker error.

> I'm not a huge fan of this function returning true or false, afaik we check
> in two places one being UniversalStatesFor() which is sort of reasonable,
> but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy
> would it be to assert we were not called with a crazy  rule, and check type
> is not eNone at the call site?

that makes sense I'll file follow ups on this.

> >+{
> >+  switch (aRule) {
> >+    case eARIAAutoComplete:
> >+    {
> 
> I can't decide if I like this switch because its explicit or want to shorten
> it.

please say more, because switch was a point of the reorg.

> >+      static EnumTypeData data(
> >+        nsGkAtoms::aria_autocomplete,
> >+        "inline", states::SUPPORTS_AUTOCOMPLETION,
> >+        "list", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION,
> >+        "both", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION);
> 
> static const?

ah, keeping const the object, const its members. What's is the best practice again?

> >+void
> 
> the prototype is static, for C style functions I think you should probably
> make the definition static as well although I'm not sure exactly what the
> spec says here.

If you like I can add it but we never did it before.

> > nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> > {
> >   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> > 
> >-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
> >+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);
> 
> are you sure that's valid? I'm fairly sure for html where we have real
> objects, but doesn't xul use a lot of funny xml / js stuffs?

I'm not sure I follow. This allows to use aria-autocomplete of XUL textboxes. What's wrong?
(Assignee)

Comment 3

6 years ago
Created attachment 612149 [details] [diff] [review]
patch2
Attachment #611462 - Attachment is obsolete: true
Attachment #611462 - Flags: review?(trev.saunders)
Attachment #612149 - Flags: review?(trev.saunders)
> > any reason not to make all these things const? afaict you never modify one
> > of these after making it.
> 
> which const, object, pointer or both? they are sort of internal objects
> without any wide usage so what is the best practice for those? i.e should we
> really care or the point is follow a good style?

I've never heard of a best practice.  Personally I tend to prefer to tell the compiler as much as I can unless I'm trying to play a trick.

> > if it were me I'd probably keep the function prototypes together if I could.
> 
> ok, if you like

just sort of usual thing I see people do when they write things like this.

> > I think I might like having the aria:: here for clarity, but I don't think
> > you should need it.
> 
> iirc I've got linker error.

ok, interesting

> > I'm not a huge fan of this function returning true or false, afaik we check
> > in two places one being UniversalStatesFor() which is sort of reasonable,
> > but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy
> > would it be to assert we were not called with a crazy  rule, and check type
> > is not eNone at the call site?
> 
> that makes sense I'll file follow ups on this.

sounds good.

> 
> > >+{
> > >+  switch (aRule) {
> > >+    case eARIAAutoComplete:
> > >+    {
> > 
> > I can't decide if I like this switch because its explicit or want to shorten
> > it.
> 
> please say more, because switch was a point of the reorg.

well, its great tin that I think its a bit clearer how things work, and for flexability.  My only sort of issue was that its pretty long, and maybe feels a little  repeditive.

> > static const?
> 
> ah, keeping const the object, const its members. What's is the best practice
> again?

same answer.

> 
> > >+void
> > 
> > the prototype is static, for C style functions I think you should probably
> > make the definition static as well although I'm not sure exactly what the
> > spec says here.
> 
> If you like I can add it but we never did it before.

I don't think we have that many static C functions with prototypes do we?  I also sort of thought gcc didn't like your way, but maybe that's only in C not C++.

> > > nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> > > {
> > >   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> > > 
> > >-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
> > >+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);
> > 
> > are you sure that's valid? I'm fairly sure for html where we have real
> > objects, but doesn't xul use a lot of funny xml / js stuffs?
> 
> I'm not sure I follow. This allows to use aria-autocomplete of XUL
> textboxes. What's wrong?

I was concerned that the nsIContent* might not be a dom::Element, but I guess if your sure I'll believe.
(Assignee)

Comment 5

6 years ago
Created attachment 612476 [details] [diff] [review]
patch

adding const stuff, it seems it goes with google style guide http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Use_of_const
Attachment #612149 - Attachment is obsolete: true
Attachment #612149 - Flags: review?(trev.saunders)
Attachment #612476 - Flags: review?(trev.saunders)
Comment on attachment 612476 [details] [diff] [review]
patch

\O/
Attachment #612476 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2d53ab4582
Target Milestone: --- → mozilla14
Clever :)
https://hg.mozilla.org/mozilla-central/rev/fc2d53ab4582
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 762876
You need to log in before you can comment on or make changes to this bug.