Last Comment Bug 300248 - select needs to generate xforms-in-range and xforms-out-of-range events
: select needs to generate xforms-in-range and xforms-out-of-range events
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
: 338994 (view as bug list)
Depends on: 329935
Blocks: 278448 322255 326372 326373 338078
  Show dependency treegraph
 
Reported: 2005-07-10 00:48 PDT by aaronr
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (2.30 KB, application/xhtml+xml)
2005-07-10 00:50 PDT, aaronr
no flags Details
first pass (16.78 KB, patch)
2006-03-04 19:02 PST, aaronr
no flags Details | Diff | Splinter Review
second pass (17.93 KB, patch)
2006-03-04 20:24 PST, aaronr
no flags Details | Diff | Splinter Review
third pass (18.23 KB, patch)
2006-03-09 11:22 PST, aaronr
no flags Details | Diff | Splinter Review
testcase 2 (4.05 KB, application/xhtml+xml)
2006-04-21 15:06 PDT, aaronr
no flags Details
fourth pass (41.21 KB, patch)
2006-04-22 17:02 PDT, aaronr
no flags Details | Diff | Splinter Review
fifth pass (48.44 KB, patch)
2006-05-21 01:33 PDT, aaronr
no flags Details | Diff | Splinter Review
ready for review (47.95 KB, patch)
2006-05-22 00:27 PDT, aaronr
allan: review-
Details | Diff | Splinter Review
fixed comments (50.21 KB, patch)
2006-05-22 23:11 PDT, aaronr
allan: review+
doronr: review+
Details | Diff | Splinter Review

Description aaronr 2005-07-10 00:48:23 PDT
As mentioned in section 8.1.10 of the spec, "For closed selections: If the
initial instance value matches the storage value of one or more of the given
items, those items are selected. If there is no match, no items are initially
selected. If any selected values do not have a choice with a matching storage
value, the form control must indicate an out-of-range condition."

And as mentioned in 4.4.16 of the spec, "[xforms-in-range] is dispatched
whenever the value of an instance data node that was not possible to represent
given the constraints specified on a form control has changed such that the
value can now be represented by the form control."

So for select and select1, we need to generate these events.  When each is
xbl-ized, we need to remember to carry over this functionality.
Comment 1 aaronr 2005-07-10 00:50:09 PDT
Created attachment 188823 [details]
testcase
Comment 2 alexander :surkov 2005-10-24 22:50:59 PDT
How should event dispatching look?

var event=document.createEvent("Events");
event.initEvent("xforms-out-of-range", false, true);
select.dispatchEvent(event);

Is it right?
Comment 3 aaronr 2005-10-25 10:09:49 PDT
(In reply to comment #2)
> How should event dispatching look?
> 
> var event=document.createEvent("Events");
> event.initEvent("xforms-out-of-range", false, true);
> select.dispatchEvent(event);
> 
> Is it right?
> 

Almost.  event.initEvent("xforms-out-of-range", true, false);
Comment 4 Allan Beaufour 2005-10-25 10:48:39 PDT
I think we should have a nsIXFormsDelegate::setInRange(in boolean aState), so that the engine handles this.
Comment 5 alexander :surkov 2005-10-26 20:15:08 PDT
As I understand range events should be fired when control isn't able to show instance data and range events firing doesn't depend on data validation. If it is true then setInRange() looks not bad (like such event firing shortcut).
Comment 6 aaronr 2005-10-27 09:26:10 PDT
(In reply to comment #5)
> As I understand range events should be fired when control isn't able to show
> instance data and range events firing doesn't depend on data validation. If it
> is true then setInRange() looks not bad (like such event firing shortcut).
> 

Not only an event firing shortcut, but setInRange() can also make sure that the control is styled as 'out-of-range'.
Comment 7 Allan Beaufour 2005-10-27 12:27:23 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > As I understand range events should be fired when control isn't able to show
> > instance data and range events firing doesn't depend on data validation. If it
> > is true then setInRange() looks not bad (like such event firing shortcut).
> > 
> 
> Not only an event firing shortcut, but setInRange() can also make sure that the
> control is styled as 'out-of-range'.

Yes! That and a "setIsLive()" or something like that is needed on nsIXFormsAccessors
Comment 8 aaronr 2006-03-04 19:02:08 PST
Created attachment 214053 [details] [diff] [review]
first pass

This patch will work in most cases.  Left to do:

1) should probably keep the in/out of range state as a property on the xbl control so that we don't have to call into the accessor when we don't need to.
2) if a select and select1 have an initial value that is in-range, I am dispatching the in-range event.  Somehow refresh is getting called on these controls and I'm toggling out of range before I determine that it really should be in range.  Maybe we are getting a refresh before the bound node value is set?
3) styling isn't happening.
4) I need to run through my copy+out-of-range testcases.
Comment 9 aaronr 2006-03-04 20:24:01 PST
Created attachment 214057 [details] [diff] [review]
second pass

