935698 - isolate nsIAccessibleValue implementation into separate class

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

fixed mochitests

a change: do not expose 0% as value on indeterminate progress elements.

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 828659 [details] [diff] [review]
patch

>+#include "mozilla/Likely.h"
>+#include "mozilla/FloatingPoint.h"

shouldn't FloatingPoint come first?

>+  double accDouble = accWrap->CurValue();

I think I'd prefer accValue, but whatever

>+Accessible::Step() const
> {
>-  NS_ENSURE_ARG_POINTER(aMinIncrement);
>-  *aMinIncrement = 0;
>-
>-  // No mimimum increment in dynamic content spec right now
>-  return NS_OK_NO_ARIA_VALUE;
>+  return UnspecifiedNaN(); // no mimimum increment (step) in ARIA.

so, you're changing behavior here for atleast atk and I think ia2 as well since they just check failure not which success code was returned.

>+Accessible::SetCurValue(double aValue)
> {
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   if (!mRoleMapEntry || mRoleMapEntry->valueRule == eNoValue)
>-    return NS_OK_NO_ARIA_VALUE;
>+    return false;

again probably changing behavior.

>+Accessible::AttrNumericValue(nsIAtom* aAttr) const
> {
>-  NS_ENSURE_ARG_POINTER(aValue);
>-  *aValue = 0;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;  // Node already shut down
>-
>- if (!mRoleMapEntry || mRoleMapEntry->valueRule == eNoValue)
>-    return NS_OK_NO_ARIA_VALUE;
>+  if (!mRoleMapEntry || mRoleMapEntry->valueRule == eNoValue)
>+    return UnspecifiedNaN();

again changing behavior

>   //////////////////////////////////////////////////////////////////////////////
>+  // Value (a numberic interface)

// numeric value interface maybe? (fix the typo atleast)

> ProgressMeterAccessible<Max>::Value(nsString& aValue)
> {
>   LeafAccessible::Value(aValue);
>   if (!aValue.IsEmpty())
>     return;
> 
>-  double maxValue = 0;
>-  nsresult rv = GetMaximumValue(&maxValue);
>-  if (NS_FAILED(rv) || maxValue == 0)
>+  double maxValue = MaxValue();
>+  if (IsNaN(maxValue) || maxValue == 0)
>     return;

why is second check needed?

> 
>+  // Value
>+  virtual double MaxValue() const;
>+  virtual double MinValue() const;
>+  virtual double CurValue() const;
>+  virtual double Step() const;
>+  virtual bool SetCurValue(double aValue);

mark them as override here and else where?

>+NS_IMETHODIMP
>+xpcAccessibleValue::GetMaximumValue(double* aValue)
>+{
>+  NS_ENSURE_ARG_POINTER(aValue);
>+  *aValue = 0;
>+
>+  Accessible* acc = static_cast<Accessible*>(this);
>+  if (acc->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  double value = acc->MaxValue();
>+  if (!IsNaN(value))

hm, it seems like you shouldn't be able to refer to that without mozilla::

also I wonder if returning 0 and not throwing is really the right thing to do.

>+  NS_IMETHOD GetMaximumValue(double* aValue) MOZ_FINAL;
>+  NS_IMETHOD GetMinimumValue(double* aValue) MOZ_FINAL;
>+  NS_IMETHOD GetCurrentValue(double* aValue) MOZ_FINAL;
>+  NS_IMETHOD SetCurrentValue(double aValue) MOZ_FINAL;
>+  NS_IMETHOD GetMinimumIncrement(double* aMinIncrement) MOZ_FINAL;

you can mark these as over ride too right?

>-    return rv;
>+  bool wasSet = AccessibleWrap::SetCurValue(aValue);
>+  if (wasSet)
>+    return true;

what do you need the var for?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >+Accessible::Step() const
> > {
> >-  NS_ENSURE_ARG_POINTER(aMinIncrement);
> >-  *aMinIncrement = 0;
> >-
> >-  // No mimimum increment in dynamic content spec right now
> >-  return NS_OK_NO_ARIA_VALUE;
> >+  return UnspecifiedNaN(); // no mimimum increment (step) in ARIA.
> 
> so, you're changing behavior here for atleast atk and I think ia2 as well
> since they just check failure not which success code was returned.

the behavior is changed in error case when no attribute is provided, returning early should be ok for error case, ATK says nothing how to handle it (https://developer.gnome.org/atk/unstable/AtkValue.html)

> >+Accessible::SetCurValue(double aValue)
> > {
> >-  if (IsDefunct())
> >-    return NS_ERROR_FAILURE;
> >-
> >   if (!mRoleMapEntry || mRoleMapEntry->valueRule == eNoValue)
> >-    return NS_OK_NO_ARIA_VALUE;
> >+    return false;
> 
> again probably changing behavior.

ATK says to return false if value cannot be changed, no ARIA value applied means no value change

> >+Accessible::AttrNumericValue(nsIAtom* aAttr) const
> again changing behavior

same

> > ProgressMeterAccessible<Max>::Value(nsString& aValue)
> >+  if (IsNaN(maxValue) || maxValue == 0)
> >     return;
> 
> why is second check needed?

to not devide by 0

> >+NS_IMETHODIMP
> >+xpcAccessibleValue::GetMaximumValue(double* aValue)
> >+{
> >+  double value = acc->MaxValue();
> >+  if (!IsNaN(value))
> 
> hm, it seems like you shouldn't be able to refer to that without mozilla::

I'll add using mozilla but it works


> also I wonder if returning 0 and not throwing is really the right thing to
> do.

I don't really like when JS throws, especially when getters do that.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >+Accessible::Step() const
> > > {
> > >-  NS_ENSURE_ARG_POINTER(aMinIncrement);
> > >-  *aMinIncrement = 0;
> > >-
> > >-  // No mimimum increment in dynamic content spec right now
> > >-  return NS_OK_NO_ARIA_VALUE;
> > >+  return UnspecifiedNaN(); // no mimimum increment (step) in ARIA.
> > 
> > so, you're changing behavior here for atleast atk and I think ia2 as well
> > since they just check failure not which success code was returned.
> 
> the behavior is changed in error case when no attribute is provided,

Well for this method there is no attribute so it always returns NaN unless its over riden right?

> returning early should be ok for error case, ATK says nothing how to handle
> it (https://developer.gnome.org/atk/unstable/AtkValue.html)

sure, it seems like a better behavior, I'd just really prefer it happened seperately from refactoring a bunch of stuff.

> > >+NS_IMETHODIMP
> > >+xpcAccessibleValue::GetMaximumValue(double* aValue)
> > >+{
> > >+  double value = acc->MaxValue();
> > >+  if (!IsNaN(value))
> > 
> > hm, it seems like you shouldn't be able to refer to that without mozilla::
> 
> I'll add using mozilla but it works

maybe because class is within mozilla namespace?

> 
> > also I wonder if returning 0 and not throwing is really the right thing to
> > do.
> 
> I don't really like when JS throws, especially when getters do that.

I guess, I don't really like silent failure either though

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> Well for this method there is no attribute so it always returns NaN unless
> its over riden right?

right, ATK says to use 0 " If zero, the minimum increment is undefined, which may mean that it is limited only by the floating point precision of the platform. "

> > returning early should be ok for error case, ATK says nothing how to handle
> > it (https://developer.gnome.org/atk/unstable/AtkValue.html)
> 
> sure, it seems like a better behavior, I'd just really prefer it happened
> seperately from refactoring a bunch of stuff.

I agree, sometimes it's hard to do

> > I don't really like when JS throws, especially when getters do that.
> 
> I guess, I don't really like silent failure either though

I see, probably it's not big deal since our consumer is the test suite and plus accessFu using limited API

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[Trevor Saunders (:tbsaunde)]

> > > returning early should be ok for error case, ATK says nothing how to handle
> > > it (https://developer.gnome.org/atk/unstable/AtkValue.html)
> > 
> > sure, it seems like a better behavior, I'd just really prefer it happened
> > seperately from refactoring a bunch of stuff.
> 
> I agree, sometimes it's hard to do

Well consider this using your get out of jail free card ;)

> > > I don't really like when JS throws, especially when getters do that.
> > 
> > I guess, I don't really like silent failure either though
> 
> I see, probably it's not big deal since our consumer is the test suite and
> plus accessFu using limited API

I guess

Comment 8

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a2ef61a6c5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1a2ef61a6c5