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

UI - PrimeFaces 6.2 Upgrade #4220

Closed
mheppler opened this Issue Oct 19, 2017 · 27 comments

Comments

Projects
None yet
8 participants
@mheppler
Contributor

mheppler commented Oct 19, 2017

Upgrade our Primefaces library from v5.3 to v6.1. Read more here: https://www.primefaces.org/documentation/

Primefaces 6.0 has been out for over a year and a half. They provide improved "accessibility of the components for 508 compliance regarding keyboard and screen reader support." 6.1 has been out for 6 months, and also boasts "major accessibility enhancements" as well as "various new features" for the DataTable component. 6.2 is planned for the end of 2017.

@mheppler

This comment has been minimized.

Contributor

mheppler commented Oct 19, 2017

Also consider other UI libraries and themes to avoid the PrimeFaces vs Bootstrap conflicts.

BootsFaces
http://showcase.bootsfaces.net/integration/PrimeFaces.jsf

PrimeFaces Avalon
https://www.primefaces.org/primefaces-avalon/

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jan 19, 2018

Accessibility issues with the selectManyCheckbox component have been reported in #4423, which have been fixed in PrimeFaces 6.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jan 19, 2018

The release date for 6.2 is now early February. A release candidate is available @ https://www.primefaces.org/primefaces-6-2-rc1-released-california/

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jan 29, 2018

Review if this related #2634 issue with PrimeFaces generated checkboxes is still resulting in alerts and errors in the ADA web tool. Potential fixes in 6.0+ referenced here and here.

@mheppler

This comment has been minimized.

Contributor

mheppler commented Feb 13, 2018

Dev investigated this update in Spike: investigate upgrading to Primefaces 6.1.1 #3883

@mheppler

This comment has been minimized.

Contributor

mheppler commented May 15, 2018

PrimeFaces 6.2 was released in Feb 2018 and in addition to including the latest and greatest jQuery, the new feature that caught my eye related to page load performance improvements!

...inline scripts are combined and moved to the bottom of the screen, this drastically improves the page load times and reduces the page size as well.

@mheppler

This comment has been minimized.

Contributor

mheppler commented May 16, 2018

PrimeFaces has provided this Migration Guide in their GitHub wiki, which outlines "changes that may cause backward compatibility issues in your applications".

@djbrooke djbrooke changed the title from UI - Primefaces 6 Upgrade to UI - Primefaces 6.2 Upgrade May 16, 2018

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 20, 2018

See my comment regarding jQuery libraries in issue UI - Bootstrap 4 Upgrade #4219.

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 21, 2018

After reviewing the Migration Guide from PrimeFaces, there doesn't seem to be a whole lot too major. The biggest change with the most impact seems to be the deprecated p:component, which returned 32 matches in 7 files.

p:component has been deprecated. Use p:resolveClientId('@id(myTable)', view) instead of p:component('myTable'). It also supports all search expressions other than ids. See primefaces/primefaces#2771

I will continue to get as much of this migration tackled as I can, but will recruit developers for some of the heavy lifting if it looks a little out of my wheelhouse. Stay tuned, PrimeFaces fans!

mheppler added a commit that referenced this issue Jun 22, 2018

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 22, 2018

For the deprecated p:component, changes have made to the following workflows:

- dataset pg > versions tab > version details load after page load
- dataset pg > locks > refresh
- replace file pg > warning popup file type different > delete
- upload/edit files pg > delete confirm popup > continue
- dataverse pg > results table > thumbnail imgs load after page load
- theme + widgets pg > theme tab form > save changes > message panel (error?)
- dataverse permission pg > roles panel > add role > roles edit popup
- dataverse permission pg > roles panel > edit role > roles edit popup
- dataverse permission pg > roles panel > copy role > roles edit popup
- dataverse permission pg > permissions panel > edit access > popup > save
- dataverse permission pg > users/groups panel > user/group table > remove assigned role > popup > continue
- files permission pg > user/group panel > user/group request table > file count link > grant access popup
- files permission pg > user/group panel > user/group request table > grant > userGroups, restrictedFiles, fileAccessRequests, userGroupsRequests, userGroupMessages
- files permission pg > user/group panel > user/group request table > reject > userGroups, restrictedFiles, fileAccessRequests, userGroupsRequests, userGroupMessages
- files permission pg > user/group panel > user/group access table > file count link > file access popup
- files permission pg > user/group panel > user/group access table > remove access > userGroups, restrictedFiles
- files permission pg > user/group panel > user/group access table > remove access > popup > continue > userGroups, restrictedFiles
- files permission pg > restricted files panel > files table > assign access > grant file access popup
- files permission pg > restricted files panel > files table > users/group count link > file access popup

