Last Comment Bug 351067 - fire event for xforms:range when value is changed and turn it into accessibility event
: fire event for xforms:range when value is changed and turn it into accessibil...
Status: RESOLVED FIXED
: access, fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: xformsa11y
  Show dependency treegraph
 
Reported: 2006-09-01 12:16 PDT by alexander :surkov
Modified: 2007-04-23 15:46 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first try (5.36 KB, patch)
2006-10-01 11:01 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (7.23 KB, patch)
2006-10-17 07:31 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (7.96 KB, patch)
2006-10-17 09:11 PDT, alexander :surkov
aaronlev: review-
Details | Diff | Splinter Review
patch3 (7.66 KB, patch)
2006-10-18 06:55 PDT, alexander :surkov
aaronlev: review+
neil: superreview-
Details | Diff | Splinter Review
patch4 (7.84 KB, patch)
2006-10-18 22:07 PDT, alexander :surkov
aaronr: review+
neil: superreview+
Details | Diff | Splinter Review
patch5 (8.55 KB, patch)
2006-10-26 22:56 PDT, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
1.8 patch (3.46 KB, patch)
2006-10-27 11:59 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2006-09-01 12:16:06 PDT
Fire event for xforms:range when value is changed and turn it into accessibility event.
Comment 1 alexander :surkov 2006-10-01 11:01:01 PDT
Created attachment 240830 [details] [diff] [review]
first try
Comment 2 Aaron Leventhal 2006-10-01 17:18:14 PDT
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 2006-10-01 23:08:45 PDT
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 2006-10-02 06:30:01 PDT
(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 2006-10-02 12:19:41 PDT
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 2006-10-02 23:10:00 PDT
(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 2006-10-03 04:33:53 PDT
Right.
Comment 8 alexander :surkov 2006-10-03 05:23:09 PDT
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 2006-10-16 22:44:34 PDT
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 2006-10-17 07:31:06 PDT
Created attachment 242500 [details] [diff] [review]
patch
Comment 11 alexander :surkov 2006-10-17 09:11:49 PDT
Created attachment 242510 [details] [diff] [review]
patch2

'widget-value-changed' event is handled as well as 'CheckboxStateChange' for example.
Comment 12 Aaron Leventhal 2006-10-17 12:25:06 PDT
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 2006-10-17 13:20:40 PDT
(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 2006-10-17 14:01:22 PDT
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 2006-10-18 06:55:22 PDT
Created attachment 242630 [details] [diff] [review]
patch3
Comment 16 neil@parkwaycc.co.uk 2006-10-18 15:33:44 PDT
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 2006-10-18 22:07:24 PDT
Created attachment 242728 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2006-10-19 05:23:18 PDT
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 2006-10-19 06:28:11 PDT
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 2006-10-26 18:00:12 PDT
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 2006-10-26 22:56:17 PDT
Created attachment 243751 [details] [diff] [review]
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 2006-10-27 11:29:13 PDT
(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 2006-10-27 11:54:17 PDT
checked into trunk for surkov
Comment 24 alexander :surkov 2006-10-27 11:59:05 PDT
Created attachment 243822 [details] [diff] [review]
1.8 patch
Comment 25 aaronr 2007-04-23 15:46:09 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.