1303447 - Implement ia2AccessibleText methods for ProxyAccessible in Windows

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Li]

MozReview-Commit-ID: IBttKLZdOgU

Comment 1

[Michael Li]

Attachment: Implement some ia2AccessibleText methods for ProxyAccessible in Windows

Compatible with shared header patch of bug 1303040 and contains QueryInterface wrappers from bug 1301148

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: implement text interface methods for the windows ProxyAccessible r?aklotz

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8802155 [details] [diff] [review]
implement text interface methods for the windows ProxyAccessible r?aklotz

Review of attachment 8802155 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure how the mingw people are going to feel about our use of _bstr_t, whose implementation is provided by MSVC and not the Windows SDK. I don't know whether or not mingw has a polyfill for that. OTOH, they're usually prompt about letting us know when they don't! ;-)

My main concern is with the cast in ScrollSubstringToPoint (see below). r=me with that fixed.

::: accessible/ipc/win/ProxyAccessible.cpp
@@ +389,5 @@
> +      return IA2_TEXT_BOUNDARY_WORD;
> +    case nsIAccessibleText::BOUNDARY_LINE_START:
> +      return IA2_TEXT_BOUNDARY_LINE;
> +    default:
> +      MOZ_ASSERT(false);

What about in the case of release builds? Should this be a release assert? Meh, I guess it's really only going to be used by our own test code, right?

@@ +394,5 @@
> +  }
> +}
> +
> +bool
> +ProxyAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOfset,

nit: s/EndOfset/EndOffset/

@@ +500,5 @@
> +  if (!acc) {
> +    return false;
> +  }
> +
> +  return !FAILED(acc->addSelection(static_cast<long>(aStartOffset),

s/!FAILED/SUCCEEDED/

@@ +512,5 @@
> +  if (!acc) {
> +    return false;
> +  }
> +
> +  return !FAILED(acc->removeSelection(static_cast<long>(aSelectionNum)));

s/!FAILED/SUCCEEDED/

@@ +545,5 @@
> +}
> +
> +void
> +ProxyAccessible::ScrollSubstringTo(int32_t aStartOffset, int32_t aEndOffset,
> +                                   uint32_t aScrollType)

Let's add a comment above this function definition pointing out that aScrollType should be a nsIAccessibleScrollType value.

@@ +554,5 @@
> +  }
> +
> +  acc->scrollSubstringTo(static_cast<long>(aStartOffset),
> +                         static_cast<long>(aEndOffset),
> +                         static_cast<IA2ScrollType>(aScrollType));

Could we maybe add a comment to the declaration of nsIAccessibleScrollType indicating that its values have a 1:1 correspondence with IA2ScrollType? That might allow an uneducated reader like me to save a few seconds looking up the IA2ScrollType IDL to compare. ;-)

@@ +559,5 @@
> +}
> +
> +void
> +ProxyAccessible::ScrollSubstringToPoint(int32_t aStartOffset, int32_t aEndOffset,
> +                                        uint32_t aCoordinateType, int32_t aX,

Comment that aCoordinateType is a nsIAccessibleCoordinateType value?

@@ +569,5 @@
> +  }
> +
> +  acc->scrollSubstringToPoint(static_cast<long>(aStartOffset),
> +                              static_cast<long>(aEndOffset),
> +                              static_cast<IA2CoordinateType>(aCoordinateType),

This look wrong given that IA2CoordinateType does not have an analog to  nsIAccessibleCoordinateType::COORDTYPE_WINDOW_RELATIVE. I don't think we can just cast this.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #3)
> ::: accessible/ipc/win/ProxyAccessible.cpp
> @@ +389,5 @@
> > +      return IA2_TEXT_BOUNDARY_WORD;
> > +    case nsIAccessibleText::BOUNDARY_LINE_START:
> > +      return IA2_TEXT_BOUNDARY_LINE;
> > +    default:
> > +      MOZ_ASSERT(false);
> 
> What about in the case of release builds? Should this be a release assert?
> Meh, I guess it's really only going to be used by our own test code, right?
> 

MOZ_ASSERT_UNREACHABLE perhaps?

Comment 5

[Trevor Saunders (:tbsaunde)]

> I'm not sure how the mingw people are going to feel about our use of
> _bstr_t, whose implementation is provided by MSVC and not the Windows SDK. I
> don't know whether or not mingw has a polyfill for that. OTOH, they're
> usually prompt about letting us know when they don't! ;-)

I just checked the mingw headers on my machine, and it looks like there's a _bstr_t class, so I expect it'll work.

I hope you mean _bstr_t is a header that comes from the compiler not it being some magic builtin, that sounds like madness.

> ::: accessible/ipc/win/ProxyAccessible.cpp
> @@ +389,5 @@
> > +      return IA2_TEXT_BOUNDARY_WORD;
> > +    case nsIAccessibleText::BOUNDARY_LINE_START:
> > +      return IA2_TEXT_BOUNDARY_LINE;
> > +    default:
> > +      MOZ_ASSERT(false);
> 
> What about in the case of release builds? Should this be a release assert?
> Meh, I guess it's really only going to be used by our own test code, right?

I agree it doesn't matter, but I guess we might as well here.

> 
> @@ +554,5 @@
> > +  }
> > +
> > +  acc->scrollSubstringTo(static_cast<long>(aStartOffset),
> > +                         static_cast<long>(aEndOffset),
> > +                         static_cast<IA2ScrollType>(aScrollType));
> 
> Could we maybe add a comment to the declaration of nsIAccessibleScrollType
> indicating that its values have a 1:1 correspondence with IA2ScrollType?
> That might allow an uneducated reader like me to save a few seconds looking
> up the IA2ScrollType IDL to compare. ;-)

it looks like there is one? though maybe it should be more visible / stronger?

> @@ +559,5 @@
> > +}
> > +
> > +void
> > +ProxyAccessible::ScrollSubstringToPoint(int32_t aStartOffset, int32_t aEndOffset,
> > +                                        uint32_t aCoordinateType, int32_t aX,
> 
> Comment that aCoordinateType is a nsIAccessibleCoordinateType value?

sure, blarg at xpidl for not allowing nice things.

> @@ +569,5 @@
> > +  }
> > +
> > +  acc->scrollSubstringToPoint(static_cast<long>(aStartOffset),
> > +                              static_cast<long>(aEndOffset),
> > +                              static_cast<IA2CoordinateType>(aCoordinateType),
> 
> This look wrong given that IA2CoordinateType does not have an analog to 
> nsIAccessibleCoordinateType::COORDTYPE_WINDOW_RELATIVE. I don't think we can
> just cast this.

good catch, I guess I should have been more careful when looking over this.

I'm not really sure what to do here with the window relative case, and for that matter I'm not terribly confident the bounds adjustment stuff for windows will work right with e10s anyway, but I guess that's what tests are for.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e487f723a0
implement text interface methods for the windows ProxyAccessible r=aklotz

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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