Last Comment Bug 670853 - undetermined progressmeters should expose mixed state
: undetermined progressmeters should expose mixed state
Status: RESOLVED FIXED
[good first bug][mentor=eitan@monoton...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Max Li [:maxli]
:
:
Mentors:
Depends on:
Blocks: html5a11y
  Show dependency treegraph
 
Reported: 2011-07-11 23:16 PDT by alexander :surkov
Modified: 2012-04-05 11:41 PDT (History)
7 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.99 KB, patch)
2012-04-01 06:31 PDT, Max Li [:maxli]
eitan: feedback-
Details | Diff | Splinter Review
Patch v2 (4.29 KB, patch)
2012-04-02 18:40 PDT, Max Li [:maxli]
no flags Details | Diff | Splinter Review
Patch v3 (44.88 KB, patch)
2012-04-03 17:37 PDT, Max Li [:maxli]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch v4 (44.79 KB, patch)
2012-04-04 15:13 PDT, Max Li [:maxli]
eitan: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2011-07-11 23:16:32 PDT
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 2011-07-11 23:17:38 PDT
btw, :indeterminate pseudo-class should be used to implement this state.
Comment 2 James Teh [:Jamie] 2011-07-11 23:20:59 PDT
Fixing summary. Hope you don't mind, Alex. :)
Comment 3 alexander :surkov 2011-07-12 01:13:51 PDT
(In reply to comment #2)
> Fixing summary. Hope you don't mind, Alex. :)

I'm grateful, and thank you :)
Comment 4 Aaron Leventhal 2012-03-30 08:49:17 PDT
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] 2012-03-30 09:02:01 PDT
(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 2012-03-30 09:07:32 PDT
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 2012-03-30 09:08:24 PDT
(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] 2012-04-01 06:31:38 PDT
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?
Comment 9 alexander :surkov 2012-04-01 20:45:39 PDT
(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] 2012-04-02 01:17:08 PDT
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] 2012-04-02 18:40:01 PDT
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 (and thus many tests have to be updated to include role.js)? Is this the right approach?
Comment 12 alexander :surkov 2012-04-02 21:08:27 PDT
(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] 2012-04-03 17:37:08 PDT
Created attachment 612038 [details] [diff] [review]
Patch v3

All tests updated.
Comment 14 alexander :surkov 2012-04-03 22:08:39 PDT
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] 2012-04-04 15:13:57 PDT
Created attachment 612359 [details] [diff] [review]
Patch v4

ARIA value removed
Comment 16 Eitan Isaacson [:eeejay] 2012-04-05 02:00:39 PDT
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 2012-04-05 02:21:08 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.