Actions inside itemsets behave incorrectly

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
10 years ago
10 months ago

People

(Reporter: aaronr, Assigned: aaronr)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
x86
All
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

1.66 KB, application/xhtml+xml
Details
4.25 KB, application/xhtml+xml
Details
60.95 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 259376 [details]
testcase
(Assignee)

Comment 2

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

Comment 3

10 years ago
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

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

Updated

10 years ago
Assignee: xforms → aaronr
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
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.
Attachment #259662 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #259662 - Attachment is patch: true
Attachment #259662 - Attachment mime type: application/octet-stream → text/plain

Comment 6

10 years ago
IIRC the fact there is contextcontainer that contains xf:item may be used in native widget selects.
(Assignee)

Comment 7

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

Updated

10 years ago
Attachment #259662 - Flags: review?(Olli.Pettay)

Comment 8

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

10 years ago
 /**
+ * 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

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

10 years ago
Aaron, correct me if I'm wrong. All you do it stub action or message won't handle any events, rigth?

Comment 12

10 years ago
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

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

Comment 14

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

Comment 15

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

Comment 16

10 years ago
Comment on attachment 259662 [details] [diff] [review]
patch

something broke with my patch, so removing reviews until I fix it.
Attachment #259662 - Flags: review?(surkov.alexander)
Attachment #259662 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 17

10 years ago
looks like select1 @appearance="compact" is missing a size="5" on the html:select, so I'll fix that in this patch, too.
(Assignee)

Comment 18

10 years ago
Created attachment 260194 [details]
testcase2
(Assignee)

Comment 19

10 years ago
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.
Attachment #259662 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #260195 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #260195 - Flags: review?(Olli.Pettay)

Comment 20

10 years ago
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.
Attachment #260195 - Flags: review?(surkov.alexander) → review+

Comment 21

10 years ago
if you comment for UpdateRepeatState method then it would be great, I spent some time before I got why it works.
(Assignee)

Comment 22

10 years ago
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.
Attachment #260195 - Attachment is obsolete: true
Attachment #260318 - Flags: review?(Olli.Pettay)
Attachment #260195 - Flags: review?(Olli.Pettay)

Comment 23

10 years ago
er,  is nsXFormsContextContainer.h missing in the patch?
(Assignee)

Comment 24

10 years ago
(In reply to comment #23)
> er,  is nsXFormsContextContainer.h missing in the patch?
> 

Yes, sorry.  same patch with that added coming up.
(Assignee)

Comment 25

10 years ago
Created attachment 260360 [details] [diff] [review]
patch4
Attachment #260318 - Attachment is obsolete: true
Attachment #260318 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Attachment #260360 - Flags: review?(Olli.Pettay)

Comment 26

10 years ago
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?
Attachment #260360 - Flags: review?(Olli.Pettay) → review+

Comment 27

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

Comment 28

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

10 years ago
I meant Windows' themes, not Firefox themes.
(Assignee)

Comment 30

10 years ago
Datepicker and select1 look/work fine under windows themes, checking in the patch.
(Assignee)

Comment 31

10 years ago
checked into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Comment 32

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.