this patch addresses 1 & 2.  Ran against my copy tests.  Throwing out-of-range too often, so I'll have to track that down.  And styling still doesn't happen.  But this patch should work fine for any select and select1's w/o copy elements in them.
Comment 10 aaronr 2006-03-09 11:22:30 PST
Created attachment 214588 [details] [diff] [review]
third pass

this patch fixes the styling issues.  Now just have to fix the problems where generating a xforms-out-of-range incorrectly in the case of some copy testcases.  I think that this last issue will depend on a fix to bug 329935.
Comment 11 Steve Speicher 2006-03-27 07:51:27 PST
1.0 test suite 8.1.10.d
Comment 12 Allan Beaufour 2006-03-29 06:49:33 PST
Comment on attachment 214588 [details] [diff] [review]
third pass

>Index: nsIXFormsAccessors.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsAccessors.idl,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsIXFormsAccessors.idl
>--- nsIXFormsAccessors.idl	23 Feb 2006 23:53:20 -0000	1.3
>+++ nsIXFormsAccessors.idl	7 Mar 2006 01:52:00 -0000
>@@ -97,9 +97,33 @@ interface nsIXFormsAccessors : nsISuppor
>    *
>    * @param aNode         setContent will take the contents of aNode and copy
>    *                      them under the control's bound node.
>    * @param aForceUpdate  Indicates whether setContent should rebuild,
>    *                      recalculate, revalidate and refresh the model that
>    *                      this control is bound to prior to returning
>    */
>   void setContent(in nsIDOMNode aNode, in boolean aForceUpdate);
>+
>+  /**
>+   * @see http://www.w3.org/TR/xforms/slice4.html#evt-in-range
>+   */
>+  boolean isInRange();
>+
>+
>+  /**
>+   * Used to tell the XForms processor whether the control can represent all
>+   * of data in the bound node.  For example, if there are 3 different
>+   * space-seperated values in the instance data bound to a xf:select1 and the
>+   * select1 only has one item, then the data must be out of the range of
>+   * the control.
>+   *
>+   * @param aInRange      If false, the control should be styled as defined by
>+   *                      out-of-range pseudo-class.  If the control was
>+   *                      previously in-range, then the xforms-out-of-range
>+   *                      event should be dispatched.
>+   *                      If true, the control should be styled as defined by
>+   *                      in-range pseudo-class or the default style matching
>+   *                      the control.  If the data was previously out-of-range,
>+   *                      then the xforms-in-range event should be dispatched.
>+   */
>+  void setInRange(in boolean aInRange);

