Add accessibility support for Textfields in Windows.

Add accessibility support for Textfields in Windows.
BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49955

[tfarina, 2010-06-10 21:34:15]

My style nits below.

http://codereview.chromium.org/2791003/diff/13001/14001
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2791003/diff/13001/14001#newcode330
views/controls/textfield/native_textfield_win.cc:330: IID_IAccPropServices,
reinterpret_cast<void**>(&pAccPropServices));
please, when align on the next line indent 4 spaces. There more cases below.

http://codereview.chromium.org/2791003/diff/13001/14001#newcode331
views/controls/textfield/native_textfield_win.cc:331: if (SUCCEEDED(hr)) {
To avoid doing this in a block, I'd return early here.

http://codereview.chromium.org/2791003/diff/13001/14001#newcode342
views/controls/textfield/native_textfield_win.cc:342: int labelIndex =
parent->GetChildIndex(textfield_) - 1;
variables should be foo_bar. Please, could you fix the other occurrences too?

http://codereview.chromium.org/2791003/diff/13001/14001#newcode363
views/controls/textfield/native_textfield_win.cc:363: if (SUCCEEDED(hr)) {
could we return early here?

[tfarina, 2010-06-10 21:36:52]

http://codereview.chromium.org/2791003/diff/13001/14002
File views/controls/textfield/native_textfield_win.h (right):

http://codereview.chromium.org/2791003/diff/13001/14002#newcode13
views/controls/textfield/native_textfield_win.h:13: #include<oleacc.h>
space between include and <

http://codereview.chromium.org/2791003/diff/13001/14002#newcode77
views/controls/textfield/native_textfield_win.h:77: void
UpdateAccessibleState(int stateFlag, bool set_value);
maybe use uint16/uint32. I remember that we defined something like that for flag
states (don't remember where).

http://codereview.chromium.org/2791003/diff/13001/14002#newcode78
views/controls/textfield/native_textfield_win.h:78: void
UpdateAccessibleValue(std::wstring value);
instead of copying the value, could we pass by const-reference? const
std::wstring& value?

[David Tseng, 2010-06-10 23:02:52]

http://codereview.chromium.org/2791003/diff/13001/14001
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2791003/diff/13001/14001#newcode330
views/controls/textfield/native_textfield_win.cc:330:
IID_IAccPropServices, reinterpret_cast<void**>(&pAccPropServices));
please, when align on the next line indent 4 spaces. There more cases
below.
Changed from 2 space indent to 4 space indent.

http://codereview.chromium.org/2791003/diff/13001/14001#newcode331
views/controls/textfield/native_textfield_win.cc:331: if (SUCCEEDED(hr))
{
To avoid doing this in a block, I'd return early here.
Done.

http://codereview.chromium.org/2791003/diff/13001/14001#newcode342
views/controls/textfield/native_textfield_win.cc:342: int labelIndex =
parent->GetChildIndex(textfield_) - 1;
variables should be foo_bar. Please, could you fix the other occurrences
too?
Done.

http://codereview.chromium.org/2791003/diff/13001/14001#newcode363
views/controls/textfield/native_textfield_win.cc:363: if (SUCCEEDED(hr))
{
could we return early here?
Done


http://codereview.chromium.org/2791003/diff/13001/14002
File views/controls/textfield/native_textfield_win.h (right):

http://codereview.chromium.org/2791003/diff/13001/14002#newcode13
views/controls/textfield/native_textfield_win.h:13: #include<oleacc.h>
space between include and <
Done.

http://codereview.chromium.org/2791003/diff/13001/14002#newcode77
views/controls/textfield/native_textfield_win.h:77: void
UpdateAccessibleState(int stateFlag, bool set_value);
maybe use uint16/uint32. I remember that we defined something like that
for flag states (don't remember where).
Changed to uint32.

http://codereview.chromium.org/2791003/diff/13001/14002#newcode78
views/controls/textfield/native_textfield_win.h:78: void
UpdateAccessibleValue(std::wstring value);
instead of copying the value, could we pass by const-reference? const
std::wstring& value?
Done.

On 6/10/10, tfarina@chromium.org <tfarina@chromium.org> wrote:
>
> http://codereview.chromium.org/2791003/diff/13001/14002
> File views/controls/textfield/native_textfield_win.h (right):
>
> http://codereview.chromium.org/2791003/diff/13001/14002#newcode13
> views/controls/textfield/native_textfield_win.h:13: #include<oleacc.h>
> space between include and <
>
> http://codereview.chromium.org/2791003/diff/13001/14002#newcode77
> views/controls/textfield/native_textfield_win.h:77: void
> UpdateAccessibleState(int stateFlag, bool set_value);
> maybe use uint16/uint32. I remember that we defined something like that
> for flag states (don't remember where).
>
> http://codereview.chromium.org/2791003/diff/13001/14002#newcode78
> views/controls/textfield/native_textfield_win.h:78: void
> UpdateAccessibleValue(std::wstring value);
> instead of copying the value, could we pass by const-reference? const
> std::wstring& value?
>
> http://codereview.chromium.org/2791003/show
>

[tfarina, 2010-06-10 23:17:25]

Some more style nits below. :)

http://codereview.chromium.org/2791003/diff/19001/16002
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2791003/diff/19001/16002#newcode331
views/controls/textfield/native_textfield_win.cc:331: if (!SUCCEEDED(hr)) {
if we return here, we may end up without setting accessible_state_ = 0;

Should it be moved above of this check?

Also no need of {} in single line statements.

http://codereview.chromium.org/2791003/diff/19001/16002#newcode350
views/controls/textfield/native_textfield_win.cc:350: && label_view
->GetAccessibleName(&name)) {
this && should be in the end of the above line.

http://codereview.chromium.org/2791003/diff/19001/16002#newcode373
views/controls/textfield/native_textfield_win.cc:373: : accessibility_state_ &
~state_flag;
this indentation looks ugly :/, please align : with ? or indent 4 spaces.

[David Tseng, 2010-06-10 23:39:44]

> http://codereview.chromium.org/2791003/diff/19001/16002
> File views/controls/textfield/native_textfield_win.cc (right):
>
> http://codereview.chromium.org/2791003/diff/19001/16002#newcode331
> views/controls/textfield/native_textfield_win.cc:331: if
> (!SUCCEEDED(hr)) {
> if we return here, we may end up without setting accessible_state_ = 0;
>
> Should it be moved above of this check?
>
> Also no need of {} in single line statements.
>
Good point.  Moved above; removed the {}.

> http://codereview.chromium.org/2791003/diff/19001/16002#newcode350
> views/controls/textfield/native_textfield_win.cc:350: && label_view
> ->GetAccessibleName(&name)) {
> this && should be in the end of the above line.
>
Done.

> http://codereview.chromium.org/2791003/diff/19001/16002#newcode373
> views/controls/textfield/native_textfield_win.cc:373: :
> accessibility_state_ & ~state_flag;
> this indentation looks ugly :/, please align : with ? or indent 4
> spaces.
>
> http://codereview.chromium.org/2791003/show
>
Done.

[Chris Guillory, 2010-06-11 18:46:54]

http://codereview.chromium.org/2791003/diff/24001/20002
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2791003/diff/24001/20002#newcode355
views/controls/textfield/native_textfield_win.cc:355: hr =
pAccPropServices->SetHwndProp(m_hWnd, OBJID_CLIENT,
Let's use SetHwndPropStr here so that we don't need to allocate a BSTR.

http://codereview.chromium.org/2791003/diff/24001/20002#newcode374
views/controls/textfield/native_textfield_win.cc:374: CHILDID_SELF,
PROPID_ACC_STATE, var);
NotifyWinEvent about the state change?

http://codereview.chromium.org/2791003/diff/24001/20002#newcode387
views/controls/textfield/native_textfield_win.cc:387: hr =
pAccPropServices->SetHwndProp(m_hWnd, OBJID_CLIENT,
SetHwndPropStr

[David Tseng, 2010-06-11 19:28:28]

>
> http://codereview.chromium.org/2791003/diff/24001/20002
> File views/controls/textfield/native_textfield_win.cc (right):
>
> http://codereview.chromium.org/2791003/diff/24001/20002#newcode355
> views/controls/textfield/native_textfield_win.cc:355: hr =
> pAccPropServices->SetHwndProp(m_hWnd, OBJID_CLIENT,
> Let's use SetHwndPropStr here so that we don't need to allocate a BSTR.
Changed (as well below).

> http://codereview.chromium.org/2791003/diff/24001/20002#newcode374
> views/controls/textfield/native_textfield_win.cc:374: CHILDID_SELF,
> PROPID_ACC_STATE, var);
> NotifyWinEvent about the state change?
I'm not sure what the convention is here but added it since text
fields could change state after the user types into it.



> http://codereview.chromium.org/2791003/diff/24001/20002#newcode387
> views/controls/textfield/native_textfield_win.cc:387: hr =
> pAccPropServices->SetHwndProp(m_hWnd, OBJID_CLIENT,
> SetHwndPropStr
>
> http://codereview.chromium.org/2791003/show
>

[Chris Guillory, 2010-06-15 17:30:16]

LGTM. One comment.

http://codereview.chromium.org/2791003/diff/28001/29001
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2791003/diff/28001/29001#newcode351
views/controls/textfield/native_textfield_win.cc:351: if (label_view
->GetClassName() == Label::kViewClassName &&
Can we DCHECK label_view ->GetClassName() == Label::kViewClassName?

[David Tseng, 2010-06-15 23:06:03]

>
>
> http://codereview.chromium.org/2791003/diff/28001/29001
> File views/controls/textfield/native_textfield_win.cc (right):
>
> http://codereview.chromium.org/2791003/diff/28001/29001#newcode351
> views/controls/textfield/native_textfield_win.cc:351: if (label_view
> ->GetClassName() == Label::kViewClassName &&
> Can we DCHECK label_view ->GetClassName() == Label::kViewClassName?
>
> http://codereview.chromium.org/2791003/show
Not sure if we want this.  I'm making the assumption that the label
immediately preceeds the TextField in the view tree.  This could
reasonably be false such as when the TextField has no label.  I'm
making my best guess as to the label for the field here.

[Chris Guillory, 2010-06-15 23:16:09]

Oh, I didn't know that this could be reasonably false. Perhaps this need a add a
comment.

[David Tseng, 2010-06-15 23:17:20]

Done.

On 6/15/10, ctguil@chromium.org <ctguil@chromium.org> wrote:
> Oh, I didn't know that this could be reasonably false. Perhaps this need a
> add a
> comment.
>
> http://codereview.chromium.org/2791003/show
>