Closed
Bug 306764
Opened 19 years ago
Closed 19 years ago
Revise the custom control interface for XForms
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: allan, Assigned: allan)
References
()
Details
(Keywords: fixed1.8.0.2, fixed1.8.1)
Attachments
(1 file, 7 obsolete files)
30.60 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
This seems to work. Allan, have you been thinking some other changes to the CC API?
Comment 2•19 years ago
|
||
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•19 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•19 years ago
|
||
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•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•19 years ago
|
Attachment #198302 -
Flags: review?(smaug)
Comment 5•19 years ago
|
||
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-
(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•19 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•19 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•19 years ago
|
||
Attachment #198302 -
Attachment is obsolete: true
Attachment #199792 -
Flags: review?(smaug)
Comment 10•19 years ago
|
||
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•19 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•19 years ago
|
||
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)
Comment 13•19 years ago
|
||
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•19 years ago
|
||
(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 15•19 years ago
|
||
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•19 years ago
|
Attachment #200604 -
Flags: review?(aaronr)
Assignee | ||
Comment 16•19 years ago
|
||
Attachment #200604 -
Attachment is obsolete: true
Attachment #200763 -
Flags: review?(aaronr)
Comment 17•19 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•19 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•19 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•19 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 21•19 years ago
|
||
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 23•19 years ago
|
||
(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•19 years ago
|
||
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 27•19 years ago
|
||
checked into MOZILLA_1_8_BRANCH via bug 323691. Leaving open for now until it gets into 1.8.0
Assignee | ||
Updated•19 years ago
|
Whiteboard: xf-to-branch
Assignee | ||
Updated•19 years ago
|
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•