The default bug view has changed. See this FAQ.

Revise the custom control interface for XForms

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
9 months ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

12 years ago
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 :(
Created attachment 195010 [details] [diff] [review]
Allan's patch + nsIClassInfo

This seems to work.
Allan, have you been thinking some other changes to the CC API?
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
(Assignee)

Comment 3

12 years ago
(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.
(Assignee)

Comment 4

12 years ago
Created attachment 198302 [details] [diff] [review]
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?
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #198302 - Flags: review?(smaug)
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?
Attachment #198302 - Flags: review?(smaug) → review-

Comment 6

12 years ago
(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.
(Assignee)

Comment 7

12 years ago
(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.
(Assignee)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
Created attachment 199792 [details] [diff] [review]
New patch w/smaug's comments
Attachment #198302 - Attachment is obsolete: true
Attachment #199792 - Flags: review?(smaug)
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.
Attachment #199792 - Flags: review?(smaug) → review-
(Assignee)

Comment 11

12 years ago
(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.
(Assignee)

Comment 12

12 years ago
Created attachment 200339 [details] [diff] [review]
Using nsIDelegateInternal

This one implements nsIDelegateInternal (just like nsIXFormsModelElement and
nsIModelElementPrivate).
Attachment #195010 - Attachment is obsolete: true
Attachment #199792 - Attachment is obsolete: true
Attachment #200339 - Flags: review?(smaug)
(Assignee)

Updated

12 years ago
Blocks: 312956
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()?
Attachment #200339 - Flags: review?(smaug) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 200604 [details] [diff] [review]
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.
Attachment #200339 - Attachment is obsolete: true
Attachment #200604 - Flags: review?(smaug)
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
Attachment #200604 - Flags: review?(smaug)
Attachment #200604 - Flags: review?(aaronr)
Attachment #200604 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #200604 - Flags: review?(aaronr)
(Assignee)

Comment 16

12 years ago
Created attachment 200763 [details] [diff] [review]
New patch w/smaug's nits fixed
Attachment #200604 - Attachment is obsolete: true
Attachment #200763 - Flags: review?(aaronr)

Comment 17

12 years ago
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
Attachment #200763 - Flags: review?(aaronr) → review+
(Assignee)

Comment 18

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 20

12 years ago
(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 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.
(Assignee)

Comment 22

12 years ago
Created attachment 201910 [details] [diff] [review]
Updated to current CVS
Attachment #200763 - Attachment is obsolete: true
(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");
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?
(Assignee)

Comment 25

12 years ago
Created attachment 201933 [details] [diff] [review]
Working patch
Attachment #201910 - Attachment is obsolete: true
(Assignee)

Comment 26

12 years ago
Checked in to trunk
Whiteboard: xf-to-branch
(Assignee)

Updated

12 years ago
Depends on: 316840

Comment 27

11 years ago
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Comment 28

11 years ago
verified fixed in MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.