Not all controls can be in-range out-of-range. Only range and selects. So only those two should expose this.
Comment 13 aaronr 2006-03-29 10:54:27 PST
(In reply to comment #12)
> (From update of attachment 214588 [details] [diff] [review] [edit])
> >Index: nsIXFormsAccessors.idl
> >===================================================================
> >RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsAccessors.idl,v
> >retrieving revision 1.3
> >diff -u -8 -p -r1.3 nsIXFormsAccessors.idl
> >--- nsIXFormsAccessors.idl	23 Feb 2006 23:53:20 -0000	1.3
> >+++ nsIXFormsAccessors.idl	7 Mar 2006 01:52:00 -0000
> >@@ -97,9 +97,33 @@ interface nsIXFormsAccessors : nsISuppor
> >    *
> >    * @param aNode         setContent will take the contents of aNode and copy
> >    *                      them under the control's bound node.
> >    * @param aForceUpdate  Indicates whether setContent should rebuild,
> >    *                      recalculate, revalidate and refresh the model that
> >    *                      this control is bound to prior to returning
> >    */
> >   void setContent(in nsIDOMNode aNode, in boolean aForceUpdate);
> >+
> >+  /**
> >+   * @see http://www.w3.org/TR/xforms/slice4.html#evt-in-range
> >+   */
> >+  boolean isInRange();
> >+
> >+
> >+  /**
> >+   * Used to tell the XForms processor whether the control can represent all
> >+   * of data in the bound node.  For example, if there are 3 different
> >+   * space-seperated values in the instance data bound to a xf:select1 and the
> >+   * select1 only has one item, then the data must be out of the range of
> >+   * the control.
> >+   *
> >+   * @param aInRange      If false, the control should be styled as defined by
> >+   *                      out-of-range pseudo-class.  If the control was
> >+   *                      previously in-range, then the xforms-out-of-range
> >+   *                      event should be dispatched.
> >+   *                      If true, the control should be styled as defined by
> >+   *                      in-range pseudo-class or the default style matching
> >+   *                      the control.  If the data was previously out-of-range,
> >+   *                      then the xforms-in-range event should be dispatched.
> >+   */
> >+  void setInRange(in boolean aInRange);
> 
> Not all controls can be in-range out-of-range. Only range and selects. So only
> those two should expose this.
> 


I thought of that, but then I figured a custom control author might like to set/style his controls as out of range
Comment 14 Allan Beaufour 2006-03-29 11:28:40 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Not all controls can be in-range out-of-range. Only range and selects. So
> > only those two should expose this.
> 
> 
> I thought of that, but then I figured a custom control author might like to
> set/style his controls as out of range

Hmmm, the question is whether a general input can be in-range/out-of-range, it might be the cae. But a group or a repeat cannot. You then either need to be able to return "neither true or false", or limit the accessors-interface to controls that actually can be in-range/out-of-range. I'm pro the latter.

Remember to be "in par" with CSS 3 UI:
http://www.w3.org/TR/css3-ui/#pseudo-range
Comment 15 aaronr 2006-04-21 15:06:47 PDT
Created attachment 219369 [details]
testcase 2

this testcase worries more about eventing and makes sure select/select1 behave similarly to each other.
Comment 16 aaronr 2006-04-22 17:02:37 PDT
Created attachment 219468 [details] [diff] [review]
fourth pass

This patch works in conjunction with the patch from bug 329935.  I acutally built this patch with that patch to get this bug fixed, then removed the changes from 329935 so this patch might not build standalone.  Make sure that this patch gets re-sync'd and reviewed after patch for 329935 gets checked in.
Comment 17 aaronr 2006-05-21 01:33:29 PDT
Created attachment 222768 [details] [diff] [review]
fifth pass

I think this gets out all of the quirks I had.
Comment 18 aaronr 2006-05-22 00:27:18 PDT
Created attachment 222828 [details] [diff] [review]
ready for review

ready for review.  In particular, what do you think about the range condition accessor?
Comment 19 Allan Beaufour 2006-05-22 08:32:47 PDT
Comment on attachment 222828 [details] [diff] [review]
ready for review

> Index: nsXFormsAccessors.cpp
> ===================================================================
 
>  NS_IMETHODIMP
>  nsXFormsAccessors::IsValid(PRBool *aStateVal)
>  {
> -  return GetState(NS_EVENT_STATE_VALID, aStateVal);
> +  return nsXFormsUtils::GetState(mElement, NS_EVENT_STATE_VALID, aStateVal);
>  }

Why this move into nsXFormsUtils?
 
> Index: nsXFormsModelElement.cpp
> ===================================================================

> +  // This loop will only run once.  Just providing a scope for the
> +  // nsPostRefresh.  We want to make sure that nsPostRefresh's destructor
> +  // runs (and thus processes the postrefresh and containerpostrefresh lists)
> +  // before we clear the dispatch flags
> +  do {
> +    nsPostRefresh postRefresh = nsPostRefresh();
> +  
> +    if (!mDocumentLoaded) {
> +      return NS_OK;
> +    }
> +  
> +    // Kick off refreshing on root node
> +    nsresult rv = RefreshSubTree(mFormControls.FirstChild(), PR_FALSE);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } while (PR_FALSE);

No need for a do/while clause
"{
  ...
}"
is all it takes.