sekmiller added a commit that referenced this issue Jun 22, 2018

sekmiller added a commit that referenced this issue Jun 22, 2018

@sekmiller

This comment has been minimized.

Contributor

sekmiller commented Jun 22, 2018

UPDATE 6/28 by @mheppler - reverted these changes 98b2431

For the deprecated autoUpdate=true, changes have been made to the following:

  • DatasetPage > metadataFragment > view edit metadata
  • Dataverse pg> edit dataverse contacts info
  • Dataverse pg> Theme/Widgets > view/edit theme and widgets

autoUpdate attribute of outputPanel, fragment, messages and growl has been deprecated. In 6.2 every component can be autoUpdateable by adding <p:autoUpdate /> as child. See primefaces/primefaces#2838

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 22, 2018

OK, I have combed over the Migration Guide and picked off the low hanging p:component fruit. Thank you, @sekmiller for jumping on the other p:autoUpdate change. General functionality like creating an account, dataverse, dataset, ingest files, publish, request/grant/reject access, et al, all seem to be functioning as expecting.

One change that caught my attention in the Migration Guide, but I wasn't sure how to find in the code was this bullet from the 5.3 to 6.0 changes.

Per default, DataTable now skips processing child components for some events like paging. If you need child processing, you can set skipChildren="false" on p:ajax.

If anyone has any thoughts on this one, let me know. If it relates to performance improvements, it could be worth investigating as part of Dataset Files DataTable - Paginator, Selection, Download, Counter #4656

I am going to pass the upgrade baton from here. Earlier I had talked to @matthew-a-dunlap about having a go at what he might find. There are some issues referenced in comments above that I am going to check to see if the upgrade resolved or not.

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 25, 2018

In my comments above, I had referenced changes in PrimeFaces 6 related to the selectManyCheckbox component, as well as checkboxes in general, that I hoped would resolve some older issues, but that does not seem to be the case unfortunately.

Re: selectManyCheckbox component #4423 -- while I was able to add the tabindex to the component, it was not being recognized while tabbing through the form, so I reverted the change.

Re: PrimeFaces generated checkboxes #2634 -- it appears that there have been improvements to the checkboxes which added the aria-checked attribute, but there were no changes to the missing label element the ADA warnings found.

I will add a comment to update both of those issues, as a reference for when we pick those issues for development.

Without any other changes to be made related to the upgrade, I am moving this to Code Review.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jun 26, 2018

I looked around dataverse with PrimeFaces 6.2 and did not see any issues at all. Prov/Publish/Thumbnails/etc seem to work correctly. Looking back over my old notes I hadn't seen much with my spike upgrade either.

I think at this point we should move on to QA.

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 27, 2018

While looking at this branch, I happened to find a mysterious bug that I can't quite nail down. In the metadataFragment.xhtml code, which is included into Edit Dataset (dataset.xhtml) and Create Dataset Template (template.xhtml), the "Add" ("+" icon) button is broken, and I am not seeing any related errors in the glassfish log or the browser console.

<p:commandLink tabindex="#{block.index+1}" title="#{bundle.add}"
    styleClass="btn btn-default btn-sm bootstrap-button-tooltip #{dsf.datasetFieldType.compound ? 'compound-field-btn' : ''}"
    actionListener="#{dsf.addDatasetFieldValue(valCount.index + 1)}"
    oncomplete="javascript:bind_bsui_components();">
        <span class="glyphicon glyphicon-plus no-text"/>
</p:commandLink>

Could not replicate this bug on demo or locally on the develop branch. Did not find anything related to this button's code in the Migration Guide.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jun 28, 2018

