Component moves - reparenting looses the component in the tree completely

[ren-zhijun-oracle]

In Trinidad framework, we have a customization / changeManager feature, one of the subfeatures there is to be able to move components around in the tree. The usecase is a very common Design-time at Run-time feature where the components are moved by Drag-And-Drop in a canvas. This feature is broken when we recently tried to update to Mojarra 2.1.7, so this is a regression from Mojarra for which we are requesting a fix a.s.a.p.

I have tried to scale down this feature in a simple managed bean code and attached the testcase. All I'm doing in the managed bean is reparenting the component to move, and expecting that 1. if the component tree state is restored by RI, the component is in its destination, or 2. if the jsf page is re-read and tags re-executed, then RI should re-instate the component is in its original position, and remove the component from its destination. The component being in its original position was the behavior from Mojarra until 2.1.7. From 2.1.7 onwards, the component that was moved is no more in the tree.

To test it, run the facelets jsf page provided as testcase (note the components are from Trinidad, so you will need to include Trinidad library, you can yourself emulate the behavior in any other component library you wish, since the essense of defect is showcased). In the page, click on the first button, that will move the component inside PanelBox1. If you set breakpoint in the bean code, you will see reparenting successful.

After this, when the page is re-rendered, the component is expected to be re-introduced in its original position (because it is in the page definition whose tags are processed), this was the behavior so far. However this behavior is no more 2.1.7 onwards, notice that the component is no more restored to the original location (this is the issue).

Environment

Any environment

Affected Versions

[2.1.7]

[ren-zhijun-oracle]

• Issue Imported From: https://github.com/javaserverfaces/mojarra/issues/2377
• Original Issue Raised By:@javaserverfaces
• Original Issue Assigned To: @manfredriem
• Original Issue Closed By:@javaserverfaces

[ren-zhijun-oracle]

@javaserverfaces Commented Reported by prakashudupa

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Can you supply at test case without Trinidad?

[ren-zhijun-oracle]

@javaserverfaces Commented prakashudupa said: I work on ADF Faces components / tags which is built on Trinidad, the issue needs some component library to be used, I'm not familiar with any other component library lower than Trinidad. I think the issue is explained well above, and attached code is easy to understand, you should be able to see the same issue in whatever library you try (like JSF tag lib for eg.).

[ren-zhijun-oracle]

@javaserverfaces Commented rogerk said: Here is my review of the recent changes:

1. Missing copyrights (2012) 2. StateManagementStrategyImpl has commented line (316?): +// actions.add(action); 3. StateContext : handleAdd method: Can you explain why you had to do:

• component.setId(null);
• component.setId(id);

4. test agnostic pom

• need to add "dynamic" module

5. From the original eventTest Acid test ui, everything passes except the toggle test.

All Mojarra tests run successfully.

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: 1. Addressed missing copyright. 2. Removed that line (it was used for debugging). 3. component.setId(id) was used to reset the clientId. 4. Added 'dynamic' module. 5. Wrote a new test to execute old toggle test.

[ren-zhijun-oracle]

@javaserverfaces Commented @edburns said:

• Add a landing page for the test app that has links to the pages used by the HtmlUnit tests.

• In StateManagementStrategyImpl.reapplyDynamicActions(), don't bother with the saving aside and restoring of the trackViewModifications property, as this is done already.

In the same method, add a comment to state the precondition that the caller is responsible for ensuring the trackViewModifications property is correctly saved and restored around the invocation of this method.

• Rename method StateManagementStrategyImpl.findComponent(). Suggestion: locateComponentByClientId().

• In addition to the suggested name change, StateManagementStrategyImpl.findComponent() may not operate correctly in partial traversal situations, with or without iterating components. Rewrite this method to first try to locate the component using UIComponent.invokeOnComponent(). Usage examples abound in the code. If that doesn't succeed in finding a component, then you can do your manual traversal.

• At the earliest point in time when this can be safely done, please add a try/finally block that ensures the dynamicActions are cleared from the stateContext after they have been processed.

• In StateManagementStrategyImpl.pruneAndReAddDynamicActions() keep only the so-called "english" part of the comment.

• In toggle.xhtml, remove text that mentions a "red box".

Apply these changes, run the tests, and let me have one more look at it. Then let's get this puppy in!

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Done the changes as requested by Ed

[ren-zhijun-oracle]

@javaserverfaces Commented @edburns said: This looks good. Please commit to the 2.1 branch.

r=edburns

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Applied to 2.1 branch

svn commit -m "Fixes http://java.net/jira/browse/JAVASERVERFACES-2373, r=edburns, tracking components that are removed so we can readd them if requested, add toggle and move test, see issue for more information" Sending jsf-ri\src\main\java\com\sun\faces\application\view\FaceletViewHandlingStrategy.java Sending jsf-ri\src\main\java\com\sun\faces\application\view\StateManagementStrategyImpl.java Sending jsf-ri\src\main\java\com\sun\faces\context\StateContext.java Sending jsf-ri\src\main\java\com\sun\faces\facelets\tag\jsf\ComponentSupport.java Sending jsf-ri\src\main\java\com\sun\faces\facelets\tag\jsf\ComponentTagHandlerDelegateImpl.java Sending jsf-ri\src\main\java\com\sun\faces\util\ComponentStruct.java Adding test\agnostic\dynamic Adding test\agnostic\dynamic\nbactions.xml Adding test\agnostic\dynamic\pom.xml Adding test\agnostic\dynamic\src Adding test\agnostic\dynamic\src\main Adding test\agnostic\dynamic\src\main\java Adding test\agnostic\dynamic\src\main\java\com Adding test\agnostic\dynamic\src\main\java\com\sun Adding test\agnostic\dynamic\src\main\java\com\sun\faces Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test\agnostic Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test\agnostic\dynamic Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test\agnostic\dynamic\MoveComponentBean.java Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test\agnostic\dynamic\ToggleBean.java Adding test\agnostic\dynamic\src\main\java\com\sun\faces\test\agnostic\dynamic\ToggleComponent.java Adding test\agnostic\dynamic\src\main\resources Adding test\agnostic\dynamic\src\main\webapp Adding test\agnostic\dynamic\src\main\webapp\WEB-INF Adding test\agnostic\dynamic\src\main\webapp\WEB-INF\dynamic.taglib.xml Adding test\agnostic\dynamic\src\main\webapp\WEB-INF\faces-config.xml Adding test\agnostic\dynamic\src\main\webapp\WEB-INF\glassfish-web.xml Adding test\agnostic\dynamic\src\main\webapp\WEB-INF\web.xml Adding test\agnostic\dynamic\src\main\webapp\index.xhtml Adding test\agnostic\dynamic\src\main\webapp\moveComponent.xhtml Adding test\agnostic\dynamic\src\main\webapp\toggle.xhtml Adding test\agnostic\dynamic\src\test Adding test\agnostic\dynamic\src\test\java Adding test\agnostic\dynamic\src\test\java\com Adding test\agnostic\dynamic\src\test\java\com\sun Adding test\agnostic\dynamic\src\test\java\com\sun\faces Adding test\agnostic\dynamic\src\test\java\com\sun\faces\test Adding test\agnostic\dynamic\src\test\java\com\sun\faces\test\agnostic Adding test\agnostic\dynamic\src\test\java\com\sun\faces\test\agnostic\dynamic Adding test\agnostic\dynamic\src\test\java\com\sun\faces\test\agnostic\dynamic\Issue2373IT.java Sending test\agnostic\pom.xml Transmitting file data .................... Committed revision 9869.

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Marking resolved pending testing

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Guarding against stateContext being null, or stateContext.getDynamicActions being null

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: svn commit -m "Fixes http://java.net/jira/browse/JAVASERVERFACES-2373, r=edburns, guard against stateContext and stateContext.getDynamicActions being null" Sending jsf-ri\src\main\java\com\sun\faces\application\view\StateManagementStrategyImpl.java Transmitting file data . Committed revision 9870.

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Marking resolved again.

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Make sure we don't call reapplyDynamicActions when doing full state saving

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Changes to make FSS working again with dynamic components

[ren-zhijun-oracle]

@javaserverfaces Commented @edburns said: http://java.net/jira/browse/JAVASERVERFACES-2373

These are Ed's notes when reviewing the 20120509 changebundle. Actionable comments addressed to Manfred start with mriem:.

Please look through these suggestions and address my comments. I think I'd like to see another iteration.

SECTION: test changes

M jsf-ri/test/com/sun/faces/application/TestViewHandlerImpl.java M jsf-ri/test/com/sun/faces/context/TestStateContext.java M jsf-ri/test/com/sun/faces/component/visit/TestTreeWithUIDataVisit.java M test/agnostic/dynamic/src/test/java/com/sun/faces/test/agnostic/dynamic/Issue2373IT.java A test/agnostic/dynamic/src/main/java/com/sun/faces/test/agnostic/dynamic/RemoveComponentBean.java M test/agnostic/dynamic/src/main/webapp/index.xhtml A test/agnostic/dynamic/src/main/webapp/removeComponent.xhtml

SECTION: State Management changes

M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java

General comments on this class:

