Last Comment Bug 374994 - Actions inside itemsets behave incorrectly
: Actions inside itemsets behave incorrectly
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-22 17:54 PDT by aaronr
Modified: 2007-04-23 16:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.66 KB, application/xhtml+xml)
2007-03-22 18:02 PDT, aaronr
no flags Details
patch (40.29 KB, patch)
2007-03-26 00:23 PDT, aaronr
no flags Details | Diff | Review
testcase2 (4.25 KB, application/xhtml+xml)
2007-03-30 16:58 PDT, aaronr
no flags Details
patch2 (59.46 KB, patch)
2007-03-30 17:03 PDT, aaronr
surkov.alexander: review+
Details | Diff | Review
patch3 (55.93 KB, patch)
2007-04-01 22:30 PDT, aaronr
no flags Details | Diff | Review
patch4 (60.95 KB, patch)
2007-04-02 11:05 PDT, aaronr
bugs: review+
Details | Diff | Review

Description aaronr 2007-03-22 17:54:39 PDT
According to the errata, xforms actions that are contained directly underneath an itemset like so:

<xf:itemset nodeset="xxx">
  <xf:label ref="@label"/>
  <xf:value ref="@value"/>
  <xf:message level="modal" ev:event="xforms-select" ref="@message"/>
</xf:itemset>

