The default bug view has changed. See this FAQ.

undetermined progressmeters should expose mixed state

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: surkov, Assigned: maxli)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=eitan@monotonous.org][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Comment 1

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

Comment 2

6 years ago
Fixing summary. Hope you don't mind, Alex. :)
Summary: undetermined progressmeters should expose mixed mixed → undetermined progressmeters should expose mixed state
(Reporter)

Comment 3

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

I'm grateful, and thank you :)
Version: unspecified → Trunk

Comment 4

5 years ago
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?
(In reply to Aaron Leventhal from comment #4)
> Perhaps they already do?

Not sure off hand. Did you notice a bug?

Alexander, GFB?
(Reporter)

Comment 6

5 years ago
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
Whiteboard: [good first bug][mentor=eitan@monotonous.org][lang=c++]
(Reporter)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
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?
Attachment #611270 - Flags: feedback?(eitan)
(Reporter)

Comment 9

5 years ago
(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).
(Reporter)

Updated

5 years ago
Assignee: nobody → maxli
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.
Attachment #611270 - Flags: feedback?(eitan) → feedback-
(Assignee)

Comment 11

5 years ago
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?
Attachment #611270 - Attachment is obsolete: true
Attachment #611684 - Flags: feedback?(eitan)
(Reporter)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 612038 [details] [diff] [review]
Patch v3

All tests updated.
Attachment #611684 - Attachment is obsolete: true
Attachment #612038 - Flags: review?(surkov.alexander)
Attachment #611684 - Flags: feedback?(eitan)
(Reporter)

Comment 14

5 years ago
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");
Attachment #612038 - Flags: review?(surkov.alexander)
Attachment #612038 - Flags: review+
Attachment #612038 - Flags: feedback?(eitan)
(Assignee)

Comment 15

5 years ago
Created attachment 612359 [details] [diff] [review]
Patch v4

ARIA value removed
Attachment #612359 - Flags: feedback?(eitan)
(Reporter)

Updated

5 years ago
Attachment #612038 - Attachment is obsolete: true
Attachment #612038 - Flags: feedback?(eitan)
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.
Attachment #612359 - Flags: feedback?(eitan) → feedback+
(Reporter)

Comment 17

5 years ago
(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.
(Reporter)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85db096112e
Flags: in-testsuite+
Target Milestone: --- → mozilla14
http://hg.mozilla.org/mozilla-central/rev/a85db096112e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.