The default bug view has changed. See this FAQ.

selected attribute on only case only determines initial state

RESOLVED WONTFIX

Status

Core Graveyard
XForms
RESOLVED WONTFIX
12 years ago
8 months ago

People

(Reporter: Allan Beaufour, Assigned: jhp (no longer active))

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
> 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?
(Assignee)

Comment 2

12 years ago
Created attachment 191700 [details] [diff] [review]
patch

Patch based off of Doron's patch from bug 301374.
Assignee: allan → jhpedemonte
Status: NEW → ASSIGNED
Attachment #191700 - Flags: review?(allan)
(Assignee)

Updated

12 years ago
Depends on: 301374
(Reporter)

Comment 3

12 years ago
(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".
(Assignee)

Updated

12 years ago
Attachment #191700 - Flags: review?(allan)
(Assignee)

Comment 4

12 years ago
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".
(Assignee)

Comment 5

12 years ago
Created attachment 192277 [details] [diff] [review]
patch v2 (checked in)
Attachment #191700 - Attachment is obsolete: true
Attachment #192277 - Flags: review?(allan)
(Assignee)

Updated

12 years ago
Attachment #192277 - Flags: review?(doronr)

Comment 6

12 years ago
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?
(Reporter)

Comment 7

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

Looks good. With Doron's comments fixed, r=me
Attachment #192277 - Flags: review?(allan) → review+

Updated

12 years ago
Attachment #192277 - Flags: review?(doronr) → review+
(Assignee)

Comment 8

12 years ago
> 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()|.
(Assignee)

Comment 9

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

->FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 10

12 years ago
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...
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

12 years ago
Created attachment 193603 [details] [diff] [review]
2nd patch

Patch to address smaug's comments.
Attachment #193603 - Flags: review?(smaug)
(Assignee)

Updated

12 years ago
Attachment #192277 - Attachment description: patch v2 → patch v2 (checked in)

Comment 12

12 years ago
(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.
(Assignee)

Comment 13

12 years ago
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.
(Assignee)

Comment 14

12 years ago
(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

12 years ago
Created attachment 196066 [details]
testcase

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

Comment 16

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

So this is not enough.
Attachment #193603 - Flags: review?(smaug) → review-

Updated

12 years ago
Attachment #196066 - Attachment is patch: false
Attachment #196066 - Attachment mime type: text/plain → application/xhtml+xml

Updated

11 years ago
Blocks: 326372

Updated

11 years ago
Blocks: 326373
(Reporter)

Updated

11 years ago
Depends on: 332129

Updated

9 years ago
Blocks: 339937
RIP xforms
Status: REOPENED → RESOLVED
Last Resolved: 12 years agoa year ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.