Support for the <case> child element of <toggle>

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Dion Sole, Assigned: Dion Sole)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10
Build Identifier: 

In XForms 1.1, the toggle action can specify its target case by by a case attribute on itself, or by the value attribute or string-value of a child case element, similar to other actions.

I've written a patch to add this, and it works correctly, but I'm not sure if it's the "correct" way to do it or not. As there's already an xforms case element that's used within <xf:switch>, XTF also creates a case element when it comes across the toggle child. I don't see any way to check for e.g. the presence of @value in nsXFormsElementFactory::CreateElement (in order to return null and let the XTF engine just create a default XML element).

Do I actually need to bother checking if it's an "action" case or a "switch" case? Would it be better to check in OnCreated() and set a flag or something for use in other functions (i.e. if (mIsActionCase) {return NS_OK) or similar), or is it not that important?

Reproducible: Always
(Assignee)

Comment 1

9 years ago
Created attachment 379645 [details]
Test case

This shows an example of using the <case> element to dynamically toggle between two cases.
(Assignee)

Comment 2

9 years ago
Created attachment 379646 [details] [diff] [review]
Patch to add support for the <case> child element

And this is the patch. As I said, I'm not sure if it's the best way to do it or not though.
(Assignee)

Updated

9 years ago
Attachment #379646 - Flags: review?(aaronr)

Comment 3

9 years ago
Comment on attachment 379646 [details] [diff] [review]
Patch to add support for the <case> child element

>Index: extensions/xforms/nsXFormsToggleElement.cpp
>===================================================================
                                         nsIXFormsActionElement *aParentAction)
> {
>-  nsAutoString caseAttr;
>-  NS_NAMED_LITERAL_STRING(caseStr, "case");
>-  mElement->GetAttribute(caseStr, caseAttr);
>-  if (caseAttr.IsEmpty())
>+  // The case's ID can come from the value attribute of the child
>+  // 'case' element, the string-value of the child 'case'
>+  // element, or the case attribute of the setfocus
>+  // element. Precedence is in that order.

setfocus element?  Do you mean 'toggle element'? :-)

Most of the rest of the code in your patch is in common with the other actions that we have that can take child elements which provide similar functionality as a current attribute on the action.  Can you generalize all of these into a nsXFormsUtils function that can do this logic so that the code only lives in one spot and not duplicated throughout the code tree?  You don't have to do that as part of this patch.  But could you do this as another bug?  But only if you can generalize the function enough that it makes sense to do this.  Perhaps taking a string for the action attribute name, a string as the child element name and have it return a string value?  Thanks.

As far as the XTF element, I wouldn't worry about trying to prevent it from being an XTF element just because it is the child of a toggle element instead of a child of a switch element.  But I do think that you need to add logic to the case element OnCreated method to remember whether its parent is a switch or not and then check for that inside nsXFormsCaseElement::SetSelected so that we can't possibly select a case element that is not a child of a switch element.  I know that won't happen as part of normal code flow since the case is selected currently through the parent switch element, but just to be safe.

So if you fix the comment, open the bug and check for the parent in OnCreated and remember it in a flag/instance data and check that flag/instance data in SetSelected, r=me
Attachment #379646 - Flags: review?(aaronr) → review+
(Assignee)

Comment 4

9 years ago
Created attachment 382195 [details] [diff] [review]
Updated patch

Updated with comment typo fixed, and a check for whether case is a child of switch or toggle, and associated check in SetSelected.

Bugzilla question now: Is it better for me to just edit the existing patch if it's something like this where I've already gotten review provided I fix a couple of small issues, or should I always upload a completely new version like I did here?
Attachment #379646 - Attachment is obsolete: true
Attachment #382195 - Flags: review?(Olli.Pettay)

Comment 5

9 years ago
(In reply to comment #4)
> Created an attachment (id=382195) [details]
> Updated patch
> 
> Updated with comment typo fixed, and a check for whether case is a child of
> switch or toggle, and associated check in SetSelected.
> 
> Bugzilla question now: Is it better for me to just edit the existing patch if
> it's something like this where I've already gotten review provided I fix a
> couple of small issues, or should I always upload a completely new version like
> I did here?

Edit the patch?  I don't think you can edit an attachment.

You should always upload the new patch then mark the old one as obsolete.  The idea being that the 'current' patch on a bug that has both reviews should be ready for checkin.  It might not always be a reviewer with personal knowledge of the bug that checks it in.  Similar with testcases.  Testcases should be as small and straight forward as possible so that someone who applies and builds a patch to get it ready for checkin should be able to quickly run any attached testcases to make sure that the bug is really fixed prior to checkin.

Updated

9 years ago
Assignee: nobody → dion.sole
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

9 years ago
I've opened bug 497237 for the utility function.

Comment 7

9 years ago
Comment on attachment 382195 [details] [diff] [review]
Updated patch

This doesn't handle the case when <case> is moved in the DOM from a <toggle> 
to a <switch>. 
So the parent check must be done always when calling SetSelected and then there is no need for mIsToggleChild.

(I know we may not support dynamic changes very well, but in this cases there is no reason to not to support it.)

Updated

9 years ago
Attachment #382195 - Flags: review?(Olli.Pettay) → review-

Comment 8

9 years ago
(In reply to comment #7)
> (From update of attachment 382195 [details] [diff] [review])
> This doesn't handle the case when <case> is moved in the DOM from a <toggle> 
> to a <switch>. 
> So the parent check must be done always when calling SetSelected and then there
> is no need for mIsToggleChild.
> 
> (I know we may not support dynamic changes very well, but in this cases there
> is no reason to not to support it.)

Could override ParentChanged, too, but it isn't like SetSelected will get called all that much, either.
(Assignee)

Comment 9

9 years ago
Which would be preferable then? A boolean flag and checking in ParentChanged, or no flag and checking in every SetSelected?

And another bugzilla question: is it possible to have bugzilla email me for changes to anything in the XForms component, regardless of if I've added myself to the CC list? I notice there's a lot of people who get emailed whenever I make a change.
(In reply to comment #9)
> no flag and checking in every SetSelected?
This one, IMO.

> 
> And another bugzilla question: is it possible to have bugzilla email me for
> changes to anything in the XForms component, regardless of if I've added myself
> to the CC list? I notice there's a lot of people who get emailed whenever I
> make a change.
Add xforms@core.bugs to your "watch list" in bugzilla email preferences.
(Assignee)

Comment 11

9 years ago
Created attachment 382590 [details] [diff] [review]
Updated patch

OK, here's a version with the flag removed and a parent check each time SetSelected is called. Also changed another "setfocus" typo to "toggle" (the wonders of copy-pasted code :P)
Attachment #382195 - Attachment is obsolete: true
Attachment #382590 - Flags: review?(Olli.Pettay)

Updated

9 years ago
Attachment #382590 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 12

9 years ago
Cool, thanks for the review :)
Do I need to ask Aaron for another also, or does his original one carry over?

Updated

9 years ago
Attachment #382590 - Flags: review+

Comment 13

9 years ago
checked into ff3 (cvs trunk).  Still needs to be checked into hg.

Comment 14

9 years ago
http://hg.mozilla.org/xforms/rev/0062f5e44b78
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.