271737 - Tracking bug for XForms Switch Module

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

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

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041122
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041122

.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

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

I'll try to do a patch today

Comment 2

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

Attachment: work-in-progress

Comment 3

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

Attachment: Adding support for nsIXFormsContextControl

Maybe something for a review ;)

I don't like copying code from nsXFormsGroupElement, we
should have a generic base class for ContextControl.
But that is a different bug.

Comment 4

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

There is still something wrong in the contextcontrol implementation.

Comment 5

[Allan Beaufour]

Comment on attachment 167270 [details] [diff] [review]
Adding support for nsIXFormsContextControl

+++ nsIXFormsSwitchElement.idl	2004-11-28 20:08:52.309329328 +0200
A comment for this interface, please.

+++ nsXFormsSwitchElement.cpp	2004-11-28 20:09:00.423095848 +0200
+/**
+ * Implementation of the XForms switch element.
Thanks for commenting! \<switch\> just for consistency.

+ * The implementation of the context control is based on
+ * nsXFormsGroupElement.

All elements that have single node binding need to support setting the
context, so I agree, we should have a stub for that.

+nsXFormsSwitchElement::Process()

+  if (model)
+    model->AddFormControl(this);
+
+  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);

result == nsnull can also mean that no binding attributes are present. We need
to distinguish between that and errors (see bug 272225).

+nsXFormsSwitchElement::FindFirstSelectableCase(nsIDOMElement* aDeselected)

Not exactly what it does is it? As far as I can see it either finds the first
selected case or the first case in the document if no cases are selected (and
ignores aDeselected). Possibly change the name, but either way add a comment
for the purpose of the function.

+nsXFormsSwitchElement::SetSelected(nsIDOMElement *aCase, PRBool aTrue)
The second half of the function could use a comment or two more, and I'm not
sure how it will behave in dynamic cases, where you insert new case's in the
DOM?

Errata E25, changes the wording from "attributes" to "states" for toggle. So
maybe the attributes should just be left alone?

In the spec. #9.2.2, it says 'If multiple cases within a switch are marked as
selected="true", the first selected case remains and all others are
deselected. If none are selected, the first becomes selected.' I do not know
how 'others are deselected', should be interpreted? It could be interpreted as
the attribute for all others are set to |false|. But if E25 means that the
attributes should be left alone, then your code's fine now.

In the spec #9.2.3, it says that @case is 'Required reference to a case
section inside the conditional construct.' I interpret this so that the
toggle can only toggle cases for its containing switch.

We have to solve the above issues, but besides that the code looks fine.

Comment 6

[Allan Beaufour]

(In reply to comment #4)
> There is still something wrong in the contextcontrol implementation.

What exactly? If it's the NS_ENSURE_TRUE stuff, see the above note for Process().

Comment 7

[Allan Beaufour]

Attachment: Testcase for focusproblems

There are some focus problems... possibly not your fault.

See this testcase. If the input field has focus, and you click it, the focus
remains in the, hidden, input field and you can still edit the field etc....

Comment 8

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

Actually I saw the focus bug too.
Perhaps the focus should move automatically to the
first form control in the navigation order in the selected <case>.
Should ask XForms WG?

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> Actually I saw the focus bug too.
> Perhaps the focus should move automatically to the
> first form control in the navigation order in the selected <case>.
> Should ask XForms WG?

David and I talked about it, and we think it makes sense to set the focus as you
say, _if_ the focus was inside the switch when the toggle happened.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Actually I saw the focus bug too.
> > Perhaps the focus should move automatically to the
> > first form control in the navigation order in the selected <case>.
> > Should ask XForms WG?
> 
> David and I talked about it, and we think it makes sense to set the focus as you
> say, _if_ the focus was inside the switch when the toggle happened.

Will do.

> 
> +nsXFormsSwitchElement::SetSelected(nsIDOMElement *aCase, PRBool aTrue)
> The second half of the function could use a comment or two more, and I'm not
> sure how it will behave in dynamic cases, where you insert new case's in the
> DOM?
> 

I'll test and fix possible bugs.

> 
> In the spec #9.2.3, it says that @case is 'Required reference to a case
> section inside the conditional construct.' I interpret this so that the
> toggle can only toggle cases for its containing switch.
> 

But according to the XForms's Schema toggle can be anywhere.
And actually I've seen one example where toggle was used inside <model>

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (In reply to comment #9)
> > In the spec #9.2.3, it says that @case is 'Required reference to a case
> > section inside the conditional construct.' I interpret this so that the
> > toggle can only toggle cases for its containing switch.
> 
> But according to the XForms's Schema toggle can be anywhere.
> And actually I've seen one example where toggle was used inside <model>

I think that is the intention too, but having my "nitpicker-glasses" on, I have
to conclude the above. But, let's ignore N. Itpick and let toggle move around
freely :)

Comment 12

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

Attachment: v2

This handles dynamic DOM modifications.

Focus change implemented.

Input element must now use nsCOMPtr<nsIDOMElement>, otherwise
there will be a segfault when document is being destroyed. Somehow
this problem is related to focusing elements. The segfault came
from nsXFormsInputElement::OnDestroyed(), actually from the
RemoveEventListener().

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> Created an attachment (id=167573)

I'll check it out later

> Input element must now use nsCOMPtr<nsIDOMElement>, otherwise
> there will be a segfault when document is being destroyed. Somehow
> this problem is related to focusing elements. The segfault came
> from nsXFormsInputElement::OnDestroyed(), actually from the
> RemoveEventListener().

I've noted that on another bug ... cannot seem to remember which. The elements
needs to keep a refcount.

Comment 14

[Allan Beaufour]

Comment on attachment 167573 [details] [diff] [review]
v2

I have not nitpicked as much as last patch, but here are my comments:

1) Patch conflicts with check in of bug 272523, but it is easily merged

2) nsXFormsSetSelected() still needs a comment before each |if| (except the
first), it's not evident what they do

3) Please put a comment on the nsIXFormsSwitchElement.idl interface, and
getSelected function in it.

4) I've talked with David about E25, and it basically says that attributes are
only used initially, to find the first selected case. So we do not need to
listen for attribute changes or change attributes accordingly. Less coding...

5) Using my focus test-case, something is wrong with the content. There is a
horisontal scroll bar, that should not be there when the input is shown.

I also have two test cases for bugs on the way

Comment 15

[Allan Beaufour]

Attachment: Two cases with selected=true

Setting @selected=true on multiple cases is not layouted properly, the content
of c2 seems to be layouted at initialization, even though it is not shown.

Comment 16

[Allan Beaufour]

Attachment: Nested switch'es

Nested switches also have layout problems. Try choosing 'Case 2'.

Comment 17

[Allan Beaufour]

(In reply to comment #12)
> Created an attachment (id=167573)
> v2

Forgot one more comment thing (I know I'm being annoying....), but could you put
your comments for the functions (in the definition of nsXFormsSwitch) inside /*
... */?