> Index: nsXFormsUtils.cpp
> ===================================================================

>  const PRInt32 kDefaultIntrinsicState =
>    NS_EVENT_STATE_ENABLED |
>    NS_EVENT_STATE_VALID |
>    NS_EVENT_STATE_OPTIONAL |
> -  NS_EVENT_STATE_MOZ_READWRITE;
> +  NS_EVENT_STATE_MOZ_READWRITE |
> +  NS_EVENT_STATE_INRANGE;
 
>  const PRInt32 kDisabledIntrinsicState =
>    NS_EVENT_STATE_DISABLED |
>    NS_EVENT_STATE_VALID |
>    NS_EVENT_STATE_OPTIONAL |
> -  NS_EVENT_STATE_MOZ_READWRITE;
> +  NS_EVENT_STATE_MOZ_READWRITE |
> +  NS_EVENT_STATE_INRANGE;

This is not correct. As I wrote in comment 14, we need to follow the CSS 3 UI specification. For example a group, can neither be in or out of range, so it should never match any of them
 
> Index: nsXFormsUtils.h
> ===================================================================

> +
> +  /**
> +   * Checks the status of the model item properties (readonly, disabled, etc.)

nit: not "disabled", "relevant"


> --- nsXFormsRangeConditionAccessors.cpp	1969-12-31 18:00:00.000000000 -0600

> @@ -0,0 +1,114 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This is not an IDL file, change the mode to C++.

> +NS_IMETHODIMP
> +nsXFormsRangeConditionAccessors::SetInRange(PRBool aInRange)
> +{
> +  PRBool currRange;
> +  IsInRange(&currRange);

check return value


> --- nsXFormsRangeConditionAccessors.h	1969-12-31 18:00:00.000000000 -0600

> +/**
> + * Implementation for the accessors for a range element,

range element?

> + * nsIXFormsRangeConditionAccessors.
> + *
> + */
> +class nsXFormsRangeConditionAccessors : public nsXFormsAccessors,
> +                                        public nsIXFormsRangeConditionAccessors
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_NSIXFORMSRANGECONDITIONACCESSORS
> +  NS_FORWARD_NSIXFORMSACCESSORS(nsXFormsAccessors::)
> +
> +  // Constructor
> +  nsXFormsRangeConditionAccessors(nsIDelegateInternal* aDelegate,
> +                                  nsIDOMElement* aElement)

"* " -> " *"

> +    : nsXFormsAccessors(aDelegate, aElement)
> +  {
> +  }
> +
> +  // nsIClassInfo overrides
> +  NS_IMETHOD GetInterfaces(PRUint32 *aCount, nsIID * **aArray);
> +
> +protected:

kill this.

> +
> +};

Except for nsXFormsUtils::GetState() and problems with CSS 3 UI it looks fine. But we need to adher to the spec.
Comment 20 aaronr 2006-05-22 17:55:27 PDT
(In reply to comment #19)
> (From update of attachment 222828 [details] [diff] [review] [edit])
> > Index: nsXFormsAccessors.cpp
> > ===================================================================
> 
> >  NS_IMETHODIMP
> >  nsXFormsAccessors::IsValid(PRBool *aStateVal)
> >  {
> > -  return GetState(NS_EVENT_STATE_VALID, aStateVal);
> > +  return nsXFormsUtils::GetState(mElement, NS_EVENT_STATE_VALID, aStateVal);
> >  }
> 
> Why this move into nsXFormsUtils?

Don't know what I was thinking.  maybe a remnant from an earlier approach.  I'll remove those changes in the next rev.

> > Index: nsXFormsUtils.cpp
> > ===================================================================
> 
> >  const PRInt32 kDefaultIntrinsicState =
> >    NS_EVENT_STATE_ENABLED |
> >    NS_EVENT_STATE_VALID |
> >    NS_EVENT_STATE_OPTIONAL |
> > -  NS_EVENT_STATE_MOZ_READWRITE;
> > +  NS_EVENT_STATE_MOZ_READWRITE |
> > +  NS_EVENT_STATE_INRANGE;
> 
> >  const PRInt32 kDisabledIntrinsicState =
> >    NS_EVENT_STATE_DISABLED |
> >    NS_EVENT_STATE_VALID |
> >    NS_EVENT_STATE_OPTIONAL |
> > -  NS_EVENT_STATE_MOZ_READWRITE;
> > +  NS_EVENT_STATE_MOZ_READWRITE |
> > +  NS_EVENT_STATE_INRANGE;
> 
> This is not correct. As I wrote in comment 14, we need to follow the CSS 3 UI
> specification. For example a group, can neither be in or out of range, so it
> should never match any of them

I read that part of the CSS 3 UI spec and I just get more confused.  So if something is a select, select1 or range, if it is not out of range then is it in range?  The CSS 3 UI spec makes it sound like in range and out of range only applies if there is bound data AND it has limiting values.  So if these are not bound or don't contain any items, then they are neither in range nor out of range?

For now I'll assume that any of these three that aren't out of range are in range unless someone feels differently about it.  I'll probably add a method to nsIXFormsControls so that we can query the default intrinsic state for each control rather than using those kXxxx constants.  SetStates gets called too often to test for the element's localname every time.
Comment 21 aaronr 2006-05-22 23:11:48 PDT
Created attachment 222997 [details] [diff] [review]
fixed comments

fixed comments.  Removed my changes to nsxformsutils.  Added defaultIntrinsicState and disabledIntrinsicState to nsIXFormsControls interface to return kDefaultIntrinsicState and kDisabledIntrinsicState for most controls.  I override them in range, select and select1 to append on NS_EVENT_STATE_INRANGE.
Comment 22 Allan Beaufour 2006-05-23 08:12:47 PDT
Comment on attachment 222997 [details] [diff] [review]
fixed comments

nit: split the long line that jst-review complains about:
nsXFormsRangeConditionAccessors::GetInterfaces(PRUint32 *aCount, nsIID * **aArray)

r=me
Comment 23 Allan Beaufour 2006-05-23 09:45:30 PDT
*** Bug 338994 has been marked as a duplicate of this bug. ***
Comment 24 aaronr 2006-05-23 13:59:56 PDT
Comment on attachment 222997 [details] [diff] [review]
fixed comments

I will fix Allan's comment in the pre-checkin patch.  Do you see any other problems Doron?
Comment 25 Doron Rosenberg (IBM) 2006-05-23 14:38:23 PDT
Comment on attachment 222997 [details] [diff] [review]
fixed comments

>diff -u -8 -p -r1.121 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	22 May 2006 09:06:14 -0000	1.121
>+++ nsXFormsModelElement.cpp	23 May 2006 06:04:30 -0000
>+  // Using brackets here to provide a scope for the
>+  // nsPostRefresh.  We want to make sure that nsPostRefresh's destructor
>+  // runs (and thus processes the postrefresh and containerpostrefresh lists)
>+  // before we clear the dispatch flags
>+  {
>+    nsPostRefresh postRefresh = nsPostRefresh();
>+  
>+    if (!mDocumentLoaded) {
>+      return NS_OK;
>+    }
>+  
>+    // Kick off refreshing on root node
>+    nsresult rv = RefreshSubTree(mFormControls.FirstChild(), PR_FALSE);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

ugly, but it'll do.
Comment 26 aaronr 2006-05-23 21:51:18 PDT
fixed the one line and checked into trunk.  Sorry, I forgot to diff again before checkin.

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