New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SelectManyCheckBox grid layout - closing tr-tag not rendered #4474

Closed
michael-knapp-j4care opened this Issue Jan 26, 2019 · 14 comments

Comments

Projects
None yet
4 participants
@michael-knapp-j4care
Copy link

michael-knapp-j4care commented Jan 26, 2019

primefaces 7.0.RC1 - org.primefaces.component.selectmanycheckbox.SelectManyCheckboxRenderer

Obvious bug: if the number of items in 'selectItem' is not a multiple of 'colums', the closing 'tr'-tag is not rendered. Result is an unbalanced DOM tree with ambiguous effects in the web browser. (parts of page not displayed, misalignments, ...)


protected void encodeGridLayout(FacesContext context, SelectManyCheckbox checkbox) throws IOException {
        ResponseWriter writer = context.getResponseWriter();
        List<SelectItem> selectItems = getSelectItems(context, checkbox);
        Converter converter = checkbox.getConverter();
        Object values = getValues(checkbox);
        Object submittedValues = getSubmittedValues(checkbox);
        int columns = checkbox.getColumns();

        if (columns != 0) {
            int idx = 0;
            int colMod;
            for (int i = 0; i < selectItems.size(); i++) {
                SelectItem selectItem = selectItems.get(i);
                colMod = idx % columns;
                if (colMod == 0) {
                    writer.startElement("tr", null);
                }

                writer.startElement("td", null);
                encodeOption(context, checkbox, values, submittedValues, converter, selectItem, idx);
                writer.endElement("td");

                idx++;
                colMod = idx % columns;

                if (colMod == 0) {
                    writer.endElement("tr");
                }
            }
        }
        else {
            throw new FacesException("The value of columns attribute must be greater than zero.");
        }
    }

@michael-knapp-j4care michael-knapp-j4care changed the title SelectManyCheckBox grid layout SelectManyCheckBox grid layout - closing tr-tag not rendered Jan 26, 2019

@tandraschko

This comment has been minimized.

Copy link
Member

tandraschko commented Jan 26, 2019

Was already fixed for Panelgrid: #4122

Would be great if you can provide a PR

@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 26, 2019

@michael-knapp-j4care

This comment has been minimized.

Copy link
Author

michael-knapp-j4care commented Jan 26, 2019

