301374 - controls show in wrong order

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

In the testcase I will attach, the checkbox should be before the select.  If the
checkbox hides and then shows the group that contains the select, then the
select is after the checkbox as it should be.

Comment 1

[aaronr]

Attachment: testcase

Comment 2

[aaronr]

Attachment: 'nother testcase

I think that this is related so I think that a patch to this bug will fix this
testcase, too.

Comment 3

[Doron Rosenberg (IBM)]

I'l working on xblizing group/case/switch to work around this xtf issue.

Comment 4

[Doron Rosenberg (IBM)]

Attachment: le patch

xblize switch/case/group.

Comment 5

[aaronr]

Comment on attachment 191257 [details] [diff] [review]
le patch

>Index: extensions/xforms/nsXFormsGroupElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsGroupElement.cpp,v
>retrieving revision 1.9
>diff -u -r1.9 nsXFormsGroupElement.cpp
>--- extensions/xforms/nsXFormsGroupElement.cpp	7 Jun 2005 07:14:31 -0000	1.9
>+++ extensions/xforms/nsXFormsGroupElement.cpp	1 Aug 2005 20:46:13 -0000
>-
>-PRBool
>-nsXFormsGroupElement::TryFocusChildControl(nsIDOMNode* aParent)
>-{
>-  if (!aParent)
>-    return PR_FALSE;
>-
>-  nsCOMPtr<nsIDOMNodeList> children;
>-  nsresult rv = aParent->GetChildNodes(getter_AddRefs(children));
>-  if (NS_FAILED(rv))
>-    return PR_FALSE;
>-
>-  PRUint32 childCount = 0;
>-  children->GetLength(&childCount);
>-  nsCOMPtr<nsIDOMNode> child;
>-
>-  for (PRUint32 i = 0; i < childCount; ++i) {
>-    children->Item(i, getter_AddRefs(child));
>-    nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(child));
>-    PRBool focus = PR_FALSE;
>-    if (control) {
>-      control->TryFocus(&focus);
>-      if (focus)
>-        return PR_TRUE;
>-    }
>-    focus = TryFocusChildControl(child);
>-    if (focus)
>-      return PR_TRUE;
>-  }
>-
>-  return PR_FALSE;
>-}
>-
>-NS_IMETHODIMP
>-nsXFormsGroupElement::TryFocus(PRBool* aOK)
>-{
>-  *aOK = PR_FALSE;
>-  if (GetRelevantState()) {
>-    *aOK = TryFocusChildControl(mElement);
>-  }
>-  return NS_OK;
>-}
>+};

You are removing the focus logic, but I don't see it in the XBL
anywhere.  Is this 'automatic' in XBL?

> 
> // Factory
> NS_HIDDEN_(nsresult)
>Index: extensions/xforms/nsXFormsSwitchElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSwitchElement.cpp,v
>retrieving revision 1.7
>diff -u -r1.7 nsXFormsSwitchElement.cpp
>--- extensions/xforms/nsXFormsSwitchElement.cpp	7 Jun 2005 07:14:31 -0000	1.7
>+++ extensions/xforms/nsXFormsSwitchElement.cpp	1 Aug 2005 20:46:13 -0000
>-
>-NS_IMETHODIMP
>-nsXFormsSwitchElement::WillRemoveChild(PRUint32 aIndex)
>-{
>-  if (!mAddingChildren) {
>-    nsCOMPtr<nsIDOMNodeList> list;
>-    mElement->GetChildNodes(getter_AddRefs(list));
>-    if (list) {
>-      nsCOMPtr<nsIDOMNode> child;
>-      list->Item(aIndex, getter_AddRefs(child));
>-      CaseChanged(child, PR_TRUE);
>-    }
>-  }
>-  return NS_OK;
>-}

I don't see this logic in the XBL.  Don't we still need it?

> 
>-NS_IMETHODIMP
>-nsXFormsSwitchElement::IsEventTarget(PRBool *aOK)
>-{
>-  *aOK = PR_FALSE;
>-  return NS_OK;
>-}

I know that we need this logic, still.	Some events are only meant for
'form controls' and this is what we use to decide whether a xforms
element is able to be the target of some of the xforms events that
are dispatched.


>Index: extensions/xforms/resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.5
>diff -u -r1.5 xforms.xml
>--- extensions/xforms/resources/content/xforms.xml	26 Jul 2005 20:29:13 -0000	1.5
>+++ extensions/xforms/resources/content/xforms.xml	1 Aug 2005 20:46:14 -0000
>@@ -401,4 +401,80 @@
>       </method>
>     </implementation>
>   </binding>
>+
>+
>+  <!-- GROUP: <DEFAULT> -->
>+  <!--  * @todo If a \<label\> is the first element child for \<group\> it is the
>+        * label for the entire group -->

Could you please add 'XXX' somewhere to this comment since that is also a good
keyword to
grep on to find todo's.

Comment 6

[Doron Rosenberg (IBM)]

Comment on attachment 191257 [details] [diff] [review]
le patch

Aaron is right, I removed too much.

Comment 7

[Doron Rosenberg (IBM)]

Attachment: put back some methods

Comment 8

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

Comment on attachment 191492 [details] [diff] [review]
put back some methods

>Index: extensions/xforms/nsIXFormsCaseElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsCaseElement.idl,v
>retrieving revision 1.1
>diff -u -r1.1 nsIXFormsCaseElement.idl
>--- extensions/xforms/nsIXFormsCaseElement.idl	15 Dec 2004 02:08:53 -0000	1.1
>+++ extensions/xforms/nsIXFormsCaseElement.idl	3 Aug 2005 15:46:05 -0000
>@@ -38,12 +38,10 @@
> 
> #include "nsISupports.idl"
> 
>-interface nsIDOMElement;
>-
> /**
>  * Interface implemented by the case element.
>  */

Add a comment that this is actually implemented by the XBL widget.

> public:
>-  nsXFormsSwitchElement() : mAddingChildren(PR_FALSE) {}

Is mAddingChildren used anywhere? If not remove it from the class.


>+
>+
>+  <!-- GROUP: <DEFAULT> -->
>+  <!--  * @todo If a \<label\> is the first element child for \<group\> it is the
>+        * label for the entire group -->
Doesn't it work if you just have <children include="label"/>?

>+  <binding id="xformswidget-group"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <html:div>

Is the html:div element needed? <group> is already (by default) display:block;

>+  <!-- SWITCH: <DEFAULT> -->
>+  <binding id="xformswidget-switch"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <html:div>

Is the html:div element needed? <swith> is already (by default) display:block;


>+        <children/>
>+      </html:div>
>+    </content>
>+
>+    <implementation implements="nsIXFormsUIWidget">
>+      <method name="refresh">
>+        <body>
>+          return true;
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>

no-op refresh is already implemented in #xformswidget-base


>+
>+  <!-- CASE: <DEFAULT> -->
>+  <binding id="xformswidget-case"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <html:div anonid="container" style="display:none">

Could you add a class attribute to html:div. Then it could be styled in the
forms.css. And there, I think, the display property for that class should
be display:inherit; The we should be able to have inline <switch><case>
-structures.
(I hope that works)

