Last Comment Bug 740851 - ARIA undetermined progressmeters should expose mixed state
: ARIA 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)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on: 741398
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-03-30 09:33 PDT by alexander :surkov
Modified: 2012-04-14 06:42 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) (5.23 KB, patch)
2012-04-11 19:33 PDT, Max Li [:maxli]
no flags Details | Diff | Review
Patch v2 (5.14 KB, patch)
2012-04-12 16:01 PDT, Max Li [:maxli]
eitan: review+
surkov.alexander: feedback+
Details | Diff | Review

Description alexander :surkov 2012-03-30 09:33:02 PDT
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.
Comment 1 alexander :surkov 2012-03-30 09:34:35 PDT
(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 Aaron Leventhal 2012-03-30 11:17:22 PDT
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.
Comment 3 alexander :surkov 2012-03-30 21:03:59 PDT
(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.
Comment 4 alexander :surkov 2012-04-06 20:20:03 PDT
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
Comment 5 Max Li [:maxli] 2012-04-11 19:33:54 PDT
Created attachment 614256 [details] [diff] [review]
Patch (v1)
Comment 6 Eitan Isaacson [:eeejay] 2012-04-12 10:03:38 PDT
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
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-12 15:51:50 PDT
> ::: 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.
Comment 8 Max Li [:maxli] 2012-04-12 16:01:54 PDT
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.
Comment 9 Eitan Isaacson [:eeejay] 2012-04-12 16:05:59 PDT
Comment on attachment 614596 [details] [diff] [review]
Patch v2

Looks good. Thanks Trevor for the context.
Comment 11 Marco Bonardo [::mak] 2012-04-14 06:42:37 PDT
https://hg.mozilla.org/mozilla-central/rev/a11fba8ea615

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