should be registered such that the observer is the anonymous item, not the actual xf:itemset.  Specifically, "An XForms processor must not allow XForms Actions contained by an itemset to handle events on the itemset." (http://www.w3.org/2006/03/REC-xforms-20060314-errata.html#E37b)

I talked to smaug.  We should probably handle this by checking inside ::HandleEvent and see if the action has eType_Template.  If it is, then just don't do any work.  This means that we'll have to make sure that eType_Xxxx will get applied to nsXFormsStubElement objects, too.  Currently only goes on nsXFormsControlStub objects.
Comment 1 aaronr 2007-03-22 18:02:33 PDT
Created attachment 259376 [details]
testcase
Comment 2 aaronr 2007-03-23 00:41:45 PDT
Olli, any reason why nsXFormsActionElement doesn't inherit from nsXFormsActionModuleBase?  I'm going to need to put WillChangeParent, WillChangeDocument and ParentChanged handlers in all actions, plus adding a test for eType_Template inside ::HandleEvent.  Seems like I could do all of this inside nsXFormsActionModuleBase if nsXFormsActionElement can inherit from it.
Comment 3 aaronr 2007-03-23 00:51:17 PDT
The other big question that we have to resolve is that now our anonymous action handlers will observe the xf:contextcontainers instead of the xf:item.  That is a problem.  I can't think of an easy way to change this or to make the current situation work for us.  Almost as if we need to get rid of nsXFormsContextContainers for itemsets and make the anonymous items special so that they can give context to the elements that they contain (normal items can't because they have no binding attributes).
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-03-23 01:09:59 PDT
(In reply to comment #2)
> Olli, any reason why nsXFormsActionElement doesn't inherit from
> nsXFormsActionModuleBase?

There *was* some reason, but right now I don't remember what. 
But feel free to change that.
Comment 5 aaronr 2007-03-26 00:23:31 PDT
Created attachment 259662 [details] [diff] [review]
patch

To get the xforms-select and xforms-deselect to be handled correctly, I had to change xf:item from being a stub element to being a subclass of context container, and no longer put contextcontainers in the generated items under an itemset.  I had also thought about subclassing xf:item and call it xf:mozitem, for example, but then I'd have had to make mozitem implement nsIXFormsContextControl, which would bring in nsIXFormsControl and that seemed wrong for something that subclasses stub element.  The only other solution I could think of was putting in some kind of event forwarder into item element to forward all events to the contextcontainer which would screw up any handlers on the select, for example, because events would get fired more often than expected.

I also had to move the repeatState stuff into stub element so that xforms actions could look to see if they are part of a template or not.  If they ARE part of a template, we'll now just return out of HandleEvent without doing anything.  I also moved the nsXFormsContextContainer class definition out of the .cpp file and into a .h file so that nsXFormsItemElement.cpp could see it.
Comment 6 alexander :surkov 2007-03-27 08:01:09 PDT
IIRC the fact there is contextcontainer that contains xf:item may be used in native widget selects.
Comment 7 aaronr 2007-03-27 12:00:04 PDT
(In reply to comment #6)
> IIRC the fact there is contextcontainer that contains xf:item may be used in
> native widget selects.
> 

I fixed the two places in selectsnw that I found.  I didn't see any others.
Comment 8 alexander :surkov 2007-03-28 01:32:11 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > IIRC the fact there is contextcontainer that contains xf:item may be used in
> > native widget selects.
> > 
> 
> I fixed the two places in selectsnw that I found.  I didn't see any others.
> 

Where these fixes are?
Comment 9 alexander :surkov 2007-03-28 01:36:06 PDT
 /**
+ * nsRepeatState is used to indicate whether the element
+ * is inside \<repeat\>'s template.

nit: It looks not very correct.

If it is, there is no need
+ * to refresh the widget bound to the element.
+ */
+enum nsRepeatState {
+  eType_Unknown,
+  eType_Template,
+  eType_GeneratedContent,
+  eType_NotApplicable
+};

nit: can you shortly document these states?

+  /**
+   * This is called when the parent node for a XForms control or action changes.

nit: I'm not big english man and I can't translate this :). Can you restate this?
Comment 10 alexander :surkov 2007-03-28 02:07:41 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > IIRC the fact there is contextcontainer that contains xf:item may be used in
> > > native widget selects.
> > > 
> > 
> > I fixed the two places in selectsnw that I found.  I didn't see any others.
> > 
> 
> Where these fixes are?
> 

Ah, I see. Just diff of the patch looks wrong.
Comment 11 alexander :surkov 2007-03-28 02:36:51 PDT
Aaron, correct me if I'm wrong. All you do it stub action or message won't handle any events, rigth?
Comment 12 alexander :surkov 2007-03-28 02:41:32 PDT
All elements that are inherited from nsXFormsActionModuleBase don't call nsXFormsActionModuleBase::HandleEvent therefore if action is a stub then it will be executed in any way. Right?
Comment 13 alexander :surkov 2007-03-28 02:44:05 PDT
(In reply to comment #12)
> All elements that are inherited from nsXFormsActionModuleBase don't call
> nsXFormsActionModuleBase::HandleEvent therefore if action is a stub then it
> will be executed in any way. Right?
> 

Why do you fix this for xf:message only?
Comment 14 aaronr 2007-03-28 17:59:43 PDT
(In reply to comment #9)
>  /**
> + * nsRepeatState is used to indicate whether the element
> + * is inside \<repeat\>'s template.
> 
> nit: It looks not very correct.

my bad.  I deleted part of the comment inappropriately.  I'll fix that.

> 
> If it is, there is no need
> + * to refresh the widget bound to the element.
> + */
> +enum nsRepeatState {
> +  eType_Unknown,
> +  eType_Template,
> +  eType_GeneratedContent,
> +  eType_NotApplicable
> +};
> 
> nit: can you shortly document these states?
> 
> +  /**
> +   * This is called when the parent node for a XForms control or action
> changes.
> 
> nit: I'm not big english man and I can't translate this :). Can you restate
> this?
> 

will fix these nits
Comment 15 aaronr 2007-03-28 18:21:00 PDT
(In reply to comment #12)
> All elements that are inherited from nsXFormsActionModuleBase don't call
> nsXFormsActionModuleBase::HandleEvent therefore if action is a stub then it
> will be executed in any way. Right?
> 

dispatch, rebuild, recalculate, revalidate, refresh, setfocus, load, setvalue, send, reset, toggle, insert, delete, and setindex inherit from nsXFormsActionModuleBase and none of them override HandleEvent, so they will all have the same inherited behavior of not executing if they have the template repeat state.

xf:action inherits directly from stub element so I had to change its HandleEvent method to check for being a template element and not allow any contained actions to be executed since they would be template elements, too.

message (and thus alert, help, and hint) inherits from control stub so I had to change its HandleEvent method to check for being a template element.

I think that answers you concerns.  I'll fix your nits and put up a new patch.
Comment 16 aaronr 2007-03-28 23:47:31 PDT
Comment on attachment 259662 [details] [diff] [review]
patch

something broke with my patch, so removing reviews until I fix it.
Comment 17 aaronr 2007-03-29 13:50:09 PDT
looks like select1 @appearance="compact" is missing a size="5" on the html:select, so I'll fix that in this patch, too.
Comment 18 aaronr 2007-03-30 16:58:26 PDT
Created attachment 260194 [details]
testcase2
Comment 19 aaronr 2007-03-30 17:03:39 PDT
Created attachment 260195 [details] [diff] [review]
patch2

fixes surkov's nits.  Also fixes holes I had but hadn't realized I hadn't already fixed (like needing to retarget the xforms-select and xforms-deselect events away from the itemset to the individual items).  Also fixes the size="5"-missing issue that I mentioned before.  Also fixes a piece of code where the xforms-select events weren't being targeted at the deferredEventTargets list.
Comment 20 alexander :surkov 2007-03-31 10:47:26 PDT
Comment on attachment 260195 [details] [diff] [review]
patch2


>+NS_IMETHODIMP
>+nsXFormsItemSetElement::ParentChanged(nsIDOMElement *aNewParent)
>+{
>   mHasParent = aNewParent != nsnull;

nit, you can call stub::ParentChanged instead of this.

good job, r=me.
Comment 21 alexander :surkov 2007-03-31 10:59:06 PDT
if you comment for UpdateRepeatState method then it would be great, I spent some time before I got why it works.
Comment 22 aaronr 2007-04-01 22:30:04 PDT
Created attachment 260318 [details] [diff] [review]
patch3

Fixed surkov's nit, added comments surkov asked for.  Also added 1px bottom padding to -moz-date-container and -moz-select1-container.  The input contained by them was overlapping the bottom border enough to make it look like it was missing in xhtml-hosted xforms.  Seemed like too small a change to open a new bug for.
Comment 23 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-04-02 09:41:18 PDT
er,  is nsXFormsContextContainer.h missing in the patch?
Comment 24 aaronr 2007-04-02 11:00:34 PDT
(In reply to comment #23)
> er,  is nsXFormsContextContainer.h missing in the patch?
> 

Yes, sorry.  same patch with that added coming up.
Comment 25 aaronr 2007-04-02 11:05:00 PDT
Created attachment 260360 [details] [diff] [review]
patch4
Comment 26 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-04-02 11:31:42 PDT
Comment on attachment 260360 [details] [diff] [review]
patch4

The dropmarker does look quite right in Linux with or without the CSS changes. But if that fixes something in windows, let's keep it. But please test with classic and winxp theme. Does anyone have Vista?
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-04-02 11:32:18 PDT
(In reply to comment #26)
> (From update of attachment 260360 [details] [diff] [review])
> The dropmarker does look quite right in Linux with or without the CSS changes.
Er, does not look...
Comment 28 aaronr 2007-04-02 12:42:31 PDT
(In reply to comment #26)
> (From update of attachment 260360 [details] [diff] [review])
> The dropmarker does look quite right in Linux with or without the CSS changes.
> But if that fixes something in windows, let's keep it. But please test with
> classic and winxp theme. Does anyone have Vista?
> 

Where do I get the themes?  Addons?  Can you give me pointers to them?  I couldn't find them.
Comment 29 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-04-02 12:54:19 PDT
I meant Windows' themes, not Firefox themes.
Comment 30 aaronr 2007-04-02 13:01:07 PDT
Datepicker and select1 look/work fine under windows themes, checking in the patch.
Comment 31 aaronr 2007-04-02 13:24:32 PDT
checked into trunk
Comment 32 aaronr 2007-04-23 16:29:21 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

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