302497 - selected attribute on only case only determines initial state

Status: RESOLVED WONTFIX

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

pr. errata E25b.

Need to kill AttributeSet() in case, and we probably need to do something like
what is done for inputs etc., which is saving the initial state, so we are not
"fooled" by attribute changes.

Comment 1

[jhp (no longer active)]

> and we probably need to do something like
> what is done for inputs etc., which is saving the initial state, so we are not
> "fooled" by attribute changes.

Allan, what do you mean by this?  Why would we need to save the initial state?

Comment 2

[jhp (no longer active)]

Attachment: patch

Patch based off of Doron's patch from bug 301374.

Comment 3

[Allan Beaufour]

(In reply to comment #1)
> > and we probably need to do something like
> > what is done for inputs etc., which is saving the initial state, so we are not
> > "fooled" by attribute changes.
> 
> Allan, what do you mean by this?  Why would we need to save the initial state?

If the selected attribute is changed by f.x. script and the control is rebuilt
(f.x. being inside a repeat), and we use the attribute on the rebuild we will
show the wrong state as I interpret "initial state".

Comment 4

[jhp (no longer active)]

Attachment: testcase

Quick and dirty testcase.  Click on "Toggle..." once, and then click on
"Append..." button.  The text "case 1" should stay visible, but the current
code switches between "case 1" and "case 2".

Comment 5

[jhp (no longer active)]

Attachment: patch v2 (checked in)

Comment 6

[Doron Rosenberg (IBM)]

Comment on attachment 192277 [details] [diff] [review]
patch v2 (checked in)

>Index: nsIXFormsCaseElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsCaseElement.idl,v
>retrieving revision 1.2
>diff -u -6 -p -r1.2 nsIXFormsCaseElement.idl
>--- nsIXFormsCaseElement.idl	9 Aug 2005 19:17:17 -0000	1.2
>+++ nsIXFormsCaseElement.idl	10 Aug 2005 22:54:17 -0000
>@@ -38,16 +38,24 @@
> 
> #include "nsISupports.idl"
> 
> /**
>  * Interface implemented by the case element.
>  */
>-[scriptable, uuid(7dd9c79b-5fa2-4a0a-8aa7-f2629281c743)]
>+[scriptable, uuid(28c8c223-ca17-4b58-b3ca-189864eefe32)]
> interface nsIXFormsCaseElement : nsISupports
> {
>   /**
>+   * According to the XForms 1.0 Errata, the "selected" attribute only
>+   * determines the initial selected state.  This attribute, then, provides
>+   * access to the initial state of the "selected" attribute, since it can
>+   * be programmatically changed via the DOM.
>+   */
>+  readonly attribute boolean initialSelectedState;
>+

Perhaps link to the errata?

> NS_IMETHODIMP
> nsXFormsCaseElement::WidgetAttached()
> {
>+  // cache the value of the "selected" attr
>+  nsAutoString selected;
>+  mElement->GetAttribute(NS_LITERAL_STRING("selected"), selected);
>+  if (selected.EqualsLiteral("true")) {
>+    mCachedSelectedAttr = PR_TRUE;
>+  }

Do we need to worry about capitalization?

Comment 7

[Allan Beaufour]

Comment on attachment 192277 [details] [diff] [review]
patch v2 (checked in)

Looks good. With Doron's comments fixed, r=me

Comment 8

[jhp (no longer active)]

> Perhaps link to the errata?

Sure.

> Do we need to worry about capitalization?

I assume you mean, why don't we also check for "TRUE" or "True", etc?  Well, as
you can see from the code that I removed, we weren't worrying about previously.
 And according to the XForms Schema, "selected" is an xsd:boolean value, which
means it can take "true", "false", "1", or "0".  So I should probably be doing
something similar to |nsXFormsSubmissionElement::GetBooleanAttr()|.

Comment 9

[jhp (no longer active)]

Checked in.  I just changed the comparison line to be:
+  if (value.EqualsLiteral("true") || value.EqualsLiteral("1")) {

->FIXED

Comment 10

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

Comment on attachment 192277 [details] [diff] [review]
patch v2 (checked in)

> 
> NS_IMETHODIMP
> nsXFormsCaseElement::WidgetAttached()
> {
>+  // cache the value of the "selected" attr
>+  nsAutoString selected;
>+  mElement->GetAttribute(NS_LITERAL_STRING("selected"), selected);
>+  if (selected.EqualsLiteral("true")) {
>+    mCachedSelectedAttr = PR_TRUE;
>+  }
>+


I think mCachedSelectAttr should be set in DoneAddingChildren().
Switch element calls case->GetInitialSelectedState first time
before any widgets are attached. (There is the Init() call in
DoneAddingChildren())

And also, whenever a widget is changed (for example from XHTML to SVG)
WidgetAttached()
will be called and then if someone has modified the selected attribute, the
initial state
of the <case> changes...

Comment 11

[jhp (no longer active)]

Attachment: 2nd patch

Patch to address smaug's comments.

Comment 12

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

(In reply to comment #10)
> I think mCachedSelectAttr should be set in DoneAddingChildren().
> Switch element calls case->GetInitialSelectedState first time
> before any widgets are attached. (There is the Init() call in
> DoneAddingChildren())
> 
> And also, whenever a widget is changed (for example from XHTML to SVG)
> WidgetAttached()
> will be called and then if someone has modified the selected attribute, the
> initial state
> of the <case> changes... 
> 

Now that I think this more, I've changed my mind a bit.
Setting mCachedSelectAttr in WillChangeParent(nsIDOMElement *aNewParent)
(if aNewParent != nsnull) should work also when <case> element is
created using scripts or <repeat>.
(DoneAddingChildren is called only when element is created by the content sink.)

Javier, does this sound reasonable? And sorry about not noticing this earlier.

Comment 13

[jhp (no longer active)]

I have a testcase that puts a <case> in a <repeat>, and |WillChangeParent()|
gets called each time a new item is added in the <repeat>.  So this means that
if I change the value of "selected" before adding the item, |WillChangeParent()|
will recalculate the cached value, which isn't what we want.

Comment 14

[jhp (no longer active)]

(In reply to comment #12)
> Setting mCachedSelectAttr in WillChangeParent(nsIDOMElement *aNewParent)
> (if aNewParent != nsnull) should work also when <case> element is
> created using scripts or <repeat>.

|DoneAddingChildren()| was working in the <repeat> case for me, but didn't test
creating it using scripts.

Comment 15

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

Attachment: testcase

With the latest patch the switch-case outside <repeat> does work, but not
inside <repeat>.

Comment 16

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

Comment on attachment 193603 [details] [diff] [review]
2nd patch

So this is not enough.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

RIP xforms