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.

> 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)

with neil's comments

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 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;

with Neil's and Aaron's comments

checked in