529289 - We dont expose new aria role "scrollbar" and property aria-orientation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

There is a new kid on the block:
http://www.w3.org/WAI/PF/aria/roles#scrollbar

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

ATK_STATE_HORIZONTAL
ATK_STATE_VERTICAL

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: early WIP

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1

Note: this patch adds the capability to specify a defined default state for an enumerated aria state list.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Filed spin off bug 530792 to make sure we define defaults where needed.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 414277 [details] [diff] [review]
patch 1

r=me. Thanks!

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 414277 [details] [diff] [review]
patch 1

looks good.

>   // eARIAMultiSelectable
>   nsStateMapEntry(&nsAccessibilityAtoms::aria_multiselectable, kBoolType, 0,
>                   nsIAccessibleStates::STATE_MULTISELECTABLE | nsIAccessibleStates::STATE_EXTSELECTABLE, 0),
> 
>+  // eARIAOrientation
>+  nsStateMapEntry(&nsAccessibilityAtoms::aria_orientation, PR_TRUE,
>+                  "vertical", 0, nsIAccessibleStates::EXT_STATE_VERTICAL,
>+                  "horizontal", 0, nsIAccessibleStates::EXT_STATE_HORIZONTAL),
>+


>+nsStateMapEntry::nsStateMapEntry(nsIAtom **aAttrName,
>+                                 PRBool aDefinedIfAbsent,
>+                                 const char *aValue1,
>+                                 PRUint32 aState1, PRUint32 aExtraState1,
>+                                 const char *aValue2,
>+                                 PRUint32 aState2, PRUint32 aExtraState2,
>+                                 const char *aValue3,
>+                                 PRUint32 aState3, PRUint32 aExtraState3) :
>+  attributeName(aAttrName), isToken(PR_TRUE), permanentState(0),
>+  value1(aValue1), state1(aState1), extraState1(aExtraState1),
>+  value2(aValue2), state2(aState2), extraState2(aExtraState2),
>+  value3(aValue3), state3(aState3), extraState3(aExtraState3),
>+  defaultState(0), defaultExtraState(0), definedIfAbsent(aDefinedIfAbsent)
>+{
>+  if (aDefinedIfAbsent) {
>+    defaultState = aState1;
>+    defaultExtraState = aExtraState1;
>+  }
>+}

>+   * Used for ARIA attributes having enumerated token based values where
>+   * the first value can be defined as a default value according to:
>+   * @param aDefinedIfAbsent [in] define first state by default?
>+   */
>+  nsStateMapEntry(nsIAtom **aAttrName,
>+                  PRBool aDefineDefaultIfAbsent,
>+                  const char *aValue1, PRUint32 aState1, PRUint32 aExtraState1,
>+                  const char *aValue2, PRUint32 aState2, PRUint32 aExtraState2,
>+                  const char *aValue3 = 0, PRUint32 aState3 = 0,
>+                  PRUint32 aExtraState3 = 0);

I thought to use aDefaultState and aDefaultExtraState instead of aDefineDefaultIfAbsent. It would make clear an implementation and probably declaration of the state map. Then I thought your approach might be a bit more friendly than this one because I see all values and say the first entry is used as default. That's not bad. But the argument name and its type aren't good. When I see 

nsStateMapEntry(&nsAccessibilityAtoms::aria_orientation, PR_TRUE,
                  "vertical", 0, nsIAccessibleStates::EXT_STATE_VERTICAL,
                  "horizontal", 0, nsIAccessibleStates::EXT_STATE_HORIZONTAL),

nothing tells me "vertical" states are used as default one. Probably you should change the atttribute name on more descriptive version to make nsStateMapEntry constructor declaration/definition clearer and use enum type instead of boolean like

nsStateMapEntry(&nsAccessibilityAtoms::aria_orientation, kUseTheFirstStatesAsDefaultIfAttrIsAbsent,
                  "vertical", 0, nsIAccessibleStates::EXT_STATE_VERTICAL,
                  "horizontal", 0, nsIAccessibleStates::EXT_STATE_HORIZONTAL),

(of course a bit shorter version :) )

and probably the same name for argument name.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 414277 [details] [diff] [review]
patch 1

>   /**
>-   * Used for ARIA attributes having enumerated values.
>+   * Used for ARIA attributes having enumerated values
>    */

Have you used this dot for something else? ;)

>   /**
>+   * Used for ARIA attributes having enumerated token based values where
>+   * the first value can be defined as a default value according to:
>+   * @param aDefinedIfAbsent [in] define first state by default?

I'm not sure it's really good when the method description needs to refer to arguments description. Usually empty line is inserted between method description and its arguments descriptions.


>diff --git a/accessible/tests/mochitest/test_events_valuechange.html b/accessible/tests/mochitest/test_events_valuechange.html
>--- a/accessible/tests/mochitest/test_events_valuechange.html
>+++ b/accessible/tests/mochitest/test_events_valuechange.html
>@@ -61,49 +61,60 @@
>     }
> 
>     function doTests()
>     {
>       // Test initial values
>       testValue("slider_vn", "5", 5, 0, 1000, 0);
>       testValue("slider_vnvt", "plain", 0, 0, 5, 0);
>       testValue("slider_vt", "hi", 0, 0, 3, 0);
>+      testValue("scrollbar", "5", 5, 0, 1000, 0);
> 
>       // Test value change events
>       gQueue = new eventQueue(nsIAccessibleEvent.EVENT_VALUE_CHANGE);
> 
>       gQueue.push(new changeValue("slider_vn", "6", undefined));
>       gQueue.push(new changeValue("slider_vt", undefined, "hey!"));
>       gQueue.push(new changeValue("slider_vnvt", "3", "sweet"));
>+      gQueue.push(new changeValue("scrollbar", "6", undefined));
> 
>       gQueue.invoke(); // Will call SimpleTest.finish();
>     }
> 
>     SimpleTest.waitForExplicitFinish();
>     addA11yLoadEvent(doTests);
>   </script>
> </head>
> 
> <body>
> 
>   <a target="_blank"
>      href="https://bugzilla.mozilla.org/show_bug.cgi?id=478032"
>      title=" Fire delayed value changed event for aria-valuetext changes">
>     Mozilla Bug 478032
>   </a>
>+  <a target="_blank"
>+    href="https://bugzilla.mozilla.org/show_bug.cgi?id=529289"
>+    title="We dont expose new aria role "scrollbar" and property aria-orientation">

oops, can we use nested quotation marks? I would prefer to use apostrophe between quotation marks.

>+
>+  <!-- ARIA scrollbar -->
>+  <div id="scrollbar" role="scrollbar" aria-valuenow="5"
>+    aria-valuemin="0" aria-valuemax="1000">slider</div>

line up arguments

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 414277 [details] [diff] [review]
patch 1

I think it's not necessary to look at next version of the patch, r=me with those fixed.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks, fixed locally. I'll wait for an open trunk.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/76d1ee7cbb38