My suggested fix: (I'm not used to git and pull requests)

    protected void encodeGridLayout(FacesContext context, SelectManyCheckbox checkbox) throws IOException {
        ResponseWriter writer = context.getResponseWriter();
        List<SelectItem> selectItems = getSelectItems(context, checkbox);
        Converter converter = checkbox.getConverter();
        Object values = getValues(checkbox);
        Object submittedValues = getSubmittedValues(checkbox);
        int columns = checkbox.getColumns();

        if (columns > 0) {
            int idx = 0;
            int rows = (selectItems.size() + columns - 1) / columns;
            for (int r = 0; r < rows; r++) {
                writer.startElement("tr", null);
                for (int c = 0; c < columns; c++) {
                    SelectItem selectItem = (idx < selectItems.size()) ? selectItems.get(idx) : null;
                    idx++;
                    writer.startElement("td", null);
                    if (selectItem != null) {
                        encodeOption(context, checkbox, values, submittedValues, converter, selectItem, idx);
                    }
                    writer.endElement("td");
                }
                writer.endElement("tr");
            }
        }
        else {
            throw new FacesException("The value of columns attribute must be greater than zero.");
        }
    }

@tandraschko tandraschko added the defect label Jan 27, 2019

@Rapster

This comment has been minimized.

Copy link
Member

Rapster commented Jan 27, 2019

@michael-knapp-j4care Well, there is always a first time, have a try 😉

@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 28, 2019

@michael-knapp-j4care I am trying to reproduce this using the Showcase code...

            <p:selectManyCheckbox id="grid" value="#{checkboxView.selectedCities}" layout="grid" columns="4">
                <f:selectItems value="#{checkboxView.cities}" var="city" itemLabel="#{city}" itemValue="#{city}" />
            </p:selectManyCheckbox>

The selectItems=9 and I have tried columns=2 (not a multiple of 9), columns=3 (is multiple), columns=4 (not a multiple) and all of it renders fine with the proper closing tag. Can you give me an example of where it is producing the problem?

@tandraschko

This comment has been minimized.

Copy link
Member

tandraschko commented Jan 28, 2019

@melloware did you use Mojarra 2.3? Other JSF impls auto closes it.

@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 28, 2019

I am using Mojarra-2.3.2 which is default in the showcase. Which one should I use?

@tandraschko

This comment has been minimized.

Copy link
Member

tandraschko commented Jan 28, 2019

should be fine.
So we need a example, otherwise -> 'can't replicate'

@michael-knapp-j4care

This comment has been minimized.

Copy link
Author

michael-knapp-j4care commented Jan 28, 2019

The original code is wrong logically, the error is even obvious by looking at it. Just looking at the browser's output is not the way how to check the correctness.
Are there some HTML beautifiers or similar things after the HTML generation?
With simple examples it may work, because browsers are quite robust with simple errors in XHTML code.
It usually affectes elements put after the SelectManyCheckBox item
I've used it in a quite complex page, which messed up parts of the page. That depends how the browser interprets the broken XHTML code and 'corrects' it.
I don't think, that the browser's "source view" displays the original code as received but some "corrected" code
Is there a way to output the "writer"'s contents?
I may set up a larger example, but I do not have time for it until next week.
I've checked out the 7.0-SNAPSHOT and fixed that problem myself. With the fix applied my page works. (on Google Chrome version 71.0.3578.98)

@tandraschko

This comment has been minimized.

Copy link
Member

tandraschko commented Jan 28, 2019

I don't think so as the DOM levels were completely broken with #4122.
It would be great if you could provide a sample this week as we will probably release RC2.

Also "view page source" in firefox e.g. should really print the plain rendered html.

@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 28, 2019

@michael-knapp-j4care I agree the code just looks broken looking at it but without a reproducible test case to prove its broken so we can prove its fixed its hard to commit a change like this.

@michael-knapp-j4care

This comment has been minimized.

Copy link
Author

michael-knapp-j4care commented Jan 28, 2019

Sample code with 7 Elements

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE html>
<html 
	xmlns="http://www.w3.org/1999/xhtml"
	xmlns:h="http://java.sun.com/jsf/html"
	xmlns:f="http://java.sun.com/jsf/core"
	xmlns:ui="http://java.sun.com/jsf/facelets"
	xmlns:p="http://primefaces.org/ui"
>
	<h:head>
		<title>SelectManyCheckbox Bug</title>
	</h:head>

	<h:body>
		<p:panel>
			<p:selectManyCheckbox id="stringsManyCheckbox"  layout="grid" columns="4" >
		        <f:selectItem itemValue="1" itemLabel="1" />
		        <f:selectItem itemValue="2" itemLabel="2" />
		        <f:selectItem itemValue="3" itemLabel="3" />
		        <f:selectItem itemValue="4" itemLabel="4" />
		        <f:selectItem itemValue="5" itemLabel="5" />
		        <f:selectItem itemValue="6" itemLabel="6" />
		        <f:selectItem itemValue="7" itemLabel="7" />
		    </p:selectManyCheckbox>
		</p:panel>
	</h:body>
</html>

Sample source on Firefox, displayed with the "view source" option (the DOM tree in the browser's debugger is different from the source)
Note the missing closing tag in the line between the blank lines, which I've added to highlight
But it is displayed correctly in the browser, but this does not mean that this is always the case.

<?xml` version="1.0" encoding="UTF-8" ?>
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"><head id="j_idt2"><link type="text/css" rel="stylesheet" href="/SMooTHViewer/javax.faces.resource/theme.css.xhtml?ln=primefaces-bootstrap" /><link type="text/css" rel="stylesheet" href="/SMooTHViewer/javax.faces.resource/fa/font-awesome.css.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT" /><link type="text/css" rel="stylesheet" href="/SMooTHViewer/javax.faces.resource/components.css.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT" /><script type="text/javascript" src="/SMooTHViewer/javax.faces.resource/jquery/jquery.js.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT"></script><script type="text/javascript" src="/SMooTHViewer/javax.faces.resource/jquery/jquery-plugins.js.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT"></script><script type="text/javascript" src="/SMooTHViewer/javax.faces.resource/core.js.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT"></script><script type="text/javascript" src="/SMooTHViewer/javax.faces.resource/components.js.xhtml?ln=primefaces&amp;v=7.0-SNAPSHOT"></script><script type="text/javascript">if(window.PrimeFaces){PrimeFaces.settings.locale='de_DE';PrimeFaces.settings.projectStage='Development';}</script>
		<title>SelectManyCheckbox Bug</title></head><body><div id="j_idt5" class="ui-panel ui-widget ui-widget-content ui-corner-all" data-widget="widget_j_idt5"><div id="j_idt5_content" class="ui-panel-content ui-widget-content"><table id="stringsManyCheckbox" role="presentation" class="ui-selectmanycheckbox ui-widget"><tr><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:0" name="stringsManyCheckbox" type="checkbox" value="1" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:0">1</label></td><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:1" name="stringsManyCheckbox" type="checkbox" value="2" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:1">2</label></td><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:2" name="stringsManyCheckbox" type="checkbox" value="3" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:2">3</label></td><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:3" name="stringsManyCheckbox" type="checkbox" value="4" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:3">4</label></td></tr><tr><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:4" name="stringsManyCheckbox" type="checkbox" value="5" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:4">5</label></td><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:5" name="stringsManyCheckbox" type="checkbox" value="6" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span></div></div><label for="stringsManyCheckbox:5">6</label></td><td><div class="ui-chkbox ui-widget"><div class="ui-helper-hidden-accessible"><input id="stringsManyCheckbox:6" name="stringsManyCheckbox" type="checkbox" value="7" /></div><div class="ui-chkbox-box ui-widget ui-corner-all ui-state-default"><span class="ui-chkbox-icon ui-icon ui-icon-blank ui-c"></span>

</div></div><label for="stringsManyCheckbox:6">7</label></td></table><script 

id="stringsManyCheckbox_s" type="text/javascript">$(function(){PrimeFaces.cw("SelectManyCheckbox","widget_stringsManyCheckbox",{id:"stringsManyCheckbox"});});</script></div></div><script id="j_idt5_s" type="text/javascript">$(function(){PrimeFaces.cw("Panel","widget_j_idt5",{id:"j_idt5"});});</script></body>
</html>
@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 28, 2019

@melloware

This comment has been minimized.

Copy link
Contributor

melloware commented Jan 29, 2019

PR Submitted for review. Test case attached here:
pf-4474.zip

I have added the TBODY tags and properly closed out the TR tags. Also slight refactoring to move the FacesException up to "fail fast".

tandraschko added a commit that referenced this issue Jan 30, 2019

Merge pull request #4479 from melloware/PF4474
Fix #4474: SelectManyCheckBox grid layout not rendering proper HTML.

@tandraschko tandraschko added this to the 7.0 milestone Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment