300248 - select needs to generate xforms-in-range and xforms-out-of-range events

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

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]

Attachment: testcase

Comment 2

[alexander :surkov (:asurkov)]

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]

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

I think we should have a nsIXFormsDelegate::setInRange(in boolean aState), so that the engine handles this.

Comment 5

[alexander :surkov (:asurkov)]

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]

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

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

Attachment: 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]

Attachment: 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]

Attachment: 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]

1.0 test suite 8.1.10.d

Comment 12

[Allan Beaufour]

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]

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

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

Attachment: testcase 2

this testcase worries more about eventing and makes sure select/select1 behave similarly to each other.

Comment 16

[aaronr]

Attachment: 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]

Attachment: fifth pass

I think this gets out all of the quirks I had.

Comment 18

[aaronr]

Attachment: ready for review

ready for review.  In particular, what do you think about the range condition accessor?

Comment 19

[Allan Beaufour]

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]

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

Attachment: 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]

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]

*** Bug 338994 has been marked as a duplicate of this bug. ***

Comment 24

[aaronr]

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

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]

fixed the one line and checked into trunk.  Sorry, I forgot to diff again before checkin.