>+
>+      <method name="setSelected">
>+        <parameter name="aEnable"/>
>+        <body>
>+          if (aEnable) {
>+            this.container.style.display = "block";

this.container.style.display = "inherit";


>+      <method name="refresh">
>+        <body>
>+          return true;
>+        </body>
>+      </method>

Already in #xformswidget-base

Comment 9

[aaronr]

> >+  <!-- GROUP: <DEFAULT> -->
> >+  <!--  * @todo If a \<label\> is the first element child for \<group\> it
is the
> >+        * label for the entire group -->
> Doesn't it work if you just have <children include="label"/>?

I think that this comment is about making sure that the label for a group is set
apart/offset from group contents.  Otherwise it looks just like the group has an
xf:output child.  Needs to be offset from the other children.


> 
> >+  <binding id="xformswidget-group"
> >+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> >+    <content>
> >+      <html:div>
> 
> Is the html:div element needed? <group> is already (by default) display:block;
> 
> >+  <!-- SWITCH: <DEFAULT> -->
> >+  <binding id="xformswidget-switch"
> >+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> >+    <content>
> >+      <html:div>
> 
> Is the html:div element needed? <swith> is already (by default) display:block;
> 
> 

I like having these div children.  Since we won't have support for ::value
anytime soon, people who want to style their forms will need a child like div,
won't they?

Comment 10

[Doron Rosenberg (IBM)]

Attachment: comments addressed

mAddingChildren is used.
I am keeping the divs because people might want to style for now.

Comment 11

[Doron Rosenberg (IBM)]

Attachment: comments addressed

mAddingChildren is used.
I am keeping the divs because people might want to style for now.

Comment 12

[Doron Rosenberg (IBM)]

Attachment: with more comments addressed

Comment 13

[aaronr]

Comment on attachment 191632 [details] [diff] [review]
with more comments addressed

Am I missing something?  Where is the anonymous content for group?  Don't we
need it anymore?

Comment 14

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

(In reply to comment #13)
> (From update of attachment 191632 [details] [diff] [review] [edit])
> Am I missing something?  Where is the anonymous content for group?  Don't we
> need it anymore?
> 

<group> is anyway (normally) just a 'block', so no need to create a useless
binding for that. This way we can easily have also 'inline' etc. <group>s.
And when adding support for the group's label, then we can add also a binding 
for <group>; but for now it is not needed. 

Comment 15

[aaronr]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 191632 [details] [diff] [review] [edit] [edit])
> > Am I missing something?  Where is the anonymous content for group?  Don't we
> > need it anymore?
> > 
> 
> <group> is anyway (normally) just a 'block', so no need to create a useless
> binding for that. This way we can easily have also 'inline' etc. <group>s.
> And when adding support for the group's label, then we can add also a binding 
> for <group>; but for now it is not needed. 


As long as it will obey when a form author wants to make it a table through CSS,
then that works for me.

Comment 16

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

Comment on attachment 191632 [details] [diff] [review]
with more comments addressed

> 
>-class nsXFormsCaseElement : public nsIXFormsCaseElement,
>-                            public nsXFormsXMLVisualStub
>+class nsXFormsCaseElement : public nsXFormsDelegateStub

Terribly sorry, I should have noticed this earlier; 
Case element used to extend nsXFormsXMLVisualStub, which means
that it shouldn't now extend nsXFormsDelegateStub but nsXFormsBindableStub.

> {
> public:
>-  nsXFormsCaseElement();
>-
>   NS_DECL_ISUPPORTS_INHERITED
> 
>-  NS_IMETHOD OnCreated(nsIXTFXMLVisualWrapper *aWrapper);
>-  NS_IMETHOD OnDestroyed();
>-  NS_IMETHOD BeginAddingChildren();
>-  NS_IMETHOD DoneAddingChildren();
>-
>-  NS_IMETHOD GetVisualContent(nsIDOMElement **aContent);
>-  NS_IMETHOD GetInsertionPoint(nsIDOMElement **aPoint);
>-
>+  NS_IMETHOD OnCreated(nsIXTFBindableElementWrapper *aWrapper);
>   NS_IMETHOD AttributeSet(nsIAtom *aName, const nsAString &aNewValue);
>-
>-  NS_DECL_NSIXFORMSCASEELEMENT
>-
>-private:
>-  PRBool mDoneAddingChildren;
>-  nsIDOMElement* mElement;

You'll need mElement if this class extends nsXFormsBindableStub.

>+NS_IMPL_ISUPPORTS_INHERITED0(nsXFormsCaseElement,
>+                             nsXFormsDelegateStub)

And now not nsXFormsDelegateStub but nsXFormsBindableStub


> 
> NS_IMETHODIMP
>-nsXFormsCaseElement::OnCreated(nsIXTFXMLVisualWrapper *aWrapper)
>+nsXFormsCaseElement::OnCreated(nsIXTFBindableElementWrapper *aWrapper)
> {
>-  aWrapper->SetNotificationMask(nsIXTFElement::NOTIFY_ATTRIBUTE_SET |
>-                                nsIXTFElement::NOTIFY_BEGIN_ADDING_CHILDREN |
>-                                nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);
>-
>-  nsCOMPtr<nsIDOMElement> node;
>-  aWrapper->GetElementNode(getter_AddRefs(node));
>-  mElement = node;
>-  NS_ASSERTION(mElement, "Wrapper is not an nsIDOMElement, we'll crash soon");

...and the code above is needed (well maybe for notificationMask it is
enough to have NOTIFY_ATTRIBUTE_SET).


>-NS_IMETHODIMP
>-nsXFormsCaseElement::OnDestroyed()
>-{
>-  mElement = nsnull;
>-  mVisual = nsnull;
>-  return NS_OK;
>-}

And leave OnDestroyed where you set mElement = nsnull;

Comment 17

[Doron Rosenberg (IBM)]

(In reply to comment #16)
> (From update of attachment 191632 [details] [diff] [review] [edit])
> > 
> >-class nsXFormsCaseElement : public nsIXFormsCaseElement,
> >-                            public nsXFormsXMLVisualStub
> >+class nsXFormsCaseElement : public nsXFormsDelegateStub
> 
> Terribly sorry, I should have noticed this earlier; 
> Case element used to extend nsXFormsXMLVisualStub, which means
> that it shouldn't now extend nsXFormsDelegateStub but nsXFormsBindableStub.
> 

I know get:

JavaScript error: , line 0: uncaught exception: [Exception... "Component
returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"
 nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame ::
chrome://xforms/content/xforms.xml :: get_delegate :: line 71"  data: no]

for each case element

Comment 18

[Doron Rosenberg (IBM)]

also, switching case to display rather than visibility is causing issues, like
outputs not showing their values, since we display:none by default.  Probably
need to revert back to the old way.

Comment 19

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

Attachment: Without binding for <switch>

Doron, does this work with all your testcases?

This patch doesn't have any binding for <switch>, but the
content author can still apply one if needed.
Note, <case> binding doesn't implement nsIXFormsUIwidget but
only nsIXFormsCaseUIElement.

Comment 20

[Doron Rosenberg (IBM)]

(In reply to comment #19)
> Created an attachment (id=191913) [edit]
> Without binding for <switch>
> 
> Doron, does this work with all your testcases?
> 
> This patch doesn't have any binding for <switch>, but the
> content author can still apply one if needed.
> Note, <case> binding doesn't implement nsIXFormsUIwidget but
> only nsIXFormsCaseUIElement.

This fixes some minor issues I was seeing, good work :)  Works fine with all my
samples.

Comment 21

[Doron Rosenberg (IBM)]

Comment on attachment 191913 [details] [diff] [review]
Without binding for <switch>

>Index: nsXFormsCaseElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsCaseElement.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsCaseElement.cpp
>--- nsXFormsCaseElement.cpp	15 Dec 2004 02:08:14 -0000	1.1
>+++ nsXFormsCaseElement.cpp	7 Aug 2005 21:08:54 -0000
> NS_IMETHODIMP
> nsXFormsCaseElement::BeginAddingChildren()
> {
>   mDoneAddingChildren = PR_FALSE;
>   return NS_OK;
>@@ -160,21 +131,34 @@ nsXFormsCaseElement::AttributeSet(nsIAto
>   }
> 
>   return NS_OK;
> }

Do we really need mDoneAddingChildren?	Widget attached gets called after that.
Also, why bring back nsXFormsCaseElement::BeginAddingChildren and
DoneAddingChildren()?

Also, I see packed booleans again, do we really need them?

Comment 22

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

(In reply to comment #21)
> 
> Do we really need mDoneAddingChildren?	Widget attached gets called after that.


mDoneAddingChildren is needed because of the 
nsXFormsCaseElement::AttributeSet(...)
(Though, bug 302497 may change that.)
The idea is that <switch> actually initializes the <switch><case>, not the
first <case> which has |selected| attribute.


> Also, why bring back nsXFormsCaseElement::BeginAddingChildren and
> DoneAddingChildren()?

Because mDoneAddingChildren requires that.

> 
> Also, I see packed booleans again, do we really need them?
> 

There are two boolean members in the class, so why not use PRPackedBool,
it is not that slow ;) 
Using PRBool would increase the size of the class by 4 bytes.
And the boolean members are needed now (mDoneAddingChildren may disappears
when Bug 302497 gets fixed).

Comment 23

[aaronr]

Comment on attachment 191913 [details] [diff] [review]
Without binding for <switch>

seems to work fine in my testcases, even without explicit anon content for
group and switch. r=me

Comment 24

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

checked in