Closed Bug 289434 Opened 16 years ago Closed 16 years ago

xbl-ized widgets for xforms

Categories

(Core Graveyard :: XForms, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andrew, Assigned: smaug)

References

Details

Attachments

(9 files, 14 obsolete files)

137.92 KB, patch
Details | Diff | Splinter Review
3.52 KB, text/xml
Details
1.42 KB, application/xhtml+xml
Details
1.39 KB, application/xhtml+xml
Details
658 bytes, application/xhtml+xml
Details
1.30 KB, application/xhtml+xml
Details
869 bytes, application/xhtml+xml
Details
88.40 KB, patch
Details | Diff | Splinter Review
92.96 KB, patch
allan
: review+
doronr
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305

We should use xbl for defining the visual content of the xforms widgets and
leave the xtf elements to just define the functionality (such as bindings,
etc.). See http://lab.cph.novell.com/~abeaufour/xforms/uidesign.html

Reproducible: Always
Attached file trigger test case. (obsolete) —
a bit of a hacked test case.. for some reason getElementById was causing me
problems.. not sure if it was just because it was late or if we have an actual
issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch second rough draft (obsolete) — Splinter Review
still just input, output and trigger. This is mostly cleanup, fixing where the
labels show up in the xbl contents, output value works (right now it's an ugly
refresh that should probably go into the delegate.. but I don't know where),
and the first attempt at getting context working. It works AFTER the first
refresh on the page.. so it's probably something is just not set up when it
first attempts to getcontext. Note: repeats still have issues so make sure to
test outside of a <repeat>
Suggestions for improvements welcome. I hope to get label xbl-ized next and
then deal with repeat.

Other issues to be wary of: ID and class have issues... also
psuedo-elements/classes. I'm sure there are a few others.
Attachment #179950 - Attachment is obsolete: true
Attachment #180015 - Attachment is obsolete: true
Attached patch update based on XTFBindable (obsolete) — Splinter Review
This is based on the patch for XTFBindableElement in bug 291026. This fixes
context issues, repeat issues, class/id issues and generally just works. Label
is also now xbl-ized. This is pretty much the final form but just requires some
cleanup and then I'll ask for review. If there are any major gotcha's, please
let me know.
-Andrew
Depends on: 291026
Attached file updated test case. (obsolete) —
We do apparently have an issue with user-defined xbl bindings. The
appearance="beaufour" isn't using the css defined -moz-binding. This may be an
issue with the XTFBindableElement patch.
Attachment #179951 - Attachment is obsolete: true
sorry for all the spam
Attachment #181437 - Attachment is obsolete: true
Attached patch Few fixes, cleaning up things (obsolete) — Splinter Review
Attachment #181445 - Attachment is obsolete: true
Attachment #181444 - Attachment is obsolete: true
(In reply to comment #8)
> Created an attachment (id=181662) [edit]
> Few fixes, cleaning up things

Why change all the return values from NS_IMETHODIMP to nsresult in
nsXFormsControlStubBase? Aren't they interface implementations?
(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=181662) [edit] [edit]
> > Few fixes, cleaning up things
> 
> Why change all the return values from NS_IMETHODIMP to nsresult in
> nsXFormsControlStubBase? Aren't they interface implementations?

nsXFormsControlStubBase does not inherit nsXFormsXMLVisualStub.
So the methods, which return nsresult aren't actually interface
implementations. And some methods do return NS_IMETHODIMP, those which
are from nsIXFormsControl.

This patch has a hack to delay the refreshing of the XBL widgets when they
are attached to the elements. Also try-catch is needed in many places in XBL
to ensure that we don't see unnecessary exceptions.
Attachment #181662 - Attachment is obsolete: true
Attachment #181804 - Attachment is obsolete: true
This might be almost the final patch. Now just trying to get the reviews for
Bug 291026
Assignee: aaronr → smaug
Attachment #181889 - Attachment is obsolete: true
Status: NEW → ASSIGNED
I applied the patch for 291026 on a build from 05/09 and then this patch on top
of it.  It won't build on Windows due to the call to SetClassAttributeName in
::Created  in nsXFormsControlStub.cpp on the aWrapper.  SetClassAttributeName
isn't a method on a nsIXTFElementWrapper, but rather a
nsIXTFStyledElementWrapper.  Shouldn't there be a QI before the call?  I also
had a problem with nsXFormsOutputElement::Bind calling
nsXFormsControlStub::Bind() directly since it now inherits from
nsXFormsControlStubBase.

Could be I screwed up when fixing up the diff, too. :=)  But could someone
please see if my errors were just my machine before this patch gets checked in?
 Thanks.
(In reply to comment #15)
> I applied the patch for 291026 on a build from 05/09 and then this patch on top
> of it.  
> ...
> Could be I screwed up when fixing up the diff, too. :=)  But could someone
> please see if my errors were just my machine before this patch gets checked in?
>  Thanks.

Sorry, this patch is not updated to work with the latest patch in bug 291026.
Attachment #181997 - Attachment is obsolete: true
Comment on attachment 183423 [details] [diff] [review]
works with the latest patch in bug 291026

Time to review? XTFB got the sr+
Attachment #183423 - Flags: review?(allan)
Here is the functionality that we currently have with XTF that we don't have
with this XBL patch (as far as I can tell, running through most of my
testcases).  Should we wait until they are addressed, or fix them all as
seperate bugs after this patch is in?

1) readonly='true()' MIP doesn't get reflected when specified on bindings to
inputs.  Input values can be changed.
2) no submit element
3) clicking/unclicking checkbox doesn't change bound data
4) removing binding attributes via script doesn't work well.
5) output elements contained inside repeat elements don't work (using the repeat
as the context for their value).
6) If I have an ephemeral message that contains a xhtml:input as a child, that
xhtml:input is not focusable.
7) setfocus element doesn't work
8) minimal trigger doesn't fire domactivates when clicked.
9) input with incremental="true" doesn't work.
(In reply to comment #19)
> Here is the functionality that we currently have with XTF that we don't have
> with this XBL patch (as far as I can tell, running through most of my
> testcases).  Should we wait until they are addressed, or fix them all as
> seperate bugs after this patch is in?

Thanks for testing it Aaron. I haven't look at the patch yet, but I think we
should try fixing your comments now. If some of them are really troublesome,
then yes, then let's create new bugs. But let's see if we cannot fix most of
them now
Attachment #183423 - Flags: review?(allan)
(In reply to comment #20)
> (In reply to comment #19)
> > Here is the functionality that we currently have with XTF that we don't have
> > with this XBL patch (as far as I can tell, running through most of my
> > testcases).  Should we wait until they are addressed, or fix them all as
> > seperate bugs after this patch is in?
> 
> Thanks for testing it Aaron. I haven't look at the patch yet, but I think we
> should try fixing your comments now. If some of them are really troublesome,
> then yes, then let's create new bugs. But let's see if we cannot fix most of
> them now

hmm, 5. was working, I think, apparently I have to re-test.
6. is weird test case ;)

I'll update the patch soon(ish).

thanks for testing Aaron.
Ok, since it looks like you'll try to fix it before checkin, better point you to
some testcase :=)  Didn't want to clog up the attachment list.  Most live in
bugzilla already, anyhow.

1) readonly input problem -
https://bugzilla.mozilla.org/attachment.cgi?id=173199.  Shouldn't be able to
edit the 'total' field.
2) submit button not implemented -
http://xformstest.org/mozilla/2005/02/03/test-get-1.xhtml.  Should see a
'submit' control following the input controls.
3) checkbox not updating bound data -
https://bugzilla.mozilla.org/attachment.cgi?id=174637  Checking and unchecking
the checkbox should change the value next to the submit control.
4) manipulating binding attrs via DOM -
https://bugzilla.mozilla.org/attachment.cgi?id=178989  Tough to explain in a
couple of sentences.  Use latest nightly next to a XBL patched build, you'll see
the differences in clicking the buttons.
5) repeat with outputs problem -
https://bugzilla.mozilla.org/attachment.cgi?id=168730  Should see the numbers
10, 30 and 45
6) ephemeral containing html:input - will attach testcase
7) set focus problem - I will attach testcase
8) minimal trigger - user error.  Seems to work when I try it now.  However,
won't generate a DOMActivate when clicking with the right mouse button with XBL
patch, but does with XTF.  Which is behavior is right?  testcase:
https://bugzilla.mozilla.org/attachment.cgi?id=175205
9) incremental input - https://bugzilla.mozilla.org/attachment.cgi?id=171769
Attached file setfocus testcase
testcase for setfocus.	Click on the trigger, input focus should move to the
input control
testcase for ephemeral message containing a html:input.  Click on the trigger,
you'll see some html appear.  click on the input field with the left mouse. 
Under XTF, this input will get input focus.  Under XBL, message will disappear.
I've fixed other test cases except 6, which is quite strange one and not
working well with XTF controls either.
In 8 the old behaviour is wrong.