Comment 18

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

Attachment: v3

- needs patch from bug 272814
- added comments
- no longer modifies attribute values, but supports still dynamic dom
modifications
- possible layout problems can be solved for example using style rules
    @namespace xf url("http://www.w3.org/2002/xforms");
    xf|switch{display:block;}
    xf|case{display:block;}

Comment 19

[Allan Beaufour]

Comment on attachment 167756 [details] [diff] [review]
v3

Looks good.

r=me

Comment 20

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

up-to-date with CVS patch coming...

Comment 21

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

Attachment: up-to-date-with-cvs

Comment 22

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

Comment on attachment 168162 [details] [diff] [review]
up-to-date-with-cvs

No real changes to previous patch, so r=allan

Comment 23

[Darin Fisher]

Comment on attachment 167756 [details] [diff] [review]
v3

>Index: nsXFormsInputElement.cpp

>-  nsIDOMElement *mElement;
>+  nsCOMPtr<nsIDOMElement> mElement;

are you sure this does not cause a leak?  wouldn't it be better to
null out mElement in the OnDestroyed method, and then null check
mElement whereever it may be used post-OnDestroyed?  That solution
is what I used in nsXFormsInstanceElement to solve bug 267990.


>+++ nsIXFormsSwitchElement.idl 2004-12-03 22:22:18.928605792 +0200

>+   * @param aTrue             The value of the selected attribute.
>+   */
>+  void setSelected(in nsIDOMElement aCase, in boolean aTrue);

why is this parameter named |aTrue|?  the comment says that it is
the value of the selected attribute, so wouldn't |aValue| be a
better parameter name?


>+++ nsXFormsSwitchElement.cpp  2004-12-03 22:23:04.558668968 +0200

>+  aWrapper->GetElementNode(getter_AddRefs(mElement));

are you sure this doesn't create a reference cycle?  ah, nevermind...
you null it out in OnDestroyed :)

>+nsXFormsSwitchElement::Process()

process feels like an odd name for this function given that it seems
to be setting up stuff rather than actually processing anything.

why does GetContext need to call Process?  should GetContext check to
see if mContextNode is already set and then not bother calling Process?


