The default bug view has changed. See this FAQ.

ARIA comboboxes don't fire value change events

RESOLVED FIXED in mozilla11

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla11
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.03 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Example: http://oaa-accessibility.org/example/10/
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #575103 - Flags: review?(trev.saunders)

Comment 2

5 years ago
Comment on attachment 575103 [details] [diff] [review]
patch

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

Comment 3

5 years ago
Comment on attachment 575103 [details] [diff] [review]
patch

yes, please
Attachment #575103 - Flags: review?(marco.zehe)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 8

5 years ago
Created attachment 577213 [details] [diff] [review]
patch2
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+
(Assignee)

Comment 10

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.