351067 - fire event for xforms:range when value is changed and turn it into accessibility event

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Fire event for xforms:range when value is changed and turn it into accessibility event.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: first try

Comment 2

[Aaron Leventhal]

I think you could just fire a "ValueChange" event and get the same effect, because nsRootAccessible::HandleEventWithTarget() already handles those generically.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 240830 [details] [diff] [review]
first try

(In reply to comment #2)
> I think you could just fire a "ValueChange" event and get the same effect,
> because nsRootAccessible::HandleEventWithTarget() already handles those
> generically.
> 

I'm not sure I can. AaronR, what do you think?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 240830 [details] [diff] [review] [edit])
> (In reply to comment #2)
> > I think you could just fire a "ValueChange" event and get the same effect,
> > because nsRootAccessible::HandleEventWithTarget() already handles those
> > generically.
> > 

AaronL, should I handle event in nsRootAccessible instead of nsXFormsRangeAccessible in any way?

Comment 5

[Aaron Leventhal]

You shouldn't have to do any handling -- just fire ValueChange.
The handling of ValueChange and conversion into an nsIAccessibleEvent::EVENT_VALUE_CHANGE already happens here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#705

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> You shouldn't have to do any handling -- just fire ValueChange.
> The handling of ValueChange and conversion into an
> nsIAccessibleEvent::EVENT_VALUE_CHANGE already happens here:
> http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#705
> 

Yes, the question is another: can we use 'ValueChange' event inside xforms? I guess Aaron Reed can answer.


(In reply to comment #4)
> AaronL, should I handle event in nsRootAccessible instead of
> nsXFormsRangeAccessible in any way?
> 

If xforms can't fire 'ValueChange' event then should I handle xforms event rather in nsRootAccessible than nsXFormsRangeAccessible?

Comment 7

[Aaron Leventhal]

Right.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 240830 [details] [diff] [review]
first try

(In reply to comment #7)
> Right.
> 
So, I should modify the patch. But what event should xforms use?

Comment 9

[alexander :surkov (:asurkov)]

Talked with AaronR on irc. He said "actually, better call it widget-value-change or widget-changed". Alex, do you some thoughts about what event it's better to fire for xforms elements (especially for xforms range), I asked because probably we should have common event for xforms and wf2 stuffs.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

'widget-value-changed' event is handled as well as 'CheckboxStateChange' for example.

Comment 12

[Aaron Leventhal]

Comment on attachment 242510 [details] [diff] [review]
patch2

Can you put in a comment as to what the bool params mean that you pass to AddEventListener now? Any time I see a non-obvious PRBool getting passed in there should be a comment for it. Also comment why you want untrusted events? I undertstand, but it's useful since it's not immediately obvious.

I'd prefer this code goes away, and that any widget-value-changed events just fires valuechange. Are you sure it's firing both for Windows and Linux?

+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(accessible));
+    NS_ENSURE_TRUE(accessNode, NS_ERROR_FAILURE);
+
+    nsCOMPtr<nsIDOMNode> node;
+    accessNode->GetDOMNode(getter_AddRefs(node));
+    NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
+
+    nsCOMPtr<nsIAccessibleProvider> provider(do_QueryInterface(node));
+    NS_ENSURE_TRUE(provider, NS_ERROR_FAILURE);
+
+    PRInt32 type;
+    nsresult rv = provider->GetAccessibleType(&type);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    switch (type) {

BTW, why not just use ValueChanged since that already works? If it has to be "widget-value-changed" then we should change all the uses of ValueChange to that. We only need 1 event type for this.

Comment 13

[aaronr]

(In reply to comment #12)
> (From update of attachment 242510 [details] [diff] [review] [edit])

> BTW, why not just use ValueChanged since that already works? If it has to be
> "widget-value-changed" then we should change all the uses of ValueChange to
> that. We only need 1 event type for this.
> 

The way that I understand this accessibility stuff, every xforms element that implements the accessiblity interfaces will have this 'HandleEventTarget' event handler on them.  And HandleEventTarget has special handling for ValueChange events.  But it is also my understanding that not every accessible xforms control needs to handle ValueChange.  For example, input.  So if we send a ValueChange to an accessible element that doesn't need to handle ValueChange, then couldn't we end up causing problems?  That is why I suggested that we use a neutral event that would have no meaning to accessiblity.

But this is your specialty.  We should certainly defer to your judgement.

Comment 14

[Aaron Leventhal]

ValueChange is really only an a11y event. It's not used for anything else AFAIK. We invented it so we could fire toolkit value changed events on arbitrary elements. Since we only fire it when and where we want those there is no problem. We don't need extra code to see if it's in the right place since we only fire it for elements who's value has changed.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 16

[[e-mail]]

Comment on attachment 242630 [details] [diff] [review]
patch3

>   // use AddEventListener from the nsIDOMEventTarget interface
>   nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mDocument));
Except you don't, at least, not here... you immediately QI this to nstarget and never use it again. Instead you should move this declaration down to the GetChromeEventHandler call, and QI mDocument directly to nstarget here.

>+  else if (eventType.EqualsLiteral("ValueChange") ||
>+           eventType.LowerCaseEqualsLiteral("widget-value-changed")) {
Why LowerCaseEqualsLiteral on this one? (LowerCaseEqualsLiteral does a case insensitive comparison, and the parameter must be a lower case literal.)

>+          this.dispatchXFormsNotificationEvent("widget-value-changed", this);
>+
>           if (!this.accessors.hasBoundNode())
>             return;
> 
>           if (!aIncremental || this.incremental)
>             this.accessors.setValue(this.control.value);
Shouldn't you dispatch the event after the change (assuming it did change)?

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch4

(In reply to comment #16)
> (From update of attachment 242630 [details] [diff] [review] [edit])
> >   // use AddEventListener from the nsIDOMEventTarget interface
> >   nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mDocument));
> Except you don't, at least, not here... you immediately QI this to nstarget and
> never use it again. Instead you should move this declaration down to the
> GetChromeEventHandler call, and QI mDocument directly to nstarget here.

Right, fixed.

> >+  else if (eventType.EqualsLiteral("ValueChange") ||
> >+           eventType.LowerCaseEqualsLiteral("widget-value-changed")) {
> Why LowerCaseEqualsLiteral on this one? (LowerCaseEqualsLiteral does a case
> insensitive comparison, and the parameter must be a lower case literal.)

Actually, I think LowerCaseEqualsLiteral is not needed.

> >+          this.dispatchXFormsNotificationEvent("widget-value-changed", this);
> >+
> >           if (!this.accessors.hasBoundNode())
> >             return;
> > 
> >           if (!aIncremental || this.incremental)
> >             this.accessors.setValue(this.control.value);
> Shouldn't you dispatch the event after the change (assuming it did change)?
> 

Event is fired after than value of xforms range is changed. I can't see reason why whe should wait for update of bound node. If xforms author wants to know when bound node is updated then he should listen 'xforms-value-changed' event.

Comment 18

[[e-mail]]

Comment on attachment 242728 [details] [diff] [review]
patch4

As long as aaronr is OK with your explanation of when you fire the event.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 242728 [details] [diff] [review]
patch4

(In reply to comment #18)
> (From update of attachment 242728 [details] [diff] [review] [edit])
> As long as aaronr is OK with your explanation of when you fire the event.
> 

AaronR, let us know what do you think about event firing.

Comment 20

[aaronr]

Comment on attachment 242728 [details] [diff] [review]
patch4


>Index: accessible/src/base/nsRootAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v
>retrieving revision 1.173
>diff -u -8 -p -r1.173 nsRootAccessible.cpp
>--- accessible/src/base/nsRootAccessible.cpp	27 Sep 2006 12:56:28 -0000	1.173
>+++ accessible/src/base/nsRootAccessible.cpp	19 Oct 2006 05:03:13 -0000
>@@ -70,16 +70,17 @@
> #include "nsIScrollableView.h"
> #include "nsISelectionPrivate.h"
> #include "nsIServiceManager.h"
> #include "nsIViewManager.h"
> #include "nsLayoutAtoms.h"
> #include "nsPIDOMWindow.h"
> #include "nsReadableUtils.h"
> #include "nsRootAccessible.h"
>+#include "nsIDOMNSEventTarget.h"
> 
> #ifdef MOZ_XUL
> #include "nsXULTreeAccessible.h"
> #include "nsIXULDocument.h"
> #endif
> 
> // Expanded version of NS_IMPL_ISUPPORTS_INHERITED2 
> // so we can QI directly to concrete nsRootAccessible
>@@ -257,32 +258,40 @@ const char* const docEvents[] = {
>   // add ourself as a RadioStateChange Listener ( custom event fired in in nsHTMLInputElement.cpp  & radio.xml)
>   "RadioStateChange",
>   "popupshown",
>   "popuphiding",
>   "DOMMenuInactive",
>   "DOMMenuItemActive",
>   "DOMMenuBarActive",
>   "DOMMenuBarInactive",
>-  "DOMContentLoaded"
>+  "DOMContentLoaded",
>+  // is used by xforms widgets when widget's value was changed
>+  "widget-value-changed"
> };

Sorry, I didn't reply sooner.  AaronL's reply must have gone through my inbox and I missed it.  AaronL thinks "ValueChange" should be ok, so lets go with that.  Also saves adding an extra event listener.

> 
> nsresult nsRootAccessible::AddEventListeners()
> {
>-  // use AddEventListener from the nsIDOMEventTarget interface
>-  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mDocument));
>-  if (target) { 
>+  // nsIDOMNSEventTarget interface allows to register event listeners to
>+  // receive untrusted events (synthetic events generated by untrusted code).
>+  // For example, XBL bindings implementations for elements that are hosted in
>+  // non chrome document fire untrusted events.
>+  nsCOMPtr<nsIDOMNSEventTarget> nstarget(do_QueryInterface(mDocument));
>+
>+  if (nstarget) {
>     for (const char* const* e = docEvents,
>                    * const* e_end = docEvents + NS_ARRAY_LENGTH(docEvents);
>          e < e_end; ++e) {
>-      nsresult rv = target->AddEventListener(NS_ConvertASCIItoUTF16(*e), this, PR_TRUE);
>+      nsresult rv = nstarget->AddEventListener(NS_ConvertASCIItoUTF16(*e),
>+                                               this, PR_TRUE, PR_TRUE);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>   }
> 
>+  nsCOMPtr<nsIDOMEventTarget> target;
>   GetChromeEventHandler(getter_AddRefs(target));
>   if (target) {
>     target->AddEventListener(NS_LITERAL_STRING("pagehide"), this, PR_TRUE);
>   }
> 
>   if (!mCaretAccessible) {
>     mCaretAccessible = new nsCaretAccessible(mDOMNode, mWeakShell, this);
>     nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(mCaretAccessible));
>@@ -503,19 +512,18 @@ void nsRootAccessible::FireCurrentFocusE
>   }
> }
> 
> // --------------- nsIDOMEventListener Methods (3) ------------------------
> 
> NS_IMETHODIMP nsRootAccessible::HandleEvent(nsIDOMEvent* aEvent)
> {
>   // Turn DOM events in accessibility events
>-
>   // Get info about event and target
>-  nsCOMPtr<nsIDOMNode> targetNode; 
>+  nsCOMPtr<nsIDOMNode> targetNode;
>   GetTargetNode(aEvent, getter_AddRefs(targetNode));
>   if (!targetNode)
>     return NS_ERROR_FAILURE;
>   
>   return HandleEventWithTarget(aEvent, targetNode);
> }
> 
> /* virtual */
>@@ -697,17 +705,18 @@ nsresult nsRootAccessible::HandleEventWi
>       return NS_OK;
>     }
>     FireAccessibleFocusEvent(accessible, focusedItem, aEvent);
>   }
>   else if (eventType.LowerCaseEqualsLiteral("namechange")) {
>     privAcc->FireToolkitEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE,
>                               accessible, nsnull);
>   }
>-  else if (eventType.EqualsLiteral("ValueChange")) {
>+  else if (eventType.EqualsLiteral("ValueChange") ||
>+           eventType.EqualsLiteral("widget-value-changed")) {

won't need this anymore

>     PRUint32 role;
>     accessible->GetFinalRole(&role);
>     if (role == ROLE_PROGRESSBAR) {
>       // For progressmeter, fire EVENT_SHOW on 1st value change
>       nsAutoString value;
>       accessible->GetValue(value);
>       if (value.EqualsLiteral("0%")) {
>         privAcc->FireToolkitEvent(nsIAccessibleEvent::EVENT_SHOW, 
>Index: extensions/xforms/resources/content/range.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/range.xml,v
>retrieving revision 1.8
>diff -u -8 -p -r1.8 range.xml
>--- extensions/xforms/resources/content/range.xml	5 Oct 2006 23:29:34 -0000	1.8
>+++ extensions/xforms/resources/content/range.xml	19 Oct 2006 05:03:13 -0000
>@@ -151,16 +151,18 @@
>           return this._accessors;
>         ]]>
>         </getter>
>       </property>
> 
>       <method name="updateInstanceData">
>         <parameter name="aIncremental"/>
>         <body>
>+          this.dispatchXFormsNotificationEvent("widget-value-changed", this);
>+

change this to ValueChange per above.  But please add a comment here about why we are sending a "ValueChange" event and what mozilla uses it for.

Alex's explanation is a good one.  We need to send an accessibility "ValueChange" event when the slider's thumb position changes, which isn't necessarily when XForms dictates the bound node value should be changed.

But perhaps the best answer is to fire it from the slider widget instead of within XForms?  What do the other controls like input do?  Does html:input automatically fire a "ValueChange"?

r=me with the changes w.r.t. "ValueChange" that I've already mentioned.  And I don't mind if we fire a ValueChange here instead of the slider widget, as long as we are consistent with how the other xforms controls will do it.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch5

(In reply to comment #20)
> change this to ValueChange per above.  But please add a comment here about why
> we are sending a "ValueChange" event and what mozilla uses it for.
> 
> Alex's explanation is a good one.  We need to send an accessibility
> "ValueChange" event when the slider's thumb position changes, which isn't
> necessarily when XForms dictates the bound node value should be changed.
> 
> But perhaps the best answer is to fire it from the slider widget instead of
> within XForms?  What do the other controls like input do?  Does html:input
> automatically fire a "ValueChange"?

I guess yes.

> r=me with the changes w.r.t. "ValueChange" that I've already mentioned.  And I
> don't mind if we fire a ValueChange here instead of the slider widget, as long
> as we are consistent with how the other xforms controls will do it.
> 

Now slider widget fires "ValueChange" event. That's fine with me until we care about event that will be used by xforms author.

Comment 22

[aaronr]

(In reply to comment #21)
> Created an attachment (id=243751) [edit]
> patch5
> 
> (In reply to comment #20)

> 
> > r=me with the changes w.r.t. "ValueChange" that I've already mentioned.  And I
> > don't mind if we fire a ValueChange here instead of the slider widget, as long
> > as we are consistent with how the other xforms controls will do it.
> > 
> 
> Now slider widget fires "ValueChange" event. That's fine with me until we care
> about event that will be used by xforms author.
> 

Yes, we'll definitely have to be consistent about what event we use, across all of our xforms widgets.

Comment 23

[aaronr]

checked into trunk for surkov

Comment 24

[alexander :surkov (:asurkov)]

Attachment: 1.8 patch

Comment 25

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16