ICEfaces
  1. ICEfaces
  2. ICE-4271

To get the DOMContext after initialization components should call getDOMContext instead of attachDOMContext

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.1
    • Fix Version/s: 1.8.2-RC1, 1.8.2
    • Component/s: ICE-Components
    • Labels:
      None
    • Environment:
      ICEfaces

      Description

      DOMContext.attach() sometimes creates a malformed subtree that is discarded by DOM error checking. This is the case with the popup menu component. In general the attach() mechanism is inefficient and should be removed. At this time, however, the CommandLinkRenderer makes use of the attach() functionality, possibly through its use of two different root nodes ("a" and "span" depending on whether the component is disabled.

      The recommended fix is to remove the attach() functionality and update the CommandLinkRenderer to a pure ResponseWriter implementation.

        Activity

        Hide
        Ted Goddard added a comment -

        Please implement CommandLinkRenderer without DOMContext .

        Show
        Ted Goddard added a comment - Please implement CommandLinkRenderer without DOMContext .
        Hide
        Ken Fyten added a comment -

        Please survey all of our standard-extended components to see if they use attach also.

        Show
        Ken Fyten added a comment - Please survey all of our standard-extended components to see if they use attach also.
        Hide
        Adnan Durrani added a comment -

        The real call is DOMContext.attachDOMContext() and it came across that almost all components are using it.

        Show
        Adnan Durrani added a comment - The real call is DOMContext.attachDOMContext() and it came across that almost all components are using it.
        Hide
        Ted Goddard added a comment -

        This is true, however the actual problem is the state that the DOM is in at the moment of the attachDOMContext() call. See the comment here in the implementation of the attachDOMContext() method:

        //context may have been severed from the tree at some point
        if (context.isInitialized()) {
        if (!(cursorParent instanceof Element))

        { context.stepOver(); return context; }

        //TODO: Remove as per ICE-4088
        //both MenuBarRenderer.encodeEnd and
        //CommandLinkRenderer.encodeBegin make use of the
        //attach method and should be fixed
        context.attach((Element) cursorParent);
        }

        One procedure for fixing this bug would be to throw an exception from the attach() method so that the components could be verified as fixed, then remove the code from DOMContext.

        Show
        Ted Goddard added a comment - This is true, however the actual problem is the state that the DOM is in at the moment of the attachDOMContext() call. See the comment here in the implementation of the attachDOMContext() method: //context may have been severed from the tree at some point if (context.isInitialized()) { if (!(cursorParent instanceof Element)) { context.stepOver(); return context; } //TODO: Remove as per ICE-4088 //both MenuBarRenderer.encodeEnd and //CommandLinkRenderer.encodeBegin make use of the //attach method and should be fixed context.attach((Element) cursorParent); } One procedure for fixing this bug would be to throw an exception from the attach() method so that the components could be verified as fixed, then remove the code from DOMContext.
        Hide
        Adnan Durrani added a comment -

        I dump the stack trace from the context.attach() and found that the following components hitting the context.attach() method.

        inputSecret
        selectOneMenu
        selectManyMenu
        selectOneListbox
        selectManyListbox
        MenuBar
        outputLabel
        outputChart
        panelCollapsible
        panelDivider
        panelSeries
        PanelStack

        Rest of the components do not hit context.attach() .

        Show
        Adnan Durrani added a comment - I dump the stack trace from the context.attach() and found that the following components hitting the context.attach() method. inputSecret selectOneMenu selectManyMenu selectOneListbox selectManyListbox MenuBar outputLabel outputChart panelCollapsible panelDivider panelSeries PanelStack Rest of the components do not hit context.attach() .
        Hide
        Adnan Durrani added a comment -

        The context.attach() being conditionally called in the DOMContext.attachDOMContext(). Here is the code snippet:
        public static DOMContext attachDOMContext(..) {
        ......
        if (context.isInitialized())

        { .... context.attach((Element) cursorParent); }

        }

        We can see the method gets invoked when context is initialized.

        • What sets initialize to "true"?
          The initialize property being set to true, when renderer calls DOMContext.setRootNode();
        • How many times renderer invokes the DOMContext.attachDOMContext()?
          Only once.
        • In what sequence renderer calls these methods?
          DOMContext.attachDOMContext() //initialize is false
          DOMContext.setRootNode() //set initialize to true

        By looking at above call sequence it seems like the context.attach( ) should never be called.

        • Why context.attach() is being called then?
          All of our components extends DOMBasicRenderer, which defines the encodeEnd method please see the snippet below.
          public void encodeEnd(...) { renderEnd(..); //renderer overrides it. DOMContext domContext = DOMContext.attachDOMContext(facesContext, uiComponent); domContext.stepOver(); }

        AS you can see it makes a call of attachDOMContext. So any of the component which doesn't override its encodeEnd ends up calling attachDOMContext twice, which will cause context.attach( ) to be called.

        Show
        Adnan Durrani added a comment - The context.attach() being conditionally called in the DOMContext.attachDOMContext(). Here is the code snippet: public static DOMContext attachDOMContext(..) { ...... if (context.isInitialized()) { .... context.attach((Element) cursorParent); } } We can see the method gets invoked when context is initialized. What sets initialize to "true"? The initialize property being set to true, when renderer calls DOMContext.setRootNode(); How many times renderer invokes the DOMContext.attachDOMContext()? Only once. In what sequence renderer calls these methods? DOMContext.attachDOMContext() //initialize is false DOMContext.setRootNode() //set initialize to true By looking at above call sequence it seems like the context.attach( ) should never be called. Why context.attach() is being called then? All of our components extends DOMBasicRenderer, which defines the encodeEnd method please see the snippet below. public void encodeEnd(...) { renderEnd(..); //renderer overrides it. DOMContext domContext = DOMContext.attachDOMContext(facesContext, uiComponent); domContext.stepOver(); } AS you can see it makes a call of attachDOMContext. So any of the component which doesn't override its encodeEnd ends up calling attachDOMContext twice, which will cause context.attach( ) to be called.
        Hide
        Ted Goddard added a comment -

        That's interesting; is it just a matter of implementing encodeEnd() then?

        Show
        Ted Goddard added a comment - That's interesting; is it just a matter of implementing encodeEnd() then?
        Hide
        Adnan Durrani added a comment -

        Yes, implementing encodeEnd() would prevent invoking attachDOMContext twice but the encodeEnd has some effect and message related code applies to all component.

        I think we should remove the following two lines from the DOMBasicRenderer.encodeEnd

        DOMContext domContext = DOMContext.attachDOMContext(facesContext, uiComponent);
        domContext.stepOver();

        and if stepOver() is required we should use getDOMContext instead:

        DOMContext domContext = DOMContext.getDOMContext(facesContext, uiComponent);
        domContext.stepOver();

        Show
        Adnan Durrani added a comment - Yes, implementing encodeEnd() would prevent invoking attachDOMContext twice but the encodeEnd has some effect and message related code applies to all component. I think we should remove the following two lines from the DOMBasicRenderer.encodeEnd DOMContext domContext = DOMContext.attachDOMContext(facesContext, uiComponent); domContext.stepOver(); and if stepOver() is required we should use getDOMContext instead: DOMContext domContext = DOMContext.getDOMContext(facesContext, uiComponent); domContext.stepOver();
        Hide
        Ted Goddard added a comment -

        That looks good.

        Show
        Ted Goddard added a comment - That looks good.
        Hide
        Adnan Durrani added a comment - - edited

        The following complete list of components is generated using the component-showcase, all of them reach context.attach():

        class --->>>> class com.icesoft.faces.component.outputconnectionstatus.OutputConnectionStatus
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneMenu
        class --->>>> class com.icesoft.faces.component.panelcollapsible.PanelCollapsible
        class --->>>> class com.icesoft.faces.component.panelstack.PanelStack
        class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyMenu
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneListbox
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyListbox
        class --->>>> class com.icesoft.faces.component.ext.HtmlMessage
        class --->>>> class com.icesoft.faces.component.ext.HtmlInputSecret
        class --->>>> class com.icesoft.faces.component.selectinputtext.SelectInputText
        class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel
        class --->>>> class com.icesoft.faces.component.outputchart.OutputChart
        class --->>>> class com.icesoft.faces.component.outputresource.OutputResource
        class --->>>> class com.icesoft.faces.component.panelseries.PanelSeries
        class --->>>> class com.icesoft.faces.component.gmap.GMap
        class --->>>> class com.icesoft.faces.component.menubar.MenuItem
        class --->>>> class com.icesoft.faces.component.menubar.MenuBar
        class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup
        class --->>>> class com.icesoft.faces.component.menubar.MenuItem
        class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup
        class --->>>> class com.icesoft.faces.component.inputrichtext.InputRichText
        class --->>>> class com.icesoft.faces.component.paneldivider.PanelDivider
        class --->>>> class com.icesoft.faces.component.panelpositioned.PanelPositioned

        Changing "attachDOMContext" to "getDOMContext" trims down the list as follows:

        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneMenu
        class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyMenu
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneListbox
        class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup
        class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyListbox
        class --->>>> class com.icesoft.faces.component.panelseries.PanelSeries
        class --->>>> class com.icesoft.faces.component.panelpositioned.PanelPositioned

        Show
        Adnan Durrani added a comment - - edited The following complete list of components is generated using the component-showcase, all of them reach context.attach(): class --->>>> class com.icesoft.faces.component.outputconnectionstatus.OutputConnectionStatus class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneMenu class --->>>> class com.icesoft.faces.component.panelcollapsible.PanelCollapsible class --->>>> class com.icesoft.faces.component.panelstack.PanelStack class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyMenu class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneListbox class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyListbox class --->>>> class com.icesoft.faces.component.ext.HtmlMessage class --->>>> class com.icesoft.faces.component.ext.HtmlInputSecret class --->>>> class com.icesoft.faces.component.selectinputtext.SelectInputText class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel class --->>>> class com.icesoft.faces.component.outputchart.OutputChart class --->>>> class com.icesoft.faces.component.outputresource.OutputResource class --->>>> class com.icesoft.faces.component.panelseries.PanelSeries class --->>>> class com.icesoft.faces.component.gmap.GMap class --->>>> class com.icesoft.faces.component.menubar.MenuItem class --->>>> class com.icesoft.faces.component.menubar.MenuBar class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup class --->>>> class com.icesoft.faces.component.menubar.MenuItem class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup class --->>>> class com.icesoft.faces.component.inputrichtext.InputRichText class --->>>> class com.icesoft.faces.component.paneldivider.PanelDivider class --->>>> class com.icesoft.faces.component.panelpositioned.PanelPositioned Changing "attachDOMContext" to "getDOMContext" trims down the list as follows: class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneMenu class --->>>> class com.icesoft.faces.component.ext.HtmlOutputLabel class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyMenu class --->>>> class com.icesoft.faces.component.ext.HtmlSelectOneListbox class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup class --->>>> class com.icesoft.faces.component.ext.HtmlSelectManyListbox class --->>>> class com.icesoft.faces.component.panelseries.PanelSeries class --->>>> class com.icesoft.faces.component.panelpositioned.PanelPositioned
        Hide
        Ted Goddard added a comment -

        The problem with attach() is actually the following code, so if components do not reach this, the method can be removed:

        if (rootNode.getParentNode() != cursorParent) {
        try

        { //re-attaching on top of another node //replace them and assume they will re-attach later cursorParent.appendChild(rootNode); }

        catch (DOMException e)

        { //this happens in strea-write mode only. }

        }

        The real problem is the DOMException that is ignored, so for work on this bug, the exception should be printed.

        Show
        Ted Goddard added a comment - The problem with attach() is actually the following code, so if components do not reach this, the method can be removed: if (rootNode.getParentNode() != cursorParent) { try { //re-attaching on top of another node //replace them and assume they will re-attach later cursorParent.appendChild(rootNode); } catch (DOMException e) { //this happens in strea-write mode only. } } The real problem is the DOMException that is ignored, so for work on this bug, the exception should be printed.
        Hide
        Adnan Durrani added a comment -

        There isn't any logger defined for this class, so I just Sysout the exception and removed all my changes. When I tested the component-showcase only menuPopup is throwing this.
        class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup
        org.w3c.dom.DOMException: HIERARCHY_REQUEST_ERR: An attempt was made to insert a node where it is no
        t permitted.

        Show
        Adnan Durrani added a comment - There isn't any logger defined for this class, so I just Sysout the exception and removed all my changes. When I tested the component-showcase only menuPopup is throwing this. class --->>>> class com.icesoft.faces.component.menupopup.MenuPopup org.w3c.dom.DOMException: HIERARCHY_REQUEST_ERR: An attempt was made to insert a node where it is no t permitted.
        Hide
        Adnan Durrani added a comment -

        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\menupopup\MenuPopupRenderer.java
        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelpositioned\PanelPositionedRenderer.java
        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelseries\PanelSeriesRenderer.java
        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\DomBasicRenderer.java
        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\LabelRenderer.java
        Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\MenuRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\MenuRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelseries\PanelSeriesRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelpositioned\PanelPositionedRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\LabelRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\menupopup\MenuPopupRenderer.java
        Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\DomBasicRenderer.java
        Completed: At revision: 19064

        Show
        Adnan Durrani added a comment - Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\menupopup\MenuPopupRenderer.java Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelpositioned\PanelPositionedRenderer.java Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelseries\PanelSeriesRenderer.java Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\DomBasicRenderer.java Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\LabelRenderer.java Modified: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\MenuRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\MenuRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelseries\PanelSeriesRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\panelpositioned\PanelPositionedRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\LabelRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\component\src\com\icesoft\faces\component\menupopup\MenuPopupRenderer.java Sending content: D:\work\development\head\svn\ossrepo\icefaces\trunk\icefaces\core\src\com\icesoft\faces\renderkit\dom_html_basic\DomBasicRenderer.java Completed: At revision: 19064
        Hide
        Adnan Durrani added a comment - - edited

        Changes made to the renderers to make sure components are calling getDOMContext to get the DOMContext after initialization and the DOMException being printed.

        I confirmed that the context.attach() never gets hit by any component.

        Show
        Adnan Durrani added a comment - - edited Changes made to the renderers to make sure components are calling getDOMContext to get the DOMContext after initialization and the DOMException being printed. I confirmed that the context.attach() never gets hit by any component.

          People

          • Assignee:
            Adnan Durrani
            Reporter:
            Ted Goddard
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: