306764 - Revise the custom control interface for XForms

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

There are some cross-platform/processor problems with the interface we currently
have for Custom Constrols:
interface nsIXFormsDelegate : nsISupports
{
  attribute DOMString value;
  readonly attribute boolean isReadonly;
  readonly attribute boolean isEnabled;
  readonly attribute boolean isRequired;
  readonly attribute boolean isValid;
  readonly attribute boolean hasBoundNode;
  void widgetAttached();
}

1. Name clashes

We could quite easily get name clashes with existing attributes, depending on
what element the current interface is exposed on. If somebody extends an already
existing input control, there is a good chance that ".value" already exists. The
could also apply to f.x. isReadonly.

2. Using attributes

If somebody wants to implement the controls in pure javascript, I've been told
that some browsers do not let you "watch" attribute changes, so it would not be
possibly to implement ".value".

To fix problem 2) we need to only use functions. To fix problem 1), I have two
possible solutions:
A) Use a "context object" like canvas does. That is, have one
.getXFormsAccessors() function that returns a new object where the above
interface is implemented.

B) Prefix every function with f.x. "form". So we would have ".formIsReadonly()".

I like A) best, but the problem is that when I tried to implement it, I hit some
security error with XBL when I tried to return the object :(

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Allan's patch + nsIClassInfo

This seems to work.
Allan, have you been thinking some other changes to the CC API?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 195010 [details] [diff] [review]
Allan's patch + nsIClassInfo

>+NS_IMETHODIMP 
>+nsXFormsAccessors::GetImplementationLanguage(PRUint32 *aImplementationLanguage)
>+{
>+  *aImplementationLanguage = nsIProgrammingLanguage::UNKNOWN;
>+  return NS_OK;
>+}
This could be nsIProgrammingLanguage::CPLUSPLUS

Comment 3

[Allan Beaufour]

(In reply to comment #1)
> Created an attachment (id=195010) [edit]
> Allan's patch + nsIClassInfo
> 
> This seems to work.

Thanks a million Olli.

> Allan, have you been thinking some other changes to the CC API?

I've got some thoughts. I'll gather them, and post them in a not to far a
distant future ;-)

I've gotten the action item for writing this stuff up for the XForms WG.

Comment 4

[Allan Beaufour]

Attachment: New patch

Here's an edition that's up-to-date. I've also changed everything to use
functions, and not attributes as cross-browser implementations in javascript
will have problems with watching attribute changes.

What do you say to it? I think this should be close to the "final" version.
Possible changes:
* using .getFeature(...) instead of .getXFormsAccessors()
* renaming and/or moving .widgetAttached() to nsIXFormsAccessors()

What's your take?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 198302 [details] [diff] [review]
New patch


> interface nsIXFormsDelegate : nsISupports
> {
....
>-  readonly attribute boolean isRequired;
>+  [noscript] attribute DOMString value;
>

I don't like this [noscript] thing here, but I don't have
any better suggestion atm

>   /**
>-   * Tells whether the XForms control is valid.
>+   * Is the delegate bound to a node?
>    */
>-  readonly attribute boolean isValid;
>+  [noscript] readonly attribute boolean hasBoundNode;
> 

Is the [noscript] hasBoundNode needed? One can always check 
nsIXFormsControl::boundNode.



>+
>+NS_IMETHODIMP
>+nsXFormsAccessors::GetImplementationLanguage(PRUint32 *aImplementationLanguage)
>+{
>+  *aImplementationLanguage = nsIProgrammingLanguage::UNKNOWN;

nsIProgrammingLanguage::CPLUSPLUS


>+
>+  // nsIXFormsAccessors
>+  NS_IMETHOD GetValue(nsAString &aValue)
>+  {
>+    return mDelegate ? mDelegate->GetValue(aValue) : NS_OK;
>+  }

I think aValue should be set null if there is no mDelegate.


>+  NS_IMETHOD HasBoundNode(PRBool *aHasBoundNode)
>+  {
>+    return mDelegate ? mDelegate->GetHasBoundNode(aHasBoundNode) : NS_OK;
>+  }

I'd set *aHasBoundNode to PR_FALSE by default.


> 
>+
>       <method name="dispatchDOMActivate">

Extra newline.


So how was it, did you discuss with other implementators?
Will it be possible to get something which works in more than just
one XForms implementation?

Comment 6

[aaronr]

(In reply to comment #5)
> (From update of attachment 198302 [details] [diff] [review] [edit])
> 
> > interface nsIXFormsDelegate : nsISupports
> > {
> ....
> >-  readonly attribute boolean isRequired;
> >+  [noscript] attribute DOMString value;
> >
> 
> I don't like this [noscript] thing here, but I don't have
> any better suggestion atm
> 
> >   /**
> >-   * Tells whether the XForms control is valid.
> >+   * Is the delegate bound to a node?
> >    */
> >-  readonly attribute boolean isValid;
> >+  [noscript] readonly attribute boolean hasBoundNode;
> > 
> 
> Is the [noscript] hasBoundNode needed? One can always check 
> nsIXFormsControl::boundNode.
> 
> 

to implement "copy" (which I realize you guys haven't seen, yet), I added
content to the delegate interface so that it was possible to get/set the content
of the bound node to be elements and not just a string.  I also added a way to
get the bound node and a way to get the DOMElement that the control was
mirroring.  I guess that the latter two would go under Accessors?  But as far as
content, we really need to keep in mind that a string for value alone isn't enough.

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> to implement "copy" (which I realize you guys haven't seen, yet),

We are waiting eagerly :)

> I added
> content to the delegate interface so that it was possible to get/set the content
> of the bound node to be elements and not just a string.  I also added a way to
> get the bound node and a way to get the DOMElement that the control was
> mirroring.  I guess that the latter two would go under Accessors?  But as far as
> content, we really need to keep in mind that a string for value alone isn't
enough.

I think I need to see the patch before answering that.

You are doing copy in XBL/JS? I'm not quite seeing why it should be done there,
and not in C++ -- but post a patch and let's take it on bug 279063.

Comment 8

[Allan Beaufour]

(In reply to comment #5)
> >   /**
> >-   * Tells whether the XForms control is valid.
> >+   * Is the delegate bound to a node?
> >    */
> >-  readonly attribute boolean isValid;
> >+  [noscript] readonly attribute boolean hasBoundNode;
> > 
> 
> Is the [noscript] hasBoundNode needed? One can always check 
> nsIXFormsControl::boundNode.

I'm not to happy about it either. Then I would need to QI to that. Should I do
that instead?

> So how was it, did you discuss with other implementators?
> Will it be possible to get something which works in more than just
> one XForms implementation?

We've discussed it internally at Novell, to find a solution that will work in
IE, our server side impl. and Mozilla. Me and another from the WG has the action
item to figure out something for the WG. My plan is to see it work in the three
mentioned processors, and get some experience with it, to add some practical
experience to the "definition" we will write.

Comment 9

[Allan Beaufour]

Attachment: New patch w/smaug's comments

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 199792 [details] [diff] [review]
New patch w/smaug's comments

>+[scriptable, uuid(5e75904d-73e8-4cee-b02c-4348a071a0e1)]
> interface nsIXFormsDelegate : nsISupports
> {
>   /**
>    * The value bound to the XForms control.
>    */
>-  attribute DOMString value;
>-
>-  /**
>-   * @see http://www.w3.org/TR/xforms/slice6.html#model-prop-readOnly
>-   */
>-  readonly attribute boolean isReadonly;
>-
>-  /**
>-   * @see http://www.w3.org/TR/xforms/slice6.html#model-prop-relevant
>-   */
>-  readonly attribute boolean isEnabled;
>-
>-  /**
>-   * @see http://www.w3.org/TR/xforms/slice6.html#model-prop-required
>-   */
>-  readonly attribute boolean isRequired;
>+  [noscript] attribute DOMString value;
> 
>   /**
>-   * Tells whether the XForms control is valid.
>+   * Is the delegate bound to a node?
>    */
>-  readonly attribute boolean isValid;
>+  [noscript] readonly attribute boolean hasBoundNode;
> 
>   /**
>-   * true, if XForms control is bound to a node in a data model.
>+   * Get the IXFormsAccessors object for this control.
>    */
>-  readonly attribute boolean hasBoundNode;
>+  nsIXFormsAccessors getXFormsAccessors();
> 
>   /**
>-   * This should be called by XBL widgets, when they are created.
>+   * This should be called by widgets, when they are created so the engine
>+   * knows when they are "live".
>    */
>   void widgetAttached();
> };

I still don't like [noscript] in this kind of 'public' API.
Could we have (not [scriptable]) nsIXFormsDelegateInternal interface.
I know we have already quite many interfaces but nsIXFormsDelegate and
nsIXFormsAccessors are something for content authors so those should be
as clea(n|r) as possible.
nsIXFormsDelegateInternal could extend nsIXFormsDelegate.
Comments?

marking r- at least for now.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (From update of attachment 199792 [details] [diff] [review] [edit])
> I still don't like [noscript] in this kind of 'public' API.
> Could we have (not [scriptable]) nsIXFormsDelegateInternal interface.
> I know we have already quite many interfaces but nsIXFormsDelegate and
> nsIXFormsAccessors are something for content authors so those should be
> as clea(n|r) as possible.
> nsIXFormsDelegateInternal could extend nsIXFormsDelegate.
> Comments?

I can only agree! Will do an internal version.

Comment 12

[Allan Beaufour]

Attachment: Using nsIDelegateInternal

This one implements nsIDelegateInternal (just like nsIXFormsModelElement and
nsIModelElementPrivate).

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200339 [details] [diff] [review]
Using nsIDelegateInternal

>+nsresult
>+nsXFormsAccessors::GetState(const nsAString &aState, PRBool *aStateVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aStateVal);
>+
>+  if (mElement) {
>+    return mElement->HasAttribute(aState, aStateVal);
>+  }
>+
>+  return NS_ERROR_NOT_AVAILABLE;
>+}
>+


Should you handle states now in a bit different way?
nsIContent::IntrinsicState()?

Comment 14

[Allan Beaufour]

Attachment: New patch

(In reply to comment #13)
> Should you handle states now in a bit different way?
> nsIContent::IntrinsicState()?

Who's that joker who checked that stuff in anyway *ahem* :) Yes, you are right. Here you go.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200604 [details] [diff] [review]
New patch

>+nsresult
>+nsXFormsAccessors::GetState(PRInt32 aState, PRBool *aStateVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aStateVal);
>+  *aStateVal = PR_FALSE;
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
>+  if (content && (content->IntrinsicState() & aState)) {
>+    *aStateVal = PR_TRUE;
>+  }  
>+  return NS_OK;
>+}


Nit, you could do:
nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
*aStateVal = (content && (content->IntrinsicState() & aState));



>+++ xforms.newcc/resources/content/xforms.xml	2005-10-24 13:37:14.000000000 +0200
>@@ -60,37 +60,50 @@
>     
>     <implementation implements="nsIXFormsUIWidget">
>       <constructor>
>         this.delegate.widgetAttached();
>       </constructor>
> 
>       <destructor>
>         this._delegate = null;
>+	this._accessors = null;
>       </destructor>

s/\t/        /;



With those r=me

Comment 16

[Allan Beaufour]

Attachment: New patch w/smaug's nits fixed

Comment 17

[aaronr]

Comment on attachment 200763 [details] [diff] [review]
New patch w/smaug's nits fixed


>diff -X patch-excludes -uprN -U8 xforms/nsXFormsAccessors.cpp xforms.newcc/nsXFormsAccessors.cpp
>--- xforms/nsXFormsAccessors.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.newcc/nsXFormsAccessors.cpp	2005-10-25 19:36:26.000000000 +0200

>+NS_IMETHODIMP
>+nsXFormsAccessors::HasBoundNode(PRBool *aHasBoundNode)
>+{
>+  *aHasBoundNode = PR_FALSE;
>+  return mDelegate ? mDelegate->GetHasBoundNode(aHasBoundNode) : NS_OK;
>+}

shouldn't you do NS_ENSURE_ARG_POINTER(aHasBoundNode) before assigning into it?


As far as the patch, that is all I found.  r=me

Comment 18

[Allan Beaufour]

(In reply to comment #17)
> (From update of attachment 200763 [details] [diff] [review] [edit])
> 
> >diff -X patch-excludes -uprN -U8 xforms/nsXFormsAccessors.cpp xforms.newcc/nsXFormsAccessors.cpp
> >--- xforms/nsXFormsAccessors.cpp	1970-01-01 01:00:00.000000000 +0100
> >+++ xforms.newcc/nsXFormsAccessors.cpp	2005-10-25 19:36:26.000000000 +0200
> 
> >+NS_IMETHODIMP
> >+nsXFormsAccessors::HasBoundNode(PRBool *aHasBoundNode)
> >+{
> >+  *aHasBoundNode = PR_FALSE;
> >+  return mDelegate ? mDelegate->GetHasBoundNode(aHasBoundNode) : NS_OK;
> >+}
> 
> shouldn't you do NS_ENSURE_ARG_POINTER(aHasBoundNode) before assigning into it?

Good point.

This patch might need a lot of merge-problems with bug 307627... so depending on the progress of that, we should possibly wait with this?

Comment 19

[aaronr]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 200763 [details] [diff] [review] [edit] [edit])
> > 
> > >diff -X patch-excludes -uprN -U8 xforms/nsXFormsAccessors.cpp xforms.newcc/nsXFormsAccessors.cpp
> > >--- xforms/nsXFormsAccessors.cpp	1970-01-01 01:00:00.000000000 +0100
> > >+++ xforms.newcc/nsXFormsAccessors.cpp	2005-10-25 19:36:26.000000000 +0200
> > 
> > >+NS_IMETHODIMP
> > >+nsXFormsAccessors::HasBoundNode(PRBool *aHasBoundNode)
> > >+{
> > >+  *aHasBoundNode = PR_FALSE;
> > >+  return mDelegate ? mDelegate->GetHasBoundNode(aHasBoundNode) : NS_OK;
> > >+}
> > 
> > shouldn't you do NS_ENSURE_ARG_POINTER(aHasBoundNode) before assigning into it?
> 
> Good point.
> 
> This patch might need a lot of merge-problems with bug 307627... so depending
> on the progress of that, we should possibly wait with this?
> 


This patch is ready to go.  307627 doesn't have a r, yet, so I'd say that is the one that should wait.  The sooner this is in, the sooner we can document to everyone how this should work.

Comment 20

[Allan Beaufour]

(In reply to comment #19)
> This patch is ready to go.  307627 doesn't have a r, yet, so I'd say that is
> the one that should wait.  The sooner this is in, the sooner we can document to
> everyone how this should work.

I was about to check it in, but now I get this error when trying to set a value through f.x. input:
JavaScript error: , line 0: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: http://localhost/xftst/test.xhtml :: onblur :: line 1"  data: no]

Sadness. What has happened? I'm _guessing_ that it's a checkin to mozilla/js (not by bclary)?
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=mozilla%2Fjs&file=&filetype=match&who=bob%25bclary.com&whotype=notregexp&sortby=Date&hours=2&date=explicit&mindate=2005-10-21&maxdate=2005-10-27&cvsroot=%2Fcvsroot

It could also be XTF.... any ideas?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200763 [details] [diff] [review]
New patch w/smaug's nits fixed

Allan, could you update this so that it could be tested with the latest trunk. I could try to debug the error you have seen.

Comment 22

[Allan Beaufour]

Attachment: Updated to current CVS

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #22)
> Created an attachment (id=201910) [edit]
> Updated to current CVS
> 

I think this won't work. It is using this.accessors.value = "something",
that should be this.accessors.setValue("something");

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

I fixed the patch (.value -> get/setValue()) locally, but I don't see the 
NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN error.
Everything seems to work fine
Allan, can you still reproduce the problem?

Comment 25

[Allan Beaufour]

Attachment: Working patch

Comment 26

[Allan Beaufour]

Checked in to trunk

Comment 27

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 28

[aaronr]

verified fixed in MOZILLA_1_8_BRANCH