Last Comment Bug 762898 - implement UIA_AriaRolePropertyId
: implement UIA_AriaRolePropertyId
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 8
: -- normal (vote)
: mozilla17
Assigned To: Andrew Quartey [:drexler]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: uia
  Show dependency treegraph
 
Reported: 2012-06-08 07:08 PDT by alexander :surkov
Modified: 2012-07-20 21:02 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.94 KB, patch)
2012-07-10 19:42 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (2.05 KB, patch)
2012-07-16 11:53 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (2.06 KB, patch)
2012-07-16 11:59 PDT, Andrew Quartey [:drexler]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
patch_for_checkin (2.04 KB, patch)
2012-07-20 14:53 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch_for_checkin (2.01 KB, patch)
2012-07-20 15:24 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-06-08 07:08:27 PDT
it's analogue of xml-roles object attributes, see Accessible::GetAttributes(nsIPersistentProperties **aAttributes)

1) add a case uiaRawElmProvider::GetPropertyValue
2) obtain ARIA role attribute
3) return its value as BSTR
Comment 1 Andrew Quartey [:drexler] 2012-07-10 19:42:46 PDT
Created attachment 640898 [details] [diff] [review]
patch

This builds locally. Unsure of how to proceed in testing this...pointers more than welcomed.
Comment 2 Trevor Saunders (:tbsaunde) 2012-07-10 20:19:25 PDT
so, first thing you should use GetAttributes() here and get the xml-roles attributes since that will give you hte full aria role string not just the stringification of the enumerated role, which isn't correct with something like role="list, grid" where xml-roles will be correct. see http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.85%29.aspx#ARIARoleMapped or section 5.3 and 5.4 of the aria implementors guide.
Comment 3 Trevor Saunders (:tbsaunde) 2012-07-10 20:22:05 PDT
Comment on attachment 640898 [details] [diff] [review]
patch

>-  // UI Automation will attempt to get the property from the host
>-  //window provider.
>+  
>   aPropertyValue->vt = VT_EMPTY;

nit, trailing spaces on the blank line

>+  if (mAcc) {

that's always true, but you should return CO_E_OBJECTNOTCONNECTED if mAcc is defunct.
Comment 4 Andrew Quartey [:drexler] 2012-07-16 11:53:24 PDT
Created attachment 642669 [details] [diff] [review]
patch

made changes per previous review comment.
Comment 5 Andrew Quartey [:drexler] 2012-07-16 11:59:38 PDT
Created attachment 642672 [details] [diff] [review]
patch

Ooops. refreshed patch.
Comment 6 Trevor Saunders (:tbsaunde) 2012-07-19 20:14:38 PDT
Comment on attachment 642672 [details] [diff] [review]
patch

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

nit, blank line after }

note you'll need to merge in the changes in bug 762806 (which I just pushed).
Comment 7 Andrew Quartey [:drexler] 2012-07-20 14:53:00 PDT
Created attachment 644486 [details] [diff] [review]
patch_for_checkin

merged and rebased
Comment 8 Andrew Quartey [:drexler] 2012-07-20 15:24:25 PDT
Created attachment 644501 [details] [diff] [review]
patch_for_checkin
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-20 17:22:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2394b87d167
Comment 10 alexander :surkov 2012-07-20 18:43:23 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> so, first thing you should use GetAttributes() here and get the xml-roles
> attributes since that will give you hte full aria role string not just the
> stringification of the enumerated role, which isn't correct with something
> like role="list, grid" where xml-roles will be correct. see
> http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.
> 85%29.aspx#ARIARoleMapped or section 5.3 and 5.4 of the aria implementors
> guide.

the problem is that xml-roles is ARIA plus extra stuff, the spec says us to expose ARIA. 

Btw, the spec (http://msdn.microsoft.com/en-us/library/dd757479%28v=vs.85%29.aspx) doesn't say that "list grid" is incorrect value, see "Identifies the AriaRole property, which is a string containing the Accessible Rich Internet Application (ARIA) role information for the automation element."

Btw, the link you provided (http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.85%29.aspx#ARIARoleMapped) says "User agents may also offer secondary roles in the AriaRole property by using space as a separator, as defined in the ARIA W3C standard" what makes "list grid" value valid.
Comment 11 Trevor Saunders (:tbsaunde) 2012-07-20 18:54:18 PDT
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > so, first thing you should use GetAttributes() here and get the xml-roles
> > attributes since that will give you hte full aria role string not just the
> > stringification of the enumerated role, which isn't correct with something
> > like role="list, grid" where xml-roles will be correct. see
> > http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.
> > 85%29.aspx#ARIARoleMapped or section 5.3 and 5.4 of the aria implementors
> > guide.
> 
> the problem is that xml-roles is ARIA plus extra stuff, the spec says us to
> expose ARIA. 

it looks like it was originally your suggestion in the description.  I wouldn't really mind just mAcc->GetNode()->GetAttribute() but iirc xml-roles is pretty close so maybe ok.

> Btw, the spec
> (http://msdn.microsoft.com/en-us/library/dd757479%28v=vs.85%29.aspx) doesn't
> say that "list grid" is incorrect value, see "Identifies the AriaRole
> property, which is a string containing the Accessible Rich Internet
> Application (ARIA) role information for the automation element."
> 
> Btw, the link you provided
> (http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.
> 85%29.aspx#ARIARoleMapped) says "User agents may also offer secondary roles
> in the AriaRole property by using space as a separator, as defined in the
> ARIA W3C standard" what makes "list grid" value valid.

I don't disagree, my point was that  the string version of ARIARole() was not correct.
Comment 12 alexander :surkov 2012-07-20 18:57:37 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > the problem is that xml-roles is ARIA plus extra stuff, the spec says us to
> > expose ARIA. 
> 
> it looks like it was originally your suggestion in the description.

I said analogue ;)

>  I
> wouldn't really mind just mAcc->GetNode()->GetAttribute()

it should be what we need

> I don't disagree, my point was that  the string version of ARIARole() was
> not correct.

k, I see.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:02:02 PDT
https://hg.mozilla.org/mozilla-central/rev/d2394b87d167

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