@mheppler The cause of this seems to be related to the deprecation of autoUpdate as an attribute for fragments (and other components), and our switch to using the p:autoUpdate tag. It looks like we followed the PrimeFaces upgrade guide correctly yet the update does not take place. If I switch back to the old autoUpdate format the page updates correctly.

There is an old comment on that page about buggy autoUpdate behaviour. Looks like in the past we had to create a dummy component to get the update to work.

<!-- There is an issue with the dynamic +/- and the primefaces autoupdate where they are not working well together for the primitive fields, but are for the compound fields. So as a workaround for the primitive, we wrap them in a ui:repeat that iterates on a single list. Note: dummyVar is never used-->

I removed this dummy ui:repeat and the deprecated autoUpdate still works, but the newer <p:autoUpdate /> still does not.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jun 28, 2018

I found one forum post with someone experiencing a similar issue, but no resolution: https://forum.primefaces.org/viewtopic.php?f=3&t=55471.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jun 28, 2018

I tried checking to see if any of our autoUpdate'd components work, but I think that only the one for metadataFragment.xhtml for Compound Fields is needed. The one in the same file for Primitive Fields does not seem to matter with our stock metadata (those primitives can't be +/-). The last one in themesAndWidgetsFragment.xhml doesn't seem to be needed for any of the interactions.

@mheppler mheppler assigned mheppler and unassigned kcondon Jun 28, 2018

@mheppler

This comment has been minimized.

Contributor

mheppler commented Jun 28, 2018

Reverted p:autoUpdate changes due to unexpected bugs (98b2431). The old attribute autoUpdate="true" still works and the "+" button functionality has returned to the forms on the add dataverse, add dataset and theme + widgets pgs.

@mheppler mheppler removed their assignment Jun 28, 2018

@kcondon kcondon self-assigned this Jun 28, 2018

@mheppler mheppler changed the title from UI - Primefaces 6.2 Upgrade to UI - PrimeFaces 6.2 Upgrade Jul 3, 2018

pdurbin added a commit that referenced this issue Jul 11, 2018

@kcondon kcondon closed this Jul 16, 2018

@kcondon kcondon removed the Status: QA label Jul 16, 2018

@djbrooke djbrooke added this to the 4.9.2 - Stata Upgrades, etc. milestone Jul 16, 2018

@tandraschko

This comment has been minimized.

tandraschko commented Jul 23, 2018

@mheppler I accidentally stumbled across this topic. If you have problems with p:autoUpdate, which worked before with autoUpdate=true, please create a issue on our PrimeFaces side and provide a forked primefaces-test project, which shows the bug.
We already have removed the autoUpdate attributes for the upcoming 6.3.

@pdurbin

This comment has been minimized.

Member

pdurbin commented Jul 24, 2018

@tandraschko hi, @mheppler is on vacation. Thanks for reaching out. We're aware that <p:fragment autoUpdate="true"> is deprecated and we're trying to get rid of it but e4a6eed is an example of a recent commit where we re-introduced it due to <p:autoUpdate> not working. I don't know the details but @sekmiller was telling me about it.

@melloware

This comment has been minimized.

melloware commented Jul 24, 2018

It looks like you had the p:autoUpdate in the wrong spot. It needs to go inside the p:fragment like...

<p:fragment>
    <p:autoUpdate/>
</p:fragement>
@pdurbin

This comment has been minimized.

Member

pdurbin commented Jul 24, 2018

@melloware thanks, over at https://gitter.im/primefaces/primefaces?at=5b571f6ee06d7e74099c0909 @tandraschko also suggested that we could add a jsf:id to the div. I let @sekmiller know.

@matthew-a-dunlap

This comment has been minimized.

Contributor

matthew-a-dunlap commented Jul 24, 2018

Thanks for the reply @melloware, this is the actual commit where we reverted the tag 98b2431

@melloware

This comment has been minimized.

melloware commented Jul 24, 2018

OK that is strange then that you were having a side effect because from looking at that commit they should be equivalent. You can try the JSF:ID to the DIV but I feel like there is some other underlying issue. If you definitely think it is a defect create a simple reproducible example using our Test project... https://github.com/primefaces/primefaces-test

Its a stripped down project that you can do "mvn clean jetty:run" and hit it at localhost:8080 to create the minimum reproducible example.

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