Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SPEC 1574 / MYFACES-4528 ] Ensure top level ul does not have border attribute #419

Merged
merged 1 commit into from
Dec 12, 2022
Merged

[SPEC 1574 / MYFACES-4528 ] Ensure top level ul does not have border attribute #419

merged 1 commit into from
Dec 12, 2022

Conversation

KyleAure
Copy link
Contributor

@KyleAure KyleAure commented Dec 9, 2022

Based on the SPEC issue here: jakartaee/faces#1574

When layout="list" the border attribute should be ignored.

When a form like this:

<h:form id="form">
	<h:selectManyCheckbox id="input" layout="list" border="5">
          ...
	</h:selectManyCheckbox>
</h:form>

Is rendered the current output is:

<html>
  <head/>
  <body>
    <form id="form" name="form" method="post" action="/LayoutAttribute/selectManyCheckBox.xhtml" enctype="application/x-www-form-urlencoded">
      <ul id="form:input" border="5">
        ...
      </ul>
  </body>
</html>

Based on the spec we should be rendering:

<html>
  <head/>
  <body>
    <form id="form" name="form" method="post" action="/LayoutAttribute/selectManyCheckBox.xhtml" enctype="application/x-www-form-urlencoded">
      <ul id="form:input">
        ...
      </ul>
  </body>
</html>

This is because we are incorrectly starting this element with table passthrough properties, instead of ul passthrough properties.

@KyleAure KyleAure force-pushed the 1574-layout-list-without-border branch from 1da27a4 to 95674e5 Compare December 9, 2022 20:11
@melloware
Copy link
Contributor

These look good to me thanks for the PR. They might want you to open a JIRA ticket if you have not already so we can track this fix.

@KyleAure KyleAure force-pushed the 1574-layout-list-without-border branch from 95674e5 to 05cfe39 Compare December 9, 2022 20:33
@KyleAure
Copy link
Contributor Author

KyleAure commented Dec 9, 2022

@melloware Thanks for the heads up. I created the issue here: https://issues.apache.org/jira/browse/MYFACES-4528
Sorry, should have done that first. This is my first commit to this community and didn't realize you were using Jira

@volosied
Copy link
Contributor

volosied commented Dec 9, 2022

This looks good -- just tested locally. Thanks for providing a patch and making the JIRA.

Related to which, I think the community should create an issues tab to allow users to create issues in this repo since JIRA is disabled for new signups. We can follow on on this later.

@volosied volosied changed the title [SPEC 1574] Ensure top level ul does not have border attribute [SPEC 1574 / MYFACES-4528 ] Ensure top level ul does not have border attribute Dec 9, 2022
@melloware
Copy link
Contributor

I actually prefer GitHub Issues myself but yes an easy way for users to report issue we can always make a JIRA ticket if we discover its a real issue

@melloware
Copy link
Contributor

Also does this fix need to be backported to 2.3 and 2.3-next?

@volosied
Copy link
Contributor

I think so. It's a fairly simple fix and should apply easily to those branches.

@volosied volosied merged commit 67f37c6 into apache:main Dec 12, 2022
@KyleAure KyleAure deleted the 1574-layout-list-without-border branch December 12, 2022 15:09
@volosied
Copy link
Contributor

@melloware It looks like the layout=list was only added to 4.0 (per the spec). I thought it was added to 2.3-next, but was mistaken. So no need to backport.

#224

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

@melloware melloware melloware approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@KyleAure @melloware @volosied