Closed Bug 762806 Opened 12 years ago Closed 12 years ago

implement UIA_AcceleratorKeyPropertyId and UIA_AccessKeyPropertyId

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: surkov, Assigned: cib123)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(2 files, 1 obsolete file)

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
Attached patch Patch (v1)Splinter Review
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"  :)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #640547 - Flags: feedback?(surkov.alexander)
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.
Attachment #640547 - Flags: feedback?(surkov.alexander)
Ok, well this is a good start then.
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
(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.
I found a volunteer so I'm assigning this.
Assignee: nobody → cib123
Status: NEW → ASSIGNED
Attached patch Cleanup of capella's patch (obsolete) — Splinter Review
Alright, tried to clean up capella's patch. Hopefully I managed to, somehow.
Attachment #642245 - Flags: review?(trev.saunders)
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()
Attachment #642245 - Flags: review?(trev.saunders) → review+
Attached patch Patch (v3)Splinter Review
Fixed up those last nit's
Attachment #642245 - Attachment is obsolete: true
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."
https://hg.mozilla.org/mozilla-central/rev/b37ed3f04df9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(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.
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: