Closed Bug 703202 Opened 8 years ago Closed 8 years ago

ARIA comboboxes don't fire value change events

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #575103 - Flags: review?(trev.saunders)
Comment on attachment 575103 [details] [diff] [review]
patch

Alex, do you still want me to review this, too?
Comment on attachment 575103 [details] [diff] [review]
patch

yes, please
Attachment #575103 - Flags: review?(marco.zehe)
Comment on attachment 575103 [details] [diff] [review]
patch

r=me. I think it's safe to do it this way.
Attachment #575103 - Flags: review?(marco.zehe) → review+
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?

>
Attachment #575103 - Flags: review?(trev.saunders) → review+
(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.
(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
Attached patch patch2Splinter Review
Attachment #575103 - Attachment is obsolete: true
Attachment #577213 - Flags: review?(trev.saunders)
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.
Attachment #577213 - Flags: review?(trev.saunders) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/eec3aca1b188
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.