Last Comment Bug 762806 - implement UIA_AcceleratorKeyPropertyId and UIA_AccessKeyPropertyId
: implement UIA_AcceleratorKeyPropertyId and UIA_AccessKeyPropertyId
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: cib123
:
Mentors:
http://msdn.microsoft.com/en-us/libra...
Depends on:
Blocks: uia
  Show dependency treegraph
 
Reported: 2012-06-08 01:03 PDT by alexander :surkov
Modified: 2012-07-20 18:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (1.62 KB, patch)
2012-07-10 03:06 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Cleanup of capella's patch (1.76 KB, patch)
2012-07-14 09:41 PDT, cib123
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch (v3) (1.65 KB, patch)
2012-07-16 13:19 PDT, cib123
no flags Details | Diff | Review

Description alexander :surkov 2012-06-08 01:03:44 PDT
spec: http://msdn.microsoft.com/en-us/library/windows/desktop/ee684017%28v=vs.85%29.aspx.

1) wait when bug 762770 landed
2) fix uiaRawElmProvider::GetPropertyValue
3) for that use Accessible::AccessKey() and KeyboardShortcut() methods
Comment 1 Mark Capella [:capella] 2012-07-10 03:06:28 PDT
Created attachment 640547 [details] [diff] [review]
Patch (v1)

Thought I'd try this out, see if I can help with UIA. This builds ok, but I'm still trying to figure out testing methods ... trying to get the debugger to kick in to tell if its "doing something"  :)
Comment 2 Trevor Saunders (:tbsaunde) 2012-07-10 03:49:12 PDT
Comment on attachment 640547 [details] [diff] [review]
Patch (v1)

>   if (!aPropertyValue)
>     return E_INVALIDARG;
> 

you should set the default to VT_EMPTY before the switch, you should also make sure mAcc is not defunct.

>+  switch (aPropertyId)
>+  {

nit,
switch(x) {

>+    // Accelerator Key / shortcut.
>+    case UIA_AcceleratorKeyPropertyId:
>+    {

same

>+      KeyBinding keyBinding = mAcc->KeyboardShortcut();

no need for the local,
mAcc->KeyboardShortcut()->ToString(keyString); is fine

>+      if (!keyString.IsEmpty()) {
>+        aPropertyValue->vt = VT_BSTR;
>+        aPropertyValue->bstrVal = ::SysAllocString(keyString.get());
>+        return S_OK;
>+      }
>+    }

you shouldn't fall through here.

>+    case UIA_AccessKeyPropertyId:
>+    {

nit, { on same line

>+      KeyBinding keyBinding = mAcc->AccessKey();

no need for the local here either.

>+      if (!keyString.IsEmpty()) {
>+        aPropertyValue->vt = VT_BSTR;
>+        aPropertyValue->bstrVal = ::SysAllocString(keyString.get());
>+        return S_OK;
>+      }
>+    }

don't fall through here either.
Comment 3 Mark Capella [:capella] 2012-07-10 06:38:40 PDT
Ok, well this is a good start then.
Comment 4 Trevor Saunders (:tbsaunde) 2012-07-10 19:57:30 PDT
(In reply to Mark Capella [:capella] from comment #3)
> Ok, well this is a good start then.

true, but needs some cleanup, and fixing the bug related to falling through.
Comment 5 Mark Capella [:capella] 2012-07-14 08:33:36 PDT
I found a volunteer so I'm assigning this.
Comment 6 cib123 2012-07-14 09:41:28 PDT
Created attachment 642245 [details] [diff] [review]
Cleanup of capella's patch

Alright, tried to clean up capella's patch. Hopefully I managed to, somehow.
Comment 7 Trevor Saunders (:tbsaunde) 2012-07-16 09:09:38 PDT
Comment on attachment 642245 [details] [diff] [review]
Cleanup of capella's patch

>   if (!aPropertyValue)
>-    return E_INVALIDARG;
>+    return E_INVALIDARG;  

nit, don't introduce trailing whitespace.

>+    case UIA_AccessKeyPropertyId: {
>+      KeyBinding keyBinding = mAcc->AccessKey();
>+
>+      nsAutoString keyString;
>+      keyBinding.ToString(keyString);

nit, you could just do mAcc->AccessKey()->ToString()
Comment 8 cib123 2012-07-16 13:19:20 PDT
Created attachment 642694 [details] [diff] [review]
Patch (v3)

Fixed up those last nit's
Comment 9 Trevor Saunders (:tbsaunde) 2012-07-19 20:00:18 PDT
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b37ed3f04df9
Comment 10 alexander :surkov 2012-07-19 22:57:26 PDT
it seems we shouldn't expose 'letter' as accesskey (not 'alt+accesskey) since spec says:
"An access key (sometimes called a mnemonic) is a character in the text of a menu, menu item, or label of a control such as a button, that activates the associated menu function. For example, to open the File menu, for which the access key is typically F, the user would press ALT+F."
Comment 11 Ed Morley [:emorley] 2012-07-20 06:42:26 PDT
https://hg.mozilla.org/mozilla-central/rev/b37ed3f04df9
Comment 12 Trevor Saunders (:tbsaunde) 2012-07-20 18:16:52 PDT
(In reply to alexander :surkov from comment #10)
> it seems we shouldn't expose 'letter' as accesskey (not 'alt+accesskey)

I guess you mean should?

> since spec says:
> "An access key (sometimes called a mnemonic) is a character in the text of a
> menu, menu item, or label of a control such as a button, that activates the
> associated menu function. For example, to open the File menu, for which the
> access key is typically F, the user would press ALT+F."

tbh I find that really confusing, and I'm not sure what to think.
Comment 13 alexander :surkov 2012-07-20 18:20:16 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> (In reply to alexander :surkov from comment #10)
> > it seems we shouldn't expose 'letter' as accesskey (not 'alt+accesskey)
> 
> I guess you mean should?

right

> > since spec says:
> > "An access key (sometimes called a mnemonic) is a character in the text of a
> > menu, menu item, or label of a control such as a button, that activates the
> > associated menu function. For example, to open the File menu, for which the
> > access key is typically F, the user would press ALT+F."
> 
> tbh I find that really confusing, and I'm not sure what to think.

the spec doesn't seem super clean, we can keep it as it is for now and if narrator doesn't pick it up then fix it.

Note You need to log in before you can comment on or make changes to this bug.