374994 - Actions inside itemsets behave incorrectly

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

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]

Attachment: testcase

Comment 2

[aaronr]

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]

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][[e-mail]]]

(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]

Attachment: 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 (:asurkov)]

IIRC the fact there is contextcontainer that contains xf:item may be used in native widget selects.

Comment 7

[aaronr]

(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 (:asurkov)]

(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 (:asurkov)]

 /**
+ * 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 (:asurkov)]

(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 (:asurkov)]

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 (:asurkov)]

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 (:asurkov)]

(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]

(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]

(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]

Comment on attachment 259662 [details] [diff] [review]
patch

something broke with my patch, so removing reviews until I fix it.

Comment 17

[aaronr]

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]

Attachment: testcase2

Comment 19

[aaronr]

Attachment: 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 (:asurkov)]

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 (:asurkov)]

if you comment for UpdateRepeatState method then it would be great, I spent some time before I got why it works.

Comment 22

[aaronr]

Attachment: 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][[e-mail]]]

er,  is nsXFormsContextContainer.h missing in the patch?

Comment 24

[aaronr]

(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]

Attachment: patch4

Comment 26

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

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][[e-mail]]]

(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]

(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][[e-mail]]]

I meant Windows' themes, not Firefox themes.

Comment 30

[aaronr]

Datepicker and select1 look/work fine under windows themes, checking in the patch.

Comment 31

[aaronr]

checked into trunk

Comment 32

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16