Last Comment Bug 302497 - selected attribute on only case only determines initial state
: selected attribute on only case only determines initial state
Status: RESOLVED WONTFIX
:
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: jhp (no longer active)
: Stephen Pride
:
Mentors:
http://www.w3.org/MarkUp/Forms/Group/...
Depends on: 301374 332129
Blocks: 326372 326373 339937
  Show dependency treegraph
 
Reported: 2005-07-28 06:41 PDT by Allan Beaufour
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.33 KB, patch)
2005-08-05 08:31 PDT, jhp (no longer active)
no flags Details | Diff | Splinter Review
testcase (2.14 KB, application/xhtml+xml)
2005-08-10 16:03 PDT, jhp (no longer active)
no flags Details
patch v2 (checked in) (6.42 KB, patch)
2005-08-10 16:05 PDT, jhp (no longer active)
allan: review+
doronr: review+
Details | Diff | Splinter Review
2nd patch (2.32 KB, patch)
2005-08-23 13:33 PDT, jhp (no longer active)
bugs: review-
Details | Diff | Splinter Review
testcase (1.48 KB, application/xhtml+xml)
2005-09-14 12:00 PDT, Olli Pettay [:smaug]
no flags Details

Description Allan Beaufour 2005-07-28 06:41:07 PDT
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) 2005-08-05 08:29:12 PDT
> 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) 2005-08-05 08:31:45 PDT
Created attachment 191700 [details] [diff] [review]
patch

Patch based off of Doron's patch from bug 301374.
Comment 3 Allan Beaufour 2005-08-08 00:46:04 PDT
(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) 2005-08-10 16:03:57 PDT
Created attachment 192276 [details]
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) 2005-08-10 16:05:07 PDT
Created attachment 192277 [details] [diff] [review]
patch v2 (checked in)
Comment 6 Doron Rosenberg (IBM) 2005-08-10 18:20:51 PDT
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 2005-08-11 01:15:33 PDT
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) 2005-08-11 08:39:54 PDT
> 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) 2005-08-11 09:17:43 PDT
Checked in.  I just changed the comparison line to be:
+  if (value.EqualsLiteral("true") || value.EqualsLiteral("1")) {

->FIXED
Comment 10 Olli Pettay [:smaug] 2005-08-12 00:54:12 PDT
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) 2005-08-23 13:33:35 PDT
Created attachment 193603 [details] [diff] [review]
2nd patch

Patch to address smaug's comments.
Comment 12 Olli Pettay [:smaug] 2005-08-23 13:57:30 PDT
(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) 2005-08-23 15:14:05 PDT
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) 2005-08-23 15:25:57 PDT
(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] 2005-09-14 12:00:08 PDT
Created attachment 196066 [details]
testcase

With the latest patch the switch-case outside <repeat> does work, but not
inside <repeat>.
Comment 16 Olli Pettay [:smaug] 2005-09-14 12:03:06 PDT
Comment on attachment 193603 [details] [diff] [review]
2nd patch

So this is not enough.
Comment 17 David Bolter [:davidb] 2016-02-04 12:20:20 PST
RIP xforms

Note You need to log in before you can comment on or make changes to this bug.