380524 - implement IAccessibleComponent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[[e-mail]]

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

>+  // The coordinates of the returned position are relative to this object's
>+  // parent or relative to the screen on which this object is rendered if it
>+  // has no parent.
>+  if (!parentAcc)
>+    return NS_OK;
Except you didn't set *aX and *aY in this case.

>+STDMETHODIMP
>+CAccessibleComponent::get_foreground(IA2Color *aForeground)
>+{
>+  IA2ARGB color = 0;
>+  HRESULT hr = GetARGBValueFromCSSProperty(NS_LITERAL_STRING("color"), &color);
>+  *aForeground = color;
>+
>+  return hr;
>+}
Why not make GetARGBValueFromCSSProperty take an IA2Color* parameter?

>+  *aColorValue = (((IA2ARGB) (blue) << IA2BlueShift) |
>+                  ((IA2ARGB) (green) << IA2GreenShift) |
>+                  ((IA2ARGB) (red) << IA2RedShift) |
>+                  ((IA2ARGB) (alpha) << IA2AlphaShift));
Unfortunately alpha is from 0.0 to 1.0 while you probably want 0 to 255.
Also it's not clear what ((IA2ARGB) (blue) << IA2BlueShift) does, perhaps writing it as (((IAARGB) blue) << IA2BlueShift) might make more sense.

Comment 2

[alexander :surkov (:asurkov)]

> Also it's not clear what ((IA2ARGB) (blue) << IA2BlueShift) does, perhaps
> writing it as (((IAARGB) blue) << IA2BlueShift) might make more sense.
> 

Ok, though I took this from GDI+ Color class implementation (GdiPlusColor.h)

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

with neil's comments

Comment 4

[[e-mail]]

Comment on attachment 264635 [details] [diff] [review]
patch2

>+  // compose ARGB value
>+  IA2ARGB colorValue = 0;
>+  colorValue = ((((IA2ARGB) blue) << IA2BlueShift) |
>+                 (((IA2ARGB) green) << IA2GreenShift) |
>+                 (((IA2ARGB) red) << IA2RedShift) |
>+                 (((IA2ARGB) (alpha * 0xff)) << IA2AlphaShift));
>+
>+  *aColorValue = colorValue;
OK, so apparently an IA2Color is just a long, so there's no point defining a private IA2ARGB type for your intermediate calculation, just change these all to IA2Color and write *aColorValue directly. sr=me with this fixed. Also you don't need the outermost set of parentheses and you have too much indentation.

It's a pity that we use ABGR internally otherwise you could have used NS_RGBA()

Comment 5

[Aaron Leventhal]

Comment on attachment 264635 [details] [diff] [review]
patch2

r+ although I think locationInParent should still work if something is offscreen (we still expose coordinates) -- it's just outside of the currently visible bounds.

  if (states & nsIAccessibleStates::STATE_INVISIBLE ||

      states & nsIAccessibleStates::STATE_OFFSCREEN)

    return S_OK;

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch3

with Neil's and Aaron's comments

Comment 7

[alexander :surkov (:asurkov)]

checked in