do not fail when nsIAccessibleValue accessible can't get value from markup

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug, {access, verified1.9.1})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #375129 - Flags: review?(marco.zehe)
Attachment #375129 - Flags: review?(david.bolter)
Attachment #375129 - Attachment is obsolete: true
Attachment #375129 - Flags: review?(marco.zehe)
Attachment #375129 - Flags: review?(david.bolter)
Comment on attachment 375129 [details] [diff] [review]
patch

new patch is coming up
Summary: use zero value when there is no or wrong @value of progressmeter → do not fail when nsIAccessibleValue accessible can't get value from markup
Posted patch patch2 (obsolete) — Splinter Review
1) do not fail when we can't get value from markup
2) implement nsIAccessibleValue for scale
Attachment #375162 - Flags: review?(marco.zehe)
Comment on attachment 375162 [details] [diff] [review]
patch2

This patch is missing value.js. Is this basically doing what the testValue function does that's being removed from test_value.xul?
(In reply to comment #4)
> (From update of attachment 375162 [details] [diff] [review])
> This patch is missing value.js. Is this basically doing what the testValue
> function does that's being removed from test_value.xul?

Right, Marco. Thank you for the catch.
Posted patch patch3Splinter Review
Attachment #375162 - Attachment is obsolete: true
Attachment #375325 - Flags: review?(marco.zehe)
Attachment #375325 - Flags: review?(david.bolter)
Attachment #375162 - Flags: review?(marco.zehe)
Comment on attachment 375325 [details] [diff] [review]
patch3

> nsXULProgressMeterAccessible::GetCurrentValue(double *aCurrentValue)
> {
>   nsresult rv = nsFormControlAccessible::GetCurrentValue(aCurrentValue);
>   if (rv != NS_OK_NO_ARIA_VALUE)
>     return rv;

Maybe you need to add this line:
*aCurrentValue = 0;
?
 
> 
>   nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
> 
>-  nsAutoString currentValue;
>-  content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::value, currentValue);
>+  nsAutoString attrValue;
>+  content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::value, attrValue);
> 
>-  PRInt32 result = NS_OK;
>-  if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::max))
>-    *aCurrentValue = currentValue.ToFloat(&result);
>-  else
>-    *aCurrentValue = currentValue.ToFloat(&result) / 100;
>+  // Return zero value if there is no attribute or its value is empty.
>+  if (attrValue.IsEmpty())
>+    return NS_OK;

Are you sure aCurrentValue is zero here?

>+  // If no max value then value is between 0 and 1 (refer to GetMaximumValue()
>+  // method where max value is assumed to be equal to 1 in this case).
>+  if (!content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::max))
>+    value /= 100;

Is there a danger of regressing anything by this new division behavior?

>-          ok(false, "Can't query " + aInterfaces[index] + " for " + aID);
>+          ok(false, "Can't query " + aInterfaces[index] + " for " + aAccOrElmOrID);

Nice.
(In reply to comment #7)

> > nsXULProgressMeterAccessible::GetCurrentValue(double *aCurrentValue)
> > {
> >   nsresult rv = nsFormControlAccessible::GetCurrentValue(aCurrentValue);

> Maybe you need to add this line:
> *aCurrentValue = 0;
> ?
> 
> Are you sure aCurrentValue is zero here?

I think this is reasonable assumption. Since every method should nulls out parameters and since we call method of base class then it must be nsnull. So I think it doesn't make sense to null it several times.

> >+  // If no max value then value is between 0 and 1 (refer to GetMaximumValue()
> >+  // method where max value is assumed to be equal to 1 in this case).
> >+  if (!content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::max))
> >+    value /= 100;
> 
> Is there a danger of regressing anything by this new division behavior?

What's new in this behaviour?
Comment on attachment 375325 [details] [diff] [review]
patch3

Alexander, thanks for putting my mind at ease (and you are right, the /100 is not new).
r=me
Attachment #375325 - Flags: review?(david.bolter) → review+
Comment on attachment 375325 [details] [diff] [review]
patch3

r=me
Attachment #375325 - Flags: review?(marco.zehe) → review+
http://hg.mozilla.org/mozilla-central/rev/ea3a2176d963
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #375325 - Flags: approval1.9.1?
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090506 Minefield/3.6a1pre (.NET CLR 3.5.30729).
Status: RESOLVED → VERIFIED
Attachment #375325 - Flags: approval1.9.1? → approval1.9.1+
Verified fixed on 1.9.1 in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre (.NET CLR 3.5.30729)
You need to log in before you can comment on or make changes to this bug.