Last Comment Bug 703202 - ARIA comboboxes don't fire value change events
: ARIA comboboxes don't fire value change events
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks: oaabugs
  Show dependency treegraph
 
Reported: 2011-11-16 23:00 PST by alexander :surkov
Modified: 2011-12-01 12:11 PST (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.77 KB, patch)
2011-11-16 23:06 PST, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: review+
Details | Diff | Splinter Review
patch2 (7.03 KB, patch)
2011-11-28 03:33 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-11-16 23:00:36 PST
Example: http://oaa-accessibility.org/example/10/
Comment 1 alexander :surkov 2011-11-16 23:06:53 PST
Created attachment 575103 [details] [diff] [review]
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) 2011-11-16 23:42:56 PST
Comment on attachment 575103 [details] [diff] [review]
patch

Alex, do you still want me to review this, too?
Comment 3 alexander :surkov 2011-11-16 23:46:22 PST
Comment on attachment 575103 [details] [diff] [review]
patch

yes, please
Comment 4 Marco Zehe (:MarcoZ) 2011-11-17 00:34:20 PST
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) 2011-11-20 05:07:42 PST
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 2011-11-21 05:48:46 PST
(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) 2011-11-23 23:38:38 PST
(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 2011-11-28 03:33:10 PST
Created attachment 577213 [details] [diff] [review]
patch2
Comment 9 Trevor Saunders (:tbsaunde) 2011-11-29 19:50:31 PST
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 2011-11-29 20:01:51 PST
(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) 2011-12-01 11:41:35 PST
https://hg.mozilla.org/mozilla-central/rev/eec3aca1b188

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