703202 - ARIA comboboxes don't fire value change events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Example: http://oaa-accessibility.org/example/10/

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Make fire value change events for ARIA comboboxes based on their children change. This doesn't affect on native comboboxes (XUL and HTML) because they have a combobox list child which is created within a combobox so not children mutation that can produce excess value change events.

The patch makes NVDA working but not JAWS. I contacted to FS to see what we can do here.

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 575103 [details] [diff] [review]
patch

Alex, do you still want me to review this, too?

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 575103 [details] [diff] [review]
patch

yes, please

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 575103 [details] [diff] [review]
patch

r=me. I think it's safe to do it this way.

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 575103 [details] [diff] [review]
patch

>+  PRUint32 hyperTextRole = mHyperText->Role();
>+  if (hyperTextRole == nsIAccessibleRole::ROLE_ENTRY ||
>+      hyperTextRole == nsIAccessibleRole::ROLE_COMBOBOX) {
>     nsRefPtr<AccEvent> valueChangeEvent =
>       new AccEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE, mHyperText,
>                    eAutoDetect, AccEvent::eRemoveDupes);
>     mDocument->FireDelayedAccessibleEvent(valueChangeEvent);
>   }

consider an inline func to get rid of the copying?

>   // Fire value change event.
>-  if (aContainer->Role() == nsIAccessibleRole::ROLE_ENTRY) {
>+  PRUint32 containerRole = aContainer->Role();
>+  if (containerRole == nsIAccessibleRole::ROLE_ENTRY ||
>+      containerRole == nsIAccessibleRole::ROLE_COMBOBOX) {
>     FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>                                aContainer->GetNode());
>   }

nit, lose the braces?

>

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> consider an inline func to get rid of the copying?

that's why I asked you for review but I hoped you will come with suggestions ;)
Ok, proper place is nsDocAccessible.h I think but I'm not sure about the name. Following the current template the name should be 'FireDelayedAccessibleValueChangeEventIfApplicable" which is incredible long

# we can get rid 'accessible' from name
# replace 'FireDelayed' on something else. Delayed event is not a concept we have, it's rather event notification because of event coalescence so that an event can be changed or dismissed at all. Maybe 'Process' will be good but it sounds too generic.
# 'IfApplicable' - can we shorten it?

> >+  if (containerRole == nsIAccessibleRole::ROLE_ENTRY ||
> >+      containerRole == nsIAccessibleRole::ROLE_COMBOBOX) {
> >     FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
> >                                aContainer->GetNode());
> >   }
> 
> nit, lose the braces?

when statement takes more than one lines I prefer to keep braces since it makes code more readable.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > consider an inline func to get rid of the copying?
> 
> that's why I asked you for review but I hoped you will come with suggestions
> ;)
> Ok, proper place is nsDocAccessible.h I think but I'm not sure about the
> name. Following the current template the name should be
> 'FireDelayedAccessibleValueChangeEventIfApplicable" which is incredible long

THE FIRST THING THAT CAME TO MY MIND WAS SOMETHING IN  tEXTuPDATOR.CPP LIKE

static inline void MaybeFireValueChange(nsHyperTextAccessible* aAcc)
{
if (blah)
  ireDelayedEvent(aAcc);
}

> # we can get rid 'accessible' from name
> # replace 'FireDelayed' on something else. Delayed event is not a concept we
> have, it's rather event notification because of event coalescence so that an
> event can be changed or dismissed at all. Maybe 'Process' will be good but
> it sounds too generic.
> # 'IfApplicable' - can we shorten it?

maybe?

how about MaybeNotifyOfValueChange() but that sounds sort of non deterministic

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 577213 [details] [diff] [review]
patch2


>-  if (aContainer->Role() == nsIAccessibleRole::ROLE_ENTRY) {
>-    FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
>-                               aContainer->GetNode());
>-  }
>+  MaybeNotifyOfValueChange(aContainer);

note this is a slight behavior change, I'm not sure if that was intended or not since the previous patch didn't do it.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 577213 [details] [diff] [review] [diff] [details] [review]
> patch2
> 
> 
> >-  if (aContainer->Role() == nsIAccessibleRole::ROLE_ENTRY) {
> >-    FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE,
> >-                               aContainer->GetNode());
> >-  }
> >+  MaybeNotifyOfValueChange(aContainer);
> 
> note this is a slight behavior change, I'm not sure if that was intended or
> not since the previous patch didn't do it.

eventually I'd like to fire event notifications against accessible so getting rid of event notifications for DOM nodes is a bonus as we go :)

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/eec3aca1b188