Wicket
  1. Wicket
  2. WICKET-3705

AjaxSubmit in modal window doesn't call form.onSubmit() before ending request

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5-RC4
    • Fix Version/s: None
    • Component/s: wicket, wicket-extensions
    • Labels:
      None

      Description

      When a AjaxSubmitLink is used to submit a form inside a ModalWindow, and inside the AjaxSubmitLink#onSubmit() the window is closed, wicket doesn't call Form.onSubmit()

      See attached quickstart for the scenario.

      1. WICKET-3705.patch
        11 kB
        Pedro Santos
      2. myproject-1.4.tgz
        19 kB
        Martijn Dashorst
      3. myproject.tgz
        19 kB
        Martijn Dashorst

        Activity

        Hide
        Martijn Dashorst added a comment -

        Wicket 1.4 quickstart showing that this is in fact a regression

        Show
        Martijn Dashorst added a comment - Wicket 1.4 quickstart showing that this is in fact a regression
        Hide
        Martijn Dashorst added a comment -

        The processing of form.onSubmit is not executed because in our submitlink#onSubmit() we close the modalWindow, causing the containing panel to be no longer marked visible.

        Show
        Martijn Dashorst added a comment - The processing of form.onSubmit is not executed because in our submitlink#onSubmit() we close the modalWindow, causing the containing panel to be no longer marked visible.
        Hide
        Peter Ertl added a comment -

        Fixed in trunk.

        The test cases report no errors so far. We had a few differences between 1.4 and 1.5. Should be similar again.

        The only remaining difference I see is a different order in processing of the submit:

        • First AjaxSubmitLink#onSubmit is invoked
        • Secondly, Form#onSubmit is invoked

        We also have a test case that demands this order:

        FormWithMultipleButtonsTest#testAjaxFallbackButtonInvokedFirst

        It points to https://issues.apache.org/jira/browse/WICKET-1894 where this order is also required.

        So the processing order is different in 1.5 but I think this still is correct.

        If you can confirm that, too, we probably should mention that in the migration guide.

        Show
        Peter Ertl added a comment - Fixed in trunk. The test cases report no errors so far. We had a few differences between 1.4 and 1.5. Should be similar again. The only remaining difference I see is a different order in processing of the submit: First AjaxSubmitLink#onSubmit is invoked Secondly, Form#onSubmit is invoked We also have a test case that demands this order: FormWithMultipleButtonsTest#testAjaxFallbackButtonInvokedFirst It points to https://issues.apache.org/jira/browse/WICKET-1894 where this order is also required. So the processing order is different in 1.5 but I think this still is correct. If you can confirm that, too, we probably should mention that in the migration guide.
        Hide
        Martin Grigorov added a comment -

        > The test cases report no errors so far.

        Without new test the tests will pass again when someone change the logic.

        Show
        Martin Grigorov added a comment - > The test cases report no errors so far. Without new test the tests will pass again when someone change the logic.
        Hide
        Martin Grigorov added a comment -

        Reopening the ticket because the order of calling onSubmit is wrong:

        ivaynberg:
        doesnt make sense, onsubmit should go from most inner to outer. means
        also processingForm.onSubmit(); should be called last...

        A test case should be added as well.

        Show
        Martin Grigorov added a comment - Reopening the ticket because the order of calling onSubmit is wrong: ivaynberg: doesnt make sense, onsubmit should go from most inner to outer. means also processingForm.onSubmit(); should be called last... A test case should be added as well.
        Hide
        Igor Vaynberg added a comment -

        ...if calling onsubmit at all makes sense... after all it is the parent whose button was pressed that is being submitted....

        Show
        Igor Vaynberg added a comment - ...if calling onsubmit at all makes sense... after all it is the parent whose button was pressed that is being submitted....
        Hide
        Peter Ertl added a comment -

        Allright, I see that the current coding is not satisfying...

        I have several questions and would love to get some feedback:

        1. form processing order:

        this is the order of form processing in 1.4.x (outer to inner form):

        // call onSubmit on nested forms
        formToProcess.visitChildren(Form.class, new IVisitor<Form<?>>()
        {
        public Object component(Form<?> component)
        {
        Form<?> form = component;
        if (form.isEnabledInHierarchy() && form.isVisibleInHierarchy())

        { form.onSubmit(); return IVisitor.CONTINUE_TRAVERSAL; }

        return IVisitor.CONTINUE_TRAVERSAL_BUT_DONT_GO_DEEPER;
        }
        });

        so do we really want to change that in 1.5 (inner to outer form) ?

        2. in Martijn's case (= quickstart) the form visibility is changed during form submit processing. Therefore the submitting component's onSubmit is called but Form#onSubmit not since the submitting component hides the modal windows which implicitly hides the contained form. Do we want that form visibility (and enabled) state changes during submit processing affect subsequent processing?

        3. is staying as close to 1.4.x processing as possible to make migration seamless an issue?

        Show
        Peter Ertl added a comment - Allright, I see that the current coding is not satisfying... I have several questions and would love to get some feedback: 1. form processing order: this is the order of form processing in 1.4.x (outer to inner form): // call onSubmit on nested forms formToProcess.visitChildren(Form.class, new IVisitor<Form<?>>() { public Object component(Form<?> component) { Form<?> form = component; if (form.isEnabledInHierarchy() && form.isVisibleInHierarchy()) { form.onSubmit(); return IVisitor.CONTINUE_TRAVERSAL; } return IVisitor.CONTINUE_TRAVERSAL_BUT_DONT_GO_DEEPER; } }); so do we really want to change that in 1.5 (inner to outer form) ? 2. in Martijn's case (= quickstart) the form visibility is changed during form submit processing. Therefore the submitting component's onSubmit is called but Form#onSubmit not since the submitting component hides the modal windows which implicitly hides the contained form. Do we want that form visibility (and enabled) state changes during submit processing affect subsequent processing? 3. is staying as close to 1.4.x processing as possible to make migration seamless an issue?
        Hide
        Igor Vaynberg added a comment -

        while changes in visibility/enableness should not effect processing within current-request we cannot guarantee it. those changes become live immediately just like with any other component. perhaps what martijn should do is rewrite that code to use a boolean flag and onconfigure to modify visibility...

        Show
        Igor Vaynberg added a comment - while changes in visibility/enableness should not effect processing within current-request we cannot guarantee it. those changes become live immediately just like with any other component. perhaps what martijn should do is rewrite that code to use a boolean flag and onconfigure to modify visibility...
        Hide
        Peter Ertl added a comment -

        I changed the code so form submit processing goes from innermost (most deeply nested form) to outermost (topmost form) and invisible or disabled forms to not get Form#onSubmit() invoked. Basically that's the way it was before.

        Only difference is that now in case the submit is caused by a form submitting component (instead of a default submit using basic html behavior) we traverse the form the submitting component is bound to (which could be different from the form the submitting component is added to).

        Show
        Peter Ertl added a comment - I changed the code so form submit processing goes from innermost (most deeply nested form) to outermost (topmost form) and invisible or disabled forms to not get Form#onSubmit() invoked. Basically that's the way it was before. Only difference is that now in case the submit is caused by a form submitting component (instead of a default submit using basic html behavior) we traverse the form the submitting component is bound to (which could be different from the form the submitting component is added to).
        Hide
        Martijn Dashorst added a comment -

        While we certainly will improve our form processing for this particular case, the issue is that this fails silently in an unexpected manner. While I have the guts to step into Wicket internals to find out what went wrong and discovered what went wrong pretty easily (after creating a quickstart), casual users will not venture that far.

        IMO we should revisit visibility/enabled flags in 1.6 (wicket 3?) and maybe try to move to a more immutable state representation. But for now, I'd rather have we determine which components need to participate in the form processing beforehand based on visibility/enabled flags and then call all listeners that are affected by the submit.

        Show
        Martijn Dashorst added a comment - While we certainly will improve our form processing for this particular case, the issue is that this fails silently in an unexpected manner. While I have the guts to step into Wicket internals to find out what went wrong and discovered what went wrong pretty easily (after creating a quickstart), casual users will not venture that far. IMO we should revisit visibility/enabled flags in 1.6 (wicket 3?) and maybe try to move to a more immutable state representation. But for now, I'd rather have we determine which components need to participate in the form processing beforehand based on visibility/enabled flags and then call all listeners that are affected by the submit.
        Hide
        Pedro Santos added a comment -

        This problem can be addressed by freeze the component tree state while processing the form. It can be done by queuing state change actions to be applied only after the form processing end.
        I'm sending a patch with an initial implementation that already solves this ticket problem and test cases preventing it.

        @devs, what do you think about this strategy to increase the state consistency while processing the form? If it is OK then we can start coding the real solution (the patch is only to show up this idea in code).

        Show
        Pedro Santos added a comment - This problem can be addressed by freeze the component tree state while processing the form. It can be done by queuing state change actions to be applied only after the form processing end. I'm sending a patch with an initial implementation that already solves this ticket problem and test cases preventing it. @devs, what do you think about this strategy to increase the state consistency while processing the form? If it is OK then we can start coding the real solution (the patch is only to show up this idea in code).
        Hide
        Igor Vaynberg added a comment -

        i really dont like this. we would have to buffer every possible action that modifies the hierarchy.

        visible and enabled are easy enough. but what if i replace an inner form with another one? we cant buffer all possible operations.

        i think the users should simply be aware of what they are doing.

        also what will help here is to call onsubmit() in reverse order since it is a much less likely usecase that a child form modifies it's parent hierarchy.

        so onsubmit() on inner forms first going up to the root form. cant in 1.4.x but in 1.5.x we can.

        as a work around in 1.4.x we can visit the forms first and store them in a list, then iterate the list and call onsubmit() - which i think is what martijn suggested above.

        Show
        Igor Vaynberg added a comment - i really dont like this. we would have to buffer every possible action that modifies the hierarchy. visible and enabled are easy enough. but what if i replace an inner form with another one? we cant buffer all possible operations. i think the users should simply be aware of what they are doing. also what will help here is to call onsubmit() in reverse order since it is a much less likely usecase that a child form modifies it's parent hierarchy. so onsubmit() on inner forms first going up to the root form. cant in 1.4.x but in 1.5.x we can. as a work around in 1.4.x we can visit the forms first and store them in a list, then iterate the list and call onsubmit() - which i think is what martijn suggested above.
        Hide
        Pedro Santos added a comment - - edited

        Sure, I noticed that I miss the involved complexity about state caching.

        The same work around in 1.4 would have to be done in 1.5 because only reverse onsubmit() sequence is not enough for all use cases.

        Mark a flag to trigger some action coded in onConfigure is a good user solution. What if we try to make clear that it's user responsibility to delay form processing sensitive changes to render phase? An possible improvement is this direction would be create the FormProcessing type that would be visible by all submit callbacks, and would provide an API do schedule post submit actions.
        e.g.
        add(new Button(){
        onSubmit(processing){
        //here code not involving interface like queries or data manipulation
        processing.schedule(new Action()

        { rootForm.disableBecauseSomeDataReachedSomeState();}

        );
        };
        });

        Show
        Pedro Santos added a comment - - edited Sure, I noticed that I miss the involved complexity about state caching. The same work around in 1.4 would have to be done in 1.5 because only reverse onsubmit() sequence is not enough for all use cases. Mark a flag to trigger some action coded in onConfigure is a good user solution. What if we try to make clear that it's user responsibility to delay form processing sensitive changes to render phase? An possible improvement is this direction would be create the FormProcessing type that would be visible by all submit callbacks, and would provide an API do schedule post submit actions. e.g. add(new Button(){ onSubmit(processing){ //here code not involving interface like queries or data manipulation processing.schedule(new Action() { rootForm.disableBecauseSomeDataReachedSomeState();} ); }; });
        Hide
        Igor Vaynberg added a comment -

        i still think its too complex. and this is a very rare usecase - first time it happened since wicket has been around. i say we just memoize the set of forms we have to visit and visit them.

        Show
        Igor Vaynberg added a comment - i still think its too complex. and this is a very rare usecase - first time it happened since wicket has been around. i say we just memoize the set of forms we have to visit and visit them.
        Hide
        Martin Grigorov added a comment -

        Do we have an agreement how to solve this problem ?
        We have to solve it soon unless it is OK to leave for 1.5.0+.

        Show
        Martin Grigorov added a comment - Do we have an agreement how to solve this problem ? We have to solve it soon unless it is OK to leave for 1.5.0+.
        Hide
        Martijn Dashorst added a comment -

        The use case is not very rare since it only happens after changes made to stricter visibility checks in late 1.4 and 1.5 releases. Previous versions didn't check visibility that strict AFAIK. The code in question in our app worked fine with 1.4.x (not sure when we left the 1.4 train).

        Show
        Martijn Dashorst added a comment - The use case is not very rare since it only happens after changes made to stricter visibility checks in late 1.4 and 1.5 releases. Previous versions didn't check visibility that strict AFAIK. The code in question in our app worked fine with 1.4.x (not sure when we left the 1.4 train).

          People

          • Assignee:
            Unassigned
            Reporter:
            Martijn Dashorst
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development