Closed Bug 1213402 Opened 4 years ago Closed 4 years ago

No ATK/AT-SPI2 object:property-change:accessible-value events for web widgets in e10s

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: jdiggs, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Steps to reproduce:
1. View http://files.paciellogroup.com/blogmisc/samples/aria/slider/
2. Run the attached pyatspi accessible-event listener in a terminal
3. Change the value of the sliders on the page

Expected results: object:property-change:accessible-value events would be emitted each time the slider value is changed.

Actual results: Things work as expected if e10s is not enabled; if e10s is enabled, the events are absent.

Note: It seems like the value interface itself is working. You can, for instance, query the value interface of any of the sliders. It's just the events which are missing. (And it's the events which tell Orca that the value interface should be queried and the results presented to the user.)
Arguably these are different things, and it will be easier to proxy events for
atk this way because atk only wants the numeric value changes.
Attachment #8682146 - Flags: review?(dbolter)
Comment on attachment 8682148 [details] [diff] [review]
Fire numeric value change events for proxies

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

::: accessible/atk/AccessibleWrap.cpp
@@ +1476,5 @@
>      // A hack using state change showing events as alert events.
>      atk_object_notify_state_change(wrapper, ATK_STATE_SHOWING, true);
>      break;
> +  case nsIAccessibleEvent::EVENT_VALUE_CHANGE:
> +    g_object_notify((GObject*)wrapper, "accessible-value");

Why do you need the cast?
(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 8682148 [details] [diff] [review]
> Fire numeric value change events for proxies
> 
> Review of attachment 8682148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/atk/AccessibleWrap.cpp
> @@ +1476,5 @@
> >      // A hack using state change showing events as alert events.
> >      atk_object_notify_state_change(wrapper, ATK_STATE_SHOWING, true);
> >      break;
> > +  case nsIAccessibleEvent::EVENT_VALUE_CHANGE:
> > +    g_object_notify((GObject*)wrapper, "accessible-value");
> 
> Why do you need the cast?

because Glib is silly and tries to reimplement C++ in C and so you need to cast to upcast.
Assignee: nobody → tbsaunde+mozbugs
Comment on attachment 8682146 [details] [diff] [review]
separate value change events into text value changes and numeric value changes

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

ok

::: accessible/base/nsAccessibilityService.h
@@ +308,5 @@
>    "location change",                         // EVENT_LOCATION_CHANGE
>    "name changed",                            // EVENT_NAME_CHANGE
>    "description change",                      // EVENT_DESCRIPTION_CHANGE
>    "value change",                            // EVENT_VALUE_CHANGE
> +  "text value change",                            // EVENT_TEXT_VALUE_CHANGE

nit: Comment indented too far (5 spaces).

::: accessible/generic/RootAccessible.cpp
@@ +453,5 @@
>    else if (accessible->NeedsDOMUIEvent() &&
>             eventType.EqualsLiteral("ValueChange")) {
> +    uint32_t event = accessible->HasNumericValue()
> +      ? nsIAccessibleEvent::EVENT_VALUE_CHANGE
> +      : nsIAccessibleEvent::EVENT_TEXT_VALUE_CHANGE;

Hmm I guess we're ok never firing an EVENT_TEXT_VALUE_CHANGE if eHasNumericValue is set...

::: accessible/interfaces/nsIAccessibleEvent.idl
@@ +414,5 @@
>  
>    /**
> +   * An object's text Value has changed.
> +   */
> +  const unsigned long EVENT_TEXT_VALUE_CHANGE = 0x0057;

I guess we're ok getting out of order here...
Attachment #8682146 - Flags: review?(dbolter) → review+
Attachment #8682148 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #5)
> Comment on attachment 8682146 [details] [diff] [review]
> separate value change events into text value changes and numeric value
> changes
> 
> Review of attachment 8682146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok
> 
> ::: accessible/base/nsAccessibilityService.h
> @@ +308,5 @@
> >    "location change",                         // EVENT_LOCATION_CHANGE
> >    "name changed",                            // EVENT_NAME_CHANGE
> >    "description change",                      // EVENT_DESCRIPTION_CHANGE
> >    "value change",                            // EVENT_VALUE_CHANGE
> > +  "text value change",                            // EVENT_TEXT_VALUE_CHANGE
> 
> nit: Comment indented too far (5 spaces).
> 
> ::: accessible/generic/RootAccessible.cpp
> @@ +453,5 @@
> >    else if (accessible->NeedsDOMUIEvent() &&
> >             eventType.EqualsLiteral("ValueChange")) {
> > +    uint32_t event = accessible->HasNumericValue()
> > +      ? nsIAccessibleEvent::EVENT_VALUE_CHANGE
> > +      : nsIAccessibleEvent::EVENT_TEXT_VALUE_CHANGE;
> 
> Hmm I guess we're ok never firing an EVENT_TEXT_VALUE_CHANGE if
> eHasNumericValue is set...
> 
> ::: accessible/interfaces/nsIAccessibleEvent.idl
> @@ +414,5 @@
> >  
> >    /**
> > +   * An object's text Value has changed.
> > +   */
> > +  const unsigned long EVENT_TEXT_VALUE_CHANGE = 0x0057;
> 
> I guess we're ok getting out of order here...

no, that's totally bogus.  This needs to line up with the various arrays this patch also updates.
Arguably these are different things, and it will be easier to proxy events for
atk this way because atk only wants the numeric value changes.
Attachment #8685579 - Flags: review?(dbolter)
Comment on attachment 8685579 [details] [diff] [review]
separate value change events into text value changes and numeric value changes

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

Order looks good, just an indent nit.

::: accessible/base/nsAccessibilityService.h
@@ +384,5 @@
>    "hypertext changed",                       // EVENT_HYPERTEXT_CHANGED
>    "hypertext links count changed",           // EVENT_HYPERTEXT_NLINKS_CHANGED
>    "object attribute changed",                // EVENT_OBJECT_ATTRIBUTE_CHANGED
>    "virtual cursor changed"                   // EVENT_VIRTUALCURSOR_CHANGED
> +  "text value change",                            // EVENT_TEXT_VALUE_CHANGE

nit: comment indent is still too deep.
Attachment #8685579 - Flags: review?(dbolter) → review+
You need to log in before you can comment on or make changes to this bug.