Patch coming.
Attached patch for review (obsolete) — Splinter Review
Allan, maybe you could review this one too ;)
Attachment #183423 - Attachment is obsolete: true
Attachment #185422 - Flags: review?(allan)
(In reply to comment #17)
> Created an attachment (id=183423) [edit]
> works with the latest patch in bug 291026
> 
> testcase:
> http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml

Try this WITHOUT this patch, and switch between Normal and SVG twice... my build
crashes in layout/style/nsStyleContext.cpp... can you look at that Olli?
This one segfaults Mozilla...
This does not work with the patch. Feel free to ignore it for now, and create a
new bug though (just remember it :) ).
Attachment #185422 - Attachment is obsolete: true
Attachment #185422 - Flags: review?(allan)
Attached patch for review 2 (obsolete) — Splinter Review
The crash was (again) from the nsXFormsUtils::ReportError.
I changed the serialization so that we don't clone nodes anymore.
I think that has been a problem earlier too.

The dynamic addition of an <output> is not yet fixed :(
Attachment #185495 - Flags: review?(allan)
This fails. I listen for the xforms-ready event, which should only be fired
when all controls are initialized. So I would expect to be able to get the
correct value?
The output with "Node: 2" is not initialized correctly in attachment 181965 [details].
(In reply to comment #27)
> (In reply to comment #17)
> > Created an attachment (id=183423) [edit] [edit]
> > works with the latest patch in bug 291026
> > 
> > testcase:
> > http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml
> 
> Try this WITHOUT this patch, and switch between Normal and SVG twice... my build
> crashes in layout/style/nsStyleContext.cpp... can you look at that Olli?

Created bug 296927 for that.
Comment on attachment 185495 [details] [diff] [review]
for review 2

Besides comment 32, it looks pretty good.

Create new bugs for the dynamic and xforms-ready issues, and create an up to
patch and I'll do code review.
Attachment #185495 - Flags: review?(allan) → review-
Attached patch for review 3 (obsolete) — Splinter Review
up-to-date and comment #32 should be fixed.
Attachment #185495 - Attachment is obsolete: true
Attachment #185798 - Flags: review?(allan)
Comment on attachment 185798 [details] [diff] [review]
for review 3

> Index: nsXFormsLabelElement.cpp
> ===================================================================

>  /**
>   * Implementation of the XForms \<label\> element.
>   * This constructs an anonymous \<span\> to hold the inline content.
>   */

Fix comment.

> Index: nsXFormsModelElement.cpp
> ===================================================================

> +nsPostRefresh::~nsPostRefresh()
> +{
> +#ifdef DEBUG_smaug
> +  printf("~nsPostRefresh\n");
> +#endif
> +  --sRefreshing;
> +  if (sPostRefreshList && !sRefreshing) {
> +    while (sPostRefreshList->Count()) {
> +      PRInt32 last = sPostRefreshList->Count() - 1;
> +      nsIXFormsControl* control =
> +        NS_STATIC_CAST(nsIXFormsControl*, sPostRefreshList->ElementAt(last));
> +      sPostRefreshList->RemoveElementAt(last);
> +      if (control)
> +        control->Refresh();
> +    }

The reason why you keep refreshing the last element is that refresh can lead
to additions/deletions in sPostRefreshList, right? Add a note about it.

> +nsXFormsModelElement::NeedsPostRefresh(nsIXFormsControl* aControl)
> +{
> +  if (sRefreshing) {
> +    if (!sPostRefreshList) {
> +      sPostRefreshList = new nsVoidArray();
> +      if (!sPostRefreshList)
> +        return;

A warning please.

> Index: nsXFormsModelElement.h
> ===================================================================

> +/**
> + * nsPostRefresh is needed by the UI Controls, which are implemented in
> + * XBL and used inside \<repeat\>.
> + */

Yes, but what is the purpose of it?

> Index: nsXFormsOutputElement.cpp
> ===================================================================
>  nsXFormsOutputElement::Bind()
>  {
> -  if (!mValue || !mHasParent)
> -    return NS_OK;
> +  nsXFormsDelegateStub::Bind();
> +
> +  mBoundNode = nsnull;
> 
>    nsresult rv;
>    rv = mElement->HasAttribute(NS_LITERAL_STRING("ref"), &mHasBinding);
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (!mHasBinding) {
>      rv = mElement->HasAttribute(NS_LITERAL_STRING("bind"), &mHasBinding);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
> 
> +    nsCOMPtr<nsIDOMXPathResult> result;
>    if (mHasBinding) {
> -    rv = nsXFormsControlStub::Bind();
> +    rv = ProcessNodeBinding(NS_LITERAL_STRING("ref"),
> +                            nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
> +                            getter_AddRefs(result));
>    } else {
> -    mBoundNode = nsnull;
> -    // This call does a bit too much, as we do the call again in Refresh(),
> -    // but we need to clear index listeners, bind to the model, get
> -    // evt. context, etc. in Bind().
> -    rv = ResetBoundNode(NS_LITERAL_STRING("value"),
> -                        nsIDOMXPathResult::STRING_TYPE);
> -  }  
> +    rv = ProcessNodeBinding(NS_LITERAL_STRING("value"),
> +                            nsIDOMXPathResult::STRING_TYPE,
> +                            getter_AddRefs(result));
> +  }
> +  
>    NS_ENSURE_SUCCESS(rv, rv);

Why all the changes here? The first ::Bind() does a lot of work that is
canceled by setting mBoundNode = nsnull, and then redone when mHasBinding is
true. What is the problem with the original approach?

> -  return rv;
> +  if (result) {
> +    result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
> +  }
> +
> +  return NS_OK;
>  }

> Index: nsXFormsTriggerElement.cpp
> ===================================================================

> -NS_NewXFormsTriggerElement(nsIXTFElement **aResult)
> -{
> -  *aResult = new nsXFormsTriggerElement();
> -  if (!*aResult)
> -    return NS_ERROR_OUT_OF_MEMORY;
> +  nsCOMPtr<nsIXFormsUIWidget> widget(do_QueryInterface(mElement));
> +  if (widget) {
> +    widget->Disable(aDisable);
> +  }

Do we really need a special function for that. Can we not just set an
attribute as HTML controls do?

> Index: nsXFormsUtils.h
> ===================================================================

> +#define NS_NAMESPACE_MOZ_XFORMS_TYPE     "http://www.mozilla.org/projects/xforms/type"

Should we include a year?

> +++ nsIXFormsDelegate.idl	2005-06-09 17:37:48.000000000 +0300

> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.
2005

> +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();
> +};

Add documentation for each attribute and the function.

> +++ nsIXFormsUIWidget.idl	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +interface nsIXFormsUIWidget : nsIDOMElement
> +{
> +  void refresh();
> +  boolean focus();
> +  void attributeChanged(in AString name, in boolean removed);
> +  void disable(in boolean disable); // for <submit>
> +};

Again, documentation for each function.

> +++ nsXFormsDelegateStub.h	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +enum nsRepeatState {
> +  eType_Unknown,
> +  eType_Template,
> +  eType_GeneratedContent,
> +  eType_NotApplicable
> +};

Documentation.

And shouldn't this handling be in a class "closer" to nsIXFormsControl
instead? It should be valid for all controls.

> +protected:
> +  nsresult GetState(const nsAString &aState, PRBool *aStateVal);
> +  nsRepeatState UpdateRepeatState();
> +  nsString      mControlType;
> +  nsRepeatState mRepeatState;
> +};

Documentation.

> +++ nsXFormsDelegateStub.cpp	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +nsXFormsDelegateStub::AttributeSet(nsIAtom *aName, const nsAString &aNewValue)
> +{
> +  nsXFormsBindableControlStub::AttributeSet(aName, aNewValue);
> +
> +  nsCOMPtr<nsIXFormsUIWidget> widget(do_QueryInterface(mElement));
> +  if (widget && aName) {
> +    nsAutoString name;
> +    aName->ToString(name);
> +    widget->AttributeChanged(name, PR_FALSE);
> +  }

Why? Can this not be solved with inherited attributes in XBL? If not, include
a comment about it, here or in the interface. Interface I think.

In fact, you are not using it are you?

> +nsXFormsDelegateStub::Refresh()
> +{
> +  if (mRepeatState == eType_Template)
> +    return NS_OK;
> +
> +  NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
> +  NS_NAMED_LITERAL_STRING(mozType, "type");
> +
> +  if (mModel && mBoundNode) {
> +    nsAutoString type, ns;
> +    nsresult rv = mModel->GetTypeAndNSFromNode(mBoundNode, type, ns);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ns.AppendLiteral("#");
> +    ns.Append(type);
> +    mElement->SetAttributeNS(mozTypeNs, mozType, ns);
> +  } else {
> +    mElement->RemoveAttributeNS(mozTypeNs, mozType);
> +  }

Extract the type code, so it can be used by nsXFormsOutput too, instead of the
duplicated code.

> +nsXFormsDelegateStub::TryFocus(PRBool* aOK)
> +{
> +  nsresult rv = NS_OK;
> +  if (GetRelevantState()) {
> +    nsCOMPtr<nsIXFormsUIWidget> widget = do_QueryInterface(mElement);
> +    NS_ENSURE_STATE(widget);

Wouldn't it be ok to just return false instead of failing if there's no widget?

> +nsXFormsDelegateStub::GetValue(nsAString& aValue) 
> +{
> +  aValue = EmptyString();

Return null if there's no bound node?

> +nsXFormsDelegateStub::GetHasBoundNode(PRBool *aHasBoundNode)
> +{
> +  *aHasBoundNode = (mBoundNode != nsnull);

*aHasBoundNode = mBoundNode ? PR_TRUE : PR_FALSE;
instead to avoid explicit nsnull check?

> +++ xforms.xml	2005-06-09 17:37:48.000000000 +0300

Include the Mozilla copyright header.
Attachment #185798 - Flags: review?(allan) → review-
Attached patch for review 4Splinter Review
I wanted to keep void disable(in boolean disable) in nsIXFormsUIWidget.
Attachment #185798 - Attachment is obsolete: true
Attachment #186257 - Flags: review?(allan)
(In reply to comment #37)
> Created an attachment (id=186257) [edit]
> for review 4
> 
> I wanted to keep void disable(in boolean disable) in nsIXFormsUIWidget.

Why?
My version of the calculator that does not use table for layout fails to fresh
the controls when we change the appearance:
http://lab.cph.novell.com/~abeaufour/xforms/calc_svg.xhtml

(this is not new, it was in attachment 185798 [details] [diff] [review] too)
i don't think a warning for when you're running out of memory is particularly
useful. just deal with it.
Attachment #186257 - Flags: review?(allan)
Attachment #186847 - Flags: review?(allan)
Comment on attachment 186847 [details] [diff] [review]
for review 4 (up-to-date)

I don't remember now who is 
on vacation and when...but
maybe Doron could look at this too.
Attachment #186847 - Flags: review?(doronr)
(In reply to comment #42)
> (From update of attachment 186847 [details] [diff] [review] [edit])
> I don't remember now who is 
> on vacation and when...but
> maybe Doron could look at this too.
 
Sorry for the delay... I was in Amsterdam last week, etc. etc. But again, a
textual description of what you did from r3 to r4 would have been nice.
Why for example does focus() return a boolean?  It is always returned as true...
Comment on attachment 186847 [details] [diff] [review]
for review 4 (up-to-date)

* I will keep saying this: Please include a brief description of what has
  happened between revisions

* Nice with some good comments, including links!

* You seem to have fixed it all, as far as I can see. I haven't had time to
push it through all my tests, but not that much has changed, so I hope it still
works :)

>Index: nsXFormsModelElement.cpp
>===================================================================

>+nsPostRefresh::~nsPostRefresh()
>+{
>+#ifdef DEBUG_smaug
>+  printf("~nsPostRefresh\n");
>+#endif
>+  --sRefreshing;
>+  if (sPostRefreshList && !sRefreshing) {
>+    while (sPostRefreshList->Count()) {
>+      // Iterating from last to first saves possibly few memcopies,
>+      // see nsVoidArray::RemoveElementsAt().

But why do you iterate? You delete the list at the end of the function. [1]

>+      PRInt32 last = sPostRefreshList->Count() - 1;
>+      nsIXFormsControl* control =
>+        NS_STATIC_CAST(nsIXFormsControl*, sPostRefreshList->ElementAt(last));
>+      sPostRefreshList->RemoveElementAt(last);
>+      if (control)
>+        control->Refresh();
>+    }
>+    delete sPostRefreshList;
>+    sPostRefreshList = nsnull;
>+  }
>+}

>Index: nsXFormsDelegateStub.h
>===================================================================

>+enum nsRepeatState {
>+  eType_Unknown,
>+  eType_Template,
>+  eType_GeneratedContent,
>+  eType_NotApplicable
>+};

You didn't answer my question in comment 36:
And shouldn't this handling be in a class "closer" to nsIXFormsControl instead?
It should be valid for all controls.

But it's not terribly important, we can always move it.

r=me, with either a fix or a comment for [1] above.

And once again, sorry for the time it took me to get to this
Attachment #186847 - Flags: review?(allan) → review+
Attachment #186847 - Flags: review?(doronr) → review+
I won't have time to watch tinderbox before Sunday night,
but if anyone wants to check this in, please do so.

The comment Allan wants could be
      // Iterating this way because refresh can lead to
      // additions/deletions in sPostRefreshList.
(In reply to comment #46)
> I won't have time to watch tinderbox before Sunday night,
> but if anyone wants to check this in, please do so.

I do not have the time either :(

> The comment Allan wants could be
>       // Iterating this way because refresh can lead to
>       // additions/deletions in sPostRefreshList.

Exactly.
I have time today, will take care of it
checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.