• Overall, this class looks much cleaner, but I'm always suspicious of code that looks dramatically simpler, because stuff that looks dirty is often just bugfixes. Nonetheless, I'll trust that your changes simultaneously add clarity, remove complexity, and preserve bug fixes. That's a tall order, though!

• mriem: Four or so methods and two inner classes have been removed. Where did these end up?

• Remove apologies from class javadoc.

• Remove ctor, moved to JspStateManagementStrategy.

• Remove all ivars, moved to JspStateManagementStrategy.

In method saveView():

• mriem: on line 69, you access the context local variable, but check for its nullness on line 71. That's an NPE waiting to happen.

• mriem: you've removed the id uniqueness check from this method. I assume it will show up somewhere else. Please verify that it does.

In method restoreView():

• mriem: you've removed the wordy comment about the need to clone the tree... Is that business handled somewhere else now? Yes: looks like it's in the new class JspStateManagementStrategy.

A jsf-ri/src/main/java/com/sun/faces/application/view/JspStateManagementStrategy.java

• Assume the responsibilities from the former StateManagerImpl.java

M jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

• Removed. Functionality taken over by pair of classes: FaceletPartialStateManagementStrategy and FaceletFullStateManagementStrategy.

• mriem: But I note that

./test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

still has this old implementation. Is that intentional?

A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletPartialStateManagementStrategy.java

Global comments:

• mriem: you have a zero arg ctor, but it's only called by test code (and cactus code no less). To guard against someone calling that ctor for real, perhaps invent some kind of magic argument that is checked by the ctor and throws an IllegalStateException if it's not right? Something like a string with a value known by the test and the real code? The test will pass the literal string?

• Again, this looks much cleaner.

• In method saveView():

Ahh, here is the id uniqueness check.

How about adding a context param that makes this off when ProjectStage is Development? After sitting through Leonardo Uribe's MyFaces vs Mojarra performance comparison, I'm ready to entertain such expediences. A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java

• mriem: please add a comment that explains why this class looks so similar to JspStateManagementStrategy. And a question for you, how does this actually differ from JspStateManagementStrategy?

M jsf-ri/src/main/java/com/sun/faces/context/StateContext.java

• change name of method partialStateSaving() to be isPartialStateSaving(). Otherwise this file is unchanged.

M jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentSupport.java

• Change callsite of partialStateSaving().

M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java

• I note that FaceletPartialStateManagementStrategy has a locateComponentByClientId() method, and FaceletViewHandlingStrategy does too. How are these to incarnations of the same named method related? Can this be moved to Util?

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java

1. The methods have moved to both FaceletFullStateManagementStrategy and JspStateManagementStrategy.

2. In method saveView: will be moving that context.getViewRoot down into if block

3. Uniqueness check shows up in each of the StateManagementStrategy impls as it is their responsibility.

4. Correct, see 1. above.

test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

1. This is your local workspace playing a trick on you. The test/unit project downloads the latest installed copy from your local Maven repo. If you did not build using mvn.deploy.snapshot.local this is why that is happening. It won't be part of the commit.

Global comments:

1. Not sure what you mean to convey here. Yes it is possible to construct an instance of a particular implementation of the StateManagementStrategies.

A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java

They look so similar because they both originated from the same source. Eventually I hope we can make it so the JSP part can be turned off completely. Separating it out should make that easier.

M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java

Correct, more refactoring will need to be done but I did want to have a point where we could push it into a release. Eventually the dynamic parts of each StateManagementStrategy will be pulled up into FaceletViewHandlingStrategy and each StateManagementStrategy just saves state (and does not need to know about how to handle dynamic components).

The id uniqueness check.

I would like to delay that until after this issue. Lets file an issue to keep track of that.

---

Can we go ahead and get this in with the fix for StateManagerImpl #2 in place?

[ren-zhijun-oracle]

@javaserverfaces Commented @edburns said: MR> Can we go ahead and get this in with the fix for StateManagerImpl #2 in place?

Yes, please go ahead. r=edburns.

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Changes committed

[ren-zhijun-oracle]

@javaserverfaces Commented @manfredriem said: Verified and closing

[ren-zhijun-oracle]

@javaserverfaces Commented File: ChangeBeanOne.java Attached By: prakashudupa

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle-fss.txt Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle-newfiles.zip Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle.txt Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle2-newfiles.zip Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle2.txt Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: changebundle3.txt Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: newfiles-fss.zip Attached By: @manfredriem

[ren-zhijun-oracle]

@javaserverfaces Commented File: testCase.jsf Attached By: prakashudupa

[ren-zhijun-oracle]

@javaserverfaces Commented Issue-Links: is related to JAVASERVERFACES-2335