>+already_AddRefed<nsIDOMElement>
>+nsXFormsSwitchElement::FindFirstSelectedCase(nsIDOMElement* aDeselected)

>+      nsAutoString name;
>+      nsAutoString ns;
>+      childElement->GetLocalName(name);
>+      childElement->GetNamespaceURI(ns);
>+      if (name.EqualsLiteral("case") && ns.EqualsLiteral(NS_NAMESPACE_XFORMS)) {

you might optimize this by checking |name| before calling |GetNamespaceURI|.


>+    nsCOMPtr<nsIDOMNode> tmp;
>+    child->GetNextSibling(getter_AddRefs(tmp));
>+    child = tmp;

nit: |child.swap(tmp);| would be slightly more optimal.


>+  nsIDOMElement* result;
>+  NS_IF_ADDREF(result = firstCase);

nit: |firstCase.swap(result);| would be slightly more optimal.


>+  //Swapping the status of the mSelected and aCase, dispatching events.

nit: add a space between '//' and 'Swapping'
(same nit applies elsewhere)


>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+  if (!doc)
>+    return;
>+
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(doc->GetScriptGlobalObject());
>+  if (!win)
>+    return;
>+
>+  nsIFocusController *focusController = win->GetRootFocusController();
>+  if (!focusController)
>+    return;
>+
>+  nsCOMPtr<nsIDOMElement> focused;
>+  focusController->GetFocusedElement(getter_AddRefs(focused));
>+  if (!focused)
>+    return;

ACK!  more unfrozen APIs.  you're certain there are no frozen interfaces
that could be used instead?


>+    nsCOMPtr<nsIDOMNode> parent;
>+    current->GetParentNode(getter_AddRefs(parent));
>+    current = parent;

nit: use .swap here too.


>+  nsAutoString name;
>+  nsAutoString ns;
>+  aCase->GetLocalName(name);
>+  aCase->GetNamespaceURI(ns);
>+  if (!name.EqualsLiteral("case") && !ns.EqualsLiteral(NS_NAMESPACE_XFORMS))
>+    return;

again, might be good to check |name| before calling |GetNamespaceURI|.
heck, how about moving this code into a common subroutine?


>+++ nsXFormsCaseElement.cpp    2004-12-03 22:23:09.370937392 +0200

>+NS_NAMED_LITERAL_STRING(kSelectedCase, "");
>+NS_NAMED_LITERAL_STRING(kDeselectedCase, "visibility:hidden;width:0px;height:0px;");

error!!!  NS_NAMED_LITERAL_STRING cannot be used at global scope
like this.  this will cause the OSX build to crash :(



>+  if (mVisual) {
>+    if (aEnable)
>+      mVisual->SetAttribute(NS_LITERAL_STRING("style"),
>+                            kSelectedCase);
>+    else
>+      mVisual->SetAttribute(NS_LITERAL_STRING("style"),
>+                            kDeselectedCase);

reduce codesize by only calling the function once:

   mVisual->SetAttribute(NS_LITERAL_STRING("style"),
			 aEnable ? kSelectedCase
				 : kDeselectedCase);


>+++ nsXFormsToggleElement.cpp  2004-12-03 22:23:16.689824752 +0200

>+nsXFormsToggleElement::nsXFormsToggleElement()
>+{
>+}

this constructor does nothing, so why define it at all?


>+  nsAutoString name;
>+  nsAutoString ns;
>+  caseEl->GetLocalName(name);
>+  caseEl->GetNamespaceURI(ns);
>+  if (!name.EqualsLiteral("case") || !ns.EqualsLiteral(NS_NAMESPACE_XFORMS))
>+    return NS_OK;

OK, now I think that helper function for this should be in nsXFormsUtils :-)\


Otherwise, this patch looks good.  But, I'd like to see a revised version
before approving -- Thanks!

Comment 24

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

Attachment: v4

Fixed Darin's comments, except:
  - Still using FocusController, I couldn't find any frozen interface.
  - I left the Process() and GetContext() as they are. Those are copy-paste
from
    nsXFormsGroupElement, but there will be a new generic implementation
    for those methods. They will be needed elsewhere too, afaik.  
    I hope this is ok for now.

And the nsXFormsInput element didn't crash anymore, something must have
changed.

Comment 25

[Darin Fisher]

Comment on attachment 168246 [details] [diff] [review]
v4

sr=darin, but requesting review from bryner because of the focuscontroller
stuff.

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 168246 [details] [diff] [review]
v4

focus controller parts look ok, though it's unfortunate that we need to use it.

Comment 27

[Darin Fisher]

fixed-on-trunk