Joanmarie Diggs Assignee
	Description • 7 years ago

Steps to reproduce:
1. Load data:text/html,<div tabindex="0" role="textbox" aria-readonly="true">text</div>
2. Use Accerciser to examine the element

Expected results: the element would be exposed with STATE_READ_ONLY
Actual results: the element is not exposed with STATE_READ_ONLY

https://rawgit.com/w3c/aria/master/core-aam/core-aam.html#ariaReadonlyTrue

(Note that at the time of this writing, Core AAM spells the state value incorrectly. There is a pending pull request to fix that.)

	Joanmarie Diggs Assignee
	Comment 1 • 7 years ago

In addition to the above:

1. Text entries with aria-readonly have STATE_EDITABLE removed (good). BUT, they still claim to implement the EditableText interface (bad). Please do not expose that interface as implemented on entries where readonly is true.

2. I believe checkable elements which are readonly should not expose STATE_CHECKABLE. That state is the equivalent of STATE_EDITABLE, but for checkboxes (and subclasses thereof).

	Joanmarie Diggs Assignee
	Comment 2 • 7 years ago

Also aria-readonly is now supported on switch, radiogroup, menuitemradio, and menuitemcheckbox.

	alexander :surkov (:asurkov)
	Comment 4 • 7 years ago

Comment on attachment 8878729 [details] [diff] [review]
1356018.patch

Review of attachment 8878729 [details] [diff] [review]:
-----------------------------------------------------------------

It is not a requirement, but it feels like the bug would benefit if split it into two (ARIA part and ATK part).

Cancelling review request for now (waiting for comments to be addressed).

::: accessible/atk/AccessibleWrap.cpp
@@ +965,5 @@
> +    role = accWrap->Role();
> +  } else if (ProxyAccessible* proxy = GetProxy(aAtkObj)) {
> +    state = proxy->State();
> +    role = proxy->Role();
> +  }

role is not the cheapest call (in non-proxy case), and generally should be avoided. It might be ok for now though. So it's up to you to use Is...() methods instead or go with Role() call.

@@ +970,1 @@
>  

state defunct part is missing

@@ +970,4 @@
>  
> +  if (state & states::READONLY) {
> +    state &= ~states::EDITABLE;
> +    state &= ~states::CHECKABLE;

I would combine these together.

@@ +975,5 @@
> +    // According to the ATK documentation, the read-only state "should only be
> +    // applied to widget types whose value is normally directly user modifiable.
> +    if (role == roles::LIST || role == roles::LISTITEM || role == roles::TERM ||
> +        role == roles::PROGRESSBAR || role == roles::DOCUMENT)
> +    state &= ~states::READONLY;

nit: indentation issue

@@ +978,5 @@
> +        role == roles::PROGRESSBAR || role == roles::DOCUMENT)
> +    state &= ~states::READONLY;
> +  }
> +
> +  TranslateStates(state, state_set);

it makes sense to put the method's content here

::: accessible/base/ARIAMap.cpp
@@ +789,4 @@
>      eNoLiveAttr,
>      kGenericAccType,
>      kNoReqStates,
> +    eARIACheckedMixed // XXX: Not according to the ARIA spec.

yep, it goes from the beginning of times, it's quite likely ARIA has changed or was misread :) So seems like a bug, worth to file a new bug on it.

@@ +800,5 @@
>      eNoLiveAttr,
>      kGenericAccType,
>      kNoReqStates,
> +    eARIACheckableMixed,
> +    eARIAReadonly

I don't see area-readonly in ARIA spec [1], where it goes from?

[1] https://www.w3.org/TR/wai-aria-1.1/#menuitem

	Joanmarie Diggs Assignee
	Comment 5 • 7 years ago

(In reply to alexander :surkov from comment #4)
> Comment on attachment 8878729 [details] [diff] [review]
> 1356018.patch

> @@ +970,1 @@
> >  
> 
> state defunct part is missing

It was moved to the initialization:

@@ -962,14 +957,29 @@ refStateSetCB(AtkObject *aAtkObj)
   AtkStateSet *state_set = nullptr;
   state_set = ATK_OBJECT_CLASS(parent_class)->ref_state_set(aAtkObj);
 
-  AccessibleWrap* accWrap = GetAccessibleWrap(aAtkObj);
-  if (accWrap)
-    TranslateStates(accWrap->State(), state_set);
-  else if (ProxyAccessible* proxy = GetProxy(aAtkObj))
-    TranslateStates(proxy->State(), state_set);
-  else
-    TranslateStates(states::DEFUNCT, state_set);
+  uint64_t state = states::DEFUNCT;
+  roles::Role role = roles::NOTHING;

But I will leave it where it was.
 
> ::: accessible/base/ARIAMap.cpp
> @@ +789,4 @@
> >      eNoLiveAttr,
> >      kGenericAccType,
> >      kNoReqStates,
> > +    eARIACheckedMixed // XXX: Not according to the ARIA spec.
> 
> yep, it goes from the beginning of times, it's quite likely ARIA has changed
> or was misread :) So seems like a bug, worth to file a new bug on it.

Since I'll split up the issue into two bugs, one for the ARIA part and one for the ATK part, I'll just lump that fix into the ARIA part.
 
