The default bug view has changed. See this FAQ.

implement UIA_AcceleratorKeyPropertyId and UIA_AccessKeyPropertyId

RESOLVED FIXED in mozilla17

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: cib123)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
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"  :)
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
(Assignee)

Comment 6

5 years ago
Created attachment 642245 [details] [diff] [review]
Cleanup of capella's patch

Alright, tried to clean up capella's patch. Hopefully I managed to, somehow.
(Reporter)

Updated

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 642694 [details] [diff] [review]
Patch (v3)

Fixed up those last nit's
Attachment #642245 - Attachment is obsolete: true
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b37ed3f04df9
(Reporter)

Comment 10

5 years ago
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
Last Resolved: 5 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.
(Reporter)

Comment 13

5 years ago
(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.