670853 - undetermined progressmeters should expose mixed state

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

After chat with Jamie we agreed that it's worth to use mixed state for undetermined progressmeters (both HTML and XUL) to allow AT distinguish determined vs undetermined progressmeters.

Any thoughts?

Comment 1

[alexander :surkov (:asurkov)]

btw, :indeterminate pseudo-class should be used to implement this state.

Comment 2

[James Teh [:Jamie]]

Fixing summary. Hope you don't mind, Alex. :)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Fixing summary. Hope you don't mind, Alex. :)

I'm grateful, and thank you :)

Comment 4

[Aaron Leventhal]

For ARIA indeterminate progressbars (where no aria-valuetext or aria-valuenow is set), Firefox returns S_FALSE from IAccessible::get_accValue instead of S_OK. XUL/HTML progressbars should probably do the same thing for consistency. Perhaps they already do?

Comment 5

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

(In reply to Aaron Leventhal from comment #4)
> Perhaps they already do?

Not sure off hand. Did you notice a bug?

Alexander, GFB?

Comment 6

[alexander :surkov (:asurkov)]

1) implement NativeState (defined at nsAccessible) for ProgressMeterAccessible (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsFormControlAccessible.h#65)
2) check if no aria-valuenow or value attribute (see ProgressMeterAccessible<Max>::GetCurrentValue) then expose state::MIXED
3) add states/test_controls.html mochitest (see http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/states/test_inputs.html?force=1 for idea)

and add a test for <progress></progress> element

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Aaron Leventhal from comment #4)
> For ARIA indeterminate progressbars (where no aria-valuetext or
> aria-valuenow is set), Firefox returns S_FALSE from
> IAccessible::get_accValue instead of S_OK. XUL/HTML progressbars should
> probably do the same thing for consistency. Perhaps they already do?

I'll file a separate bug for ARIA

Comment 8

[Max Li [:maxli]]

Attachment: Patch v1

Everything works except one of the mochitests fails because it checks for a mixed element being checkable. Since that doesn't appear to make sense if we add this patch, should I remove that from states.js?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Max Li from comment #8)
> Created attachment 611270 [details] [diff] [review]
> Patch v1
> 
> Everything works except one of the mochitests fails because it checks for a
> mixed element being checkable. Since that doesn't appear to make sense if we
> add this patch, should I remove that from states.js?

please add progress bar role check instead (i.e. do not check checkable state if role is progress bar).

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 611270 [details] [diff] [review]
Patch v1

Review of attachment 611270 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I would just add some comments explaining what is happening and why. And as Alex says the "MIXED is CHECKABLE" test should also check for appropriate roles.

::: accessible/src/base/nsFormControlAccessible.cpp
@@ +90,5 @@
> +  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, attrValue);
> +
> +  nsAutoString attrARIAValue;
> +  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_valuenow, attrARIAValue);
> +

I think a comment here explaining the rationale makes sense.

::: accessible/tests/mochitest/states/test_controls.html
@@ +14,5 @@
> +          src="../states.js"></script>
> +
> +  <script type="application/javascript">
> +  function doTest()
> +  {

Human-readable comments before each test of what state we are expecting, and why.

Comment 11

[Max Li [:maxli]]

Attachment: Patch v2

I added the comments, but I wasn't sure how to go about checking for the role when testing the state. If I add the check in states.js, then doesn't states.js depend on roles.js (and thus many tests have to be updated to include role.js)? Is this the right approach?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Max Li from comment #11)
> Created attachment 611684 [details] [diff] [review]
> Patch v2
> 
> I added the comments, but I wasn't sure how to go about checking for the
> role when testing the state. If I add the check in states.js, then doesn't
> states.js depend on roles.js

yes

> (and thus many tests have to be updated to
> include role.js)?

maybe not many of them

> Is this the right approach?

well, we do that, maybe it's not super nice but it's hard to keep things separately.

Comment 13

[Max Li [:maxli]]

Attachment: Patch v3

All tests updated.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 612038 [details] [diff] [review]
Patch v3

Review of attachment 612038 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsFormControlAccessible.cpp
@@ +90,5 @@
> +  nsAutoString attrValue;
> +  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, attrValue);
> +
> +  nsAutoString attrARIAValue;
> +  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_valuenow, attrARIAValue);

you know, just remove this ARIA thing (sorry for saying before to do that). ARIA part should take care about accessible state (bug 740851). In either case NativeState() is about markup, not about ARIA.

::: accessible/tests/mochitest/states/test_controls.html
@@ +22,5 @@
> +    testStates("progress", STATE_MIXED);
> +    // Determined progressbar (has value): shouldn't have mixed state
> +    testStates("progress2", 0, 0, STATE_MIXED);
> +    // Determined progressbar (has aria-value): shouldn't have mixed state
> +    testStates("progress3", 0, 0, STATE_MIXED);

please comment out "progress3" test and add todo like
todo(false, "we should respect ARIA");

Comment 15

[Max Li [:maxli]]

Attachment: Patch v4

ARIA value removed

Comment 16

[Eitan Isaacson [:eeejay]]

Comment on attachment 612359 [details] [diff] [review]
Patch v4

Looks right. It is unfortunate that for 2 lines you need to pull in role.js every time you use states.js. Maybe the simple role stuff could go in common.js? But that is not related to this bug.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Looks right. It is unfortunate that for 2 lines you need to pull in role.js
> every time you use states.js. Maybe the simple role stuff could go in
> common.js? 

if js would have #include option :) then I would include role.js and states.js into common.js.

Comment 18

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a85db096112e

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a85db096112e