Joanmarie Diggs Reporter
	Description • 9 years ago

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.)

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 9 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 3 • 9 years ago

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?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 4 • 9 years ago

(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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 9 years ago

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...

	Trevor Saunders (:tbsaunde) Assignee
	Comment 6 • 9 years ago

(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.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 7 • 9 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 9 years ago

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.

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/72f55d458dd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3d821ef31b

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e714d10b5e

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b40b6c44fe97

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/72f55d458dd6
https://hg.mozilla.org/mozilla-central/rev/5a3d821ef31b
https://hg.mozilla.org/mozilla-central/rev/e1e714d10b5e
https://hg.mozilla.org/mozilla-central/rev/b40b6c44fe97