The default bug view has changed. See this FAQ.

ARIA undetermined progressmeters should expose mixed state

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: surkov, Assigned: maxli)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
similar to bug 670853 but for ARIA progressbars. If aria-value or aria-valuenow is not specified on ARIA progressbar then expose MIXED state.

1) introduce eIndeterminateIfNoValue nsStateMapEntry (similar to eReadonlyUntilEditable from bug 733382)
2) fix "S_FALSE from IAccessible::get_accValue instead of S_OK" issue as Aaron pointed in bug 670853
3) add a test to states/test_aria.html

since nsStateMapEntry is not greatly extensible for these needs then it's not good first bug. We need to figure out how to fix that.
(Reporter)

Comment 1

5 years ago
(In reply to alexander :surkov from comment #0)
> similar to bug 670853 but for ARIA progressbars. If aria-value or
> aria-valuenow is not specified on ARIA progressbar then expose MIXED state.

misspelling: aria-value -> aria-valuetext

Comment 2

5 years ago
I thought that returning S_FALSE was the de facto standard way. 

Interestingly, here's what it says in the original version of the ARIA implementation guide:

"If the value is not set on a control that requires value, then current value should return an error. This is a valid condition for progressbar, where the current value could be indeterminate."

True, Firefox uses S_FALSE, which is not technically an error value. Why not standardize on using S_FALSE which already works? 

Whatever is done should be reflected in the implementation guide so that other browsers can follow suit.

Updated

5 years ago
Whiteboard: [UIAG doc needed]
(Reporter)

Comment 3

5 years ago
(In reply to Aaron Leventhal from comment #2)
> I thought that returning S_FALSE was the de facto standard way. 

Oh, right, that should be MSAA requirement for empty values.

> Interestingly, here's what it says in the original version of the ARIA
> implementation guide:
> 
> "If the value is not set on a control that requires value, then current
> value should return an error. This is a valid condition for progressbar,
> where the current value could be indeterminate."
> 
> True, Firefox uses S_FALSE, which is not technically an error value. Why not
> standardize on using S_FALSE which already works? 
> 
> Whatever is done should be reflected in the implementation guide so that
> other browsers can follow suit.

Agree.
(Reporter)

Updated

5 years ago
Depends on: 741398
(Reporter)

Comment 4

5 years ago
1) introduce eIndeterminateIfNoValue EStateRule and implement it in aria::MapToState: if aria-valuenow and aria-valuetext are not specified then expose MIXED state (see ARIAStateMap.h)
2) apply eIndeterminateIfNoValue to "progressbar" role (gWAIRoleMap in nsARIAMap.cpp)
3) add a test to states/test_aria.html
<div role="progressbar"></div> - mixed
<div role="progressbar" aria-valuenow="1"></div> - no mixed
<div role="progressbar" aria-text="value"></div> - no mixed
Whiteboard: [UIAG doc needed] → [good first bug][mentor=eitan@monotonous.org][lang=c++][UIAG doc needed]
(Assignee)

Comment 5

5 years ago
Created attachment 614256 [details] [diff] [review]
Patch (v1)
Assignee: nobody → maxli
Attachment #614256 - Flags: review?(eitan)
Comment on attachment 614256 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/base/ARIAStateMap.cpp
@@ +294,5 @@
> +        *aState |= states::MIXED;
> +
> +      return true;
> +    }
> +

This kind of breaks the MapTokenType/MapEnumType pattern in previous cases. You could modify MapEnumType to have a -1 (ATTR_MISSING) case, and add a field to EnumTypeData for a state to add when the attribute is missing.

Alex, do you think it is worth it, or should we just go with these 3 lines?

::: accessible/tests/mochitest/states/test_aria.html
@@ +164,5 @@
>        testStates("aria_vslider", 0, EXT_STATE_VERTICAL);
> +      
> +      // indeterminate ARIA progressbars (no aria-valuenow or aria-valuetext attribute)
> +      // should expose mixed state
> +      

Remove extra newline
> ::: accessible/src/base/ARIAStateMap.cpp
> @@ +294,5 @@
> > +        *aState |= states::MIXED;
> > +
> > +      return true;
> > +    }
> > +
> 
> This kind of breaks the MapTokenType/MapEnumType pattern in previous cases.

you can call it a pattern if you like, but really its just that a lot of the cases are the same, and so can be handled the same way.

> You could modify MapEnumType to have a -1 (ATTR_MISSING) case, and add a
> field to EnumTypeData for a state to add when the attribute is missing.

please no, I guess you never had to deal with it, but that's moving toward the old nsStateMap stuff we got rid of in bug 741398 which was a horrible confusing mess.
The point of using a switch here is that we can do things like this, we already have one similar case, and if adding another makes sense then we should just do it.

> Alex, do you think it is worth it, or should we just go with these 3 lines?

I think this patch is exactly what  he suggested in comment 4, but maybe he wants to suprise me and try and convince me what you propose is reasonable.
(Assignee)

Comment 8

5 years ago
Created attachment 614596 [details] [diff] [review]
Patch v2

(In reply to Eitan Isaacson [:eeejay] from comment #6)

> This kind of breaks the MapTokenType/MapEnumType pattern in previous cases.
> You could modify MapEnumType to have a -1 (ATTR_MISSING) case, and add a
> field to EnumTypeData for a state to add when the attribute is missing.

Perhaps I'm missing something, but how would this work in this case (since both attributes must be not present for the mixed state to be exposed)?

Anyways, I removed the newline.
Attachment #614256 - Attachment is obsolete: true
Attachment #614256 - Flags: review?(eitan)
Attachment #614596 - Flags: review?(eitan)
Comment on attachment 614596 [details] [diff] [review]
Patch v2

Looks good. Thanks Trevor for the context.
Attachment #614596 - Flags: review?(eitan) → review+
(Reporter)

Updated

5 years ago
Attachment #614596 - Flags: feedback+
(Reporter)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a11fba8ea615
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/a11fba8ea615
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.