Closed Bug 529289 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: davidb, Assigned: davidb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → bolterbugz
Blocks: aria
Summary: We don new aria role "scrollbar" → We dont expose new aria role "scrollbar" and property aria-orientation
ATK_STATE_HORIZONTAL
ATK_STATE_VERTICAL
Attached patch early WIP (obsolete) — Splinter Review
Attached patch patch 1Splinter Review
Note: this patch adds the capability to specify a defined default state for an enumerated aria state list.
Attachment #412865 - Attachment is obsolete: true
Attachment #414277 - Flags: review?(surkov.alexander)
Attachment #414277 - Flags: review?(marco.zehe)
Filed spin off bug 530792 to make sure we define defaults where needed.
Status: NEW → ASSIGNED
Comment on attachment 414277 [details] [diff] [review]
patch 1

r=me. Thanks!
Attachment #414277 - Flags: review?(marco.zehe) → review+
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 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 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.
Attachment #414277 - Flags: review?(surkov.alexander) → review+
Thanks, fixed locally. I'll wait for an open trunk.
http://hg.mozilla.org/mozilla-central/rev/76d1ee7cbb38
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.