> @@ +800,5 @@
> >      eNoLiveAttr,
> >      kGenericAccType,
> >      kNoReqStates,
> > +    eARIACheckableMixed,
> > +    eARIAReadonly
> 
> I don't see area-readonly in ARIA spec [1], where it goes from?
> 
> [1] https://www.w3.org/TR/wai-aria-1.1/#menuitem

Sorry, I guess I'm confused. In my patch, the menuitem role is the one which has:

> > +    eARIACheckedMixed // XXX: Not according to the ARIA spec.

What I have in my patch at line 804 is menuitemcheckbox:

   },
   { // menuitemcheckbox
     &nsGkAtoms::menuitemcheckbox,
@@ -800,7 +800,8 @@ static const nsRoleMapEntry sWAIRoleMaps
     eNoLiveAttr,
     kGenericAccType,
     kNoReqStates,
-    eARIACheckableMixed
+    eARIACheckableMixed,
+    eARIAReadonly
   },

	alexander :surkov (:asurkov)
	Comment 6 • 7 years ago

(In reply to Joanmarie Diggs from comment #5)

> > state defunct part is missing
> 
> It was moved to the initialization:

right

> > yep, it goes from the beginning of times, it's quite likely ARIA has changed
> > or was misread :) So seems like a bug, worth to file a new bug on it.
> 
> Since I'll split up the issue into two bugs, one for the ARIA part and one
> for the ATK part, I'll just lump that fix into the ARIA part.

thanks

>  
> > @@ +800,5 @@
> > >      eNoLiveAttr,
> > >      kGenericAccType,
> > >      kNoReqStates,
> > > +    eARIACheckableMixed,
> > > +    eARIAReadonly
> > 
> > I don't see area-readonly in ARIA spec [1], where it goes from?
> > 
> > [1] https://www.w3.org/TR/wai-aria-1.1/#menuitem
> 
> Sorry, I guess I'm confused. In my patch, the menuitem role is the one which
> has:
> 
> > > +    eARIACheckedMixed // XXX: Not according to the ARIA spec.
> 
> What I have in my patch at line 804 is menuitemcheckbox:

got it, the diff gives so little of the context, I must have confused by it.

	Joanmarie Diggs Assignee
	Comment 7 • 7 years ago

(In reply to alexander :surkov from comment #6)
> (In reply to Joanmarie Diggs from comment #5)
> 
> > > state defunct part is missing
> > 
> > It was moved to the initialization:
> 
> right
> 
> > > yep, it goes from the beginning of times, it's quite likely ARIA has changed
> > > or was misread :) So seems like a bug, worth to file a new bug on it.
> > 
> > Since I'll split up the issue into two bugs, one for the ARIA part and one
> > for the ATK part, I'll just lump that fix into the ARIA part.
> 
> thanks

The ARIA part is filed as bug 1374316, with patch attached.

	Joanmarie Diggs Assignee
	Comment 8 • 7 years ago

(In reply to alexander :surkov from comment #4)
 
> role is not the cheapest call (in non-proxy case), and generally should be
> avoided. It might be ok for now though. So it's up to you to use Is...()
> methods instead or go with Role() call.

If it generally should be avoided, I'm happy to avoid it. :) And IsWidget() may be a better fit (non-widgets shouldn't expose ATK_STATE_READ_ONLY). Any suggestions for the proxy case?

Thanks!

	alexander :surkov (:asurkov)
	Comment 9 • 7 years ago

(In reply to Joanmarie Diggs from comment #8)
> (In reply to alexander :surkov from comment #4)
>  
> > role is not the cheapest call (in non-proxy case), and generally should be
> > avoided. It might be ok for now though. So it's up to you to use Is...()
> > methods instead or go with Role() call.
> 
> If it generally should be avoided, I'm happy to avoid it. :) And IsWidget()
> may be a better fit (non-widgets shouldn't expose ATK_STATE_READ_ONLY).

As long as it's safe to assume Gecko's internal widget term matches the one you need. It possibly is, no test coverage for ATK though. Alternatively you could implement those IsBla() on the proxy via cached mRole, but I agree IsWidget() would be nicer if it works.

> Any
> suggestions for the proxy case?

it's better to ask Aaron.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 10 • 7 years ago

(In reply to alexander :surkov from comment #9)
> (In reply to Joanmarie Diggs from comment #8)
> > (In reply to alexander :surkov from comment #4)
> > Any
> > suggestions for the proxy case?
> 
> it's better to ask Aaron.

I think this should be okay. Role is cached with the ProxyAccessible, so there's one IPC round-trip to retrieve the state.

	alexander :surkov (:asurkov)
	Comment 11 • 7 years ago

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Joanmarie Diggs from comment #8)
> > > (In reply to alexander :surkov from comment #4)
> > > Any
> > > suggestions for the proxy case?
> > 
> > it's better to ask Aaron.
> 
> I think this should be okay. Role is cached with the ProxyAccessible, so
> there's one IPC round-trip to retrieve the state.

Aaron, I meant if we can do ipc for Accessible::IsWidget [1], which isn't present on the proxy class, and whether this is a good idea at all.

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.h#827

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 12 • 7 years ago

This is something we should triage and fix but isn't among our most urgent bugs.(Contributions welcome as always!)