[Bug] Rendering odd whitespace at end of page

[andrewazores]

Notice the whitish band of space at the bottom of the image. I notice this when using my workstation with a 1920x1200 display.

It seems to be caused by #858 . If I check out a commit before that, or locally revert that patch, then the same page looks like this (normal):

[tthvo]

@andrewazores I also see this when using PF topology extension. My issue was with the SkipToContent component in AppLayout and removing it fixed the issue. Is this the same?

[andrewazores]

Yes, removing SkipToContent looks like it clears this up for me as well.

[tthvo]

I guess we can remove that component for now (might lose some functionality for keyboard users and accessibility support) and keep the react-table version bump?

[andrewazores]

Needs more investigation, I think. If the react-table version bump isn't bringing us anything on its own then I would prefer to keep the SkipToContent component. But, it's not necessarily clear what exactly was changed in that react-table update. The release notes might be helpful. Otherwise, maybe we should do a bisect of that [6, 39] patch version range and see which particular version introduced the problem. If we can narrow that down we can try to prepare a separate small reproducer for the observed problem. If we can get it to happen again then we file a bug with PF, otherwise there is some other problem in our setup that should be fixable.

[tthvo]

Yehh sure! That make sense! Just that we need that a react-table version that allows ActionsColumn to have a menuAppendTo props. Seeing error for 4.112.6 now that is missing now.

[tthvo]

patternfly/patternfly-react#8521. seems to be pulling in 4.112.31? But still seeing the bug tho.

[tthvo]

I think this is the issue:

This line specifies that SkipToContent applies styles from button, button-primary, and skipToContent.

https://github.com/patternfly/patternfly-react/blob/25b70978ef1d779ff98260036ce83b94bff5c34e/packages/react-core/src/components/SkipToContent/SkipToContent.tsx#L37

But when rendering, notice how the position: absolute from skipToContent style is being override by position: relative from button.

[tthvo]

Adding this line to app.css fixes it:

.pf-c-skip-to-content {
  position: absolute !important;
}

I suspect exported @patternfly/react-core base.css order is incorrect.

[andrewazores]

Awesome investigative work.

Next thing to check is if creating a brand new application, starting from patternfly-react-seed again, with the same dependency versions, and putting these components together in the same way results in the same problem still. And then if the same app.css fix still works. If that all checks out then this is an upstream PF bug we can report.

[tthvo]

I tried creating a complete new application with same deps as cryostat-web: https://github.com/tthvo/sample-pf-react-app but could not reproduce the issue. Same for PF example with SkipToContent.

But I found out that the root problem seems to originates from code-splitting, which also split *.css (removing lazy loads fixes the issue). These css chunks can be applied in any order and unfortunately, it messes up the order. Not PF bug I think but our setup. So, the css fix seems reasonable now.

[andrewazores]

Ouch. Well, at least we don't need to bother the PF team.

But this does mean we could run into similar problems with css ordering due to splitting in the future. In this case the fix is small and seems low risk, but I imagine it could get a lot hairier in the future with other bugs in the same class.

So you're saying removing the splitting or css lazyload from the webpack config also resolves this particular issue? This seems to mean that what we're doing at the moment is trading correctness for performance. Maybe we need to back that out and be a little more conservative and what gets split and lazy loaded.

[tthvo]

Maybe we need to back that out and be a little more conservative and what gets split and lazy loaded.

Probably a problem with css splitting. Maybe we can back out css splitting and just bundle all css in a single chunk?

[andrewazores]

Would it be one giant css file that combines every css from the application and all deps? Or would it be "vendored" css files, roughly one from the application and one from each dep?

[tthvo]

Or would it be "vendored" css files, roughly one from the application and one from each dep?

hmm is this what happens now during build? I am bit unsure what the css orders are supposed to be in PF package so bundling all together cant seem to make sense...

EDIT: Reading a bit more into PF origins of the base.css, this needs to be applied first and if any, overriden by component's specific css.

[tthvo]

Actually, I just found out something even more interesting now that I look closer. Behaviour only happens if we visit route with table. That just means originally the order is correct. I am now suspecting a duplicate css bundle with incorrect order that comes with a lazy bundle...

Been seeing this warning while building with webpack that might explain... Removing split chunks get rid of these warning.

WARNING in chunk 4001 [mini-css-extract-plugin]
Conflicting order. Following module has been added:
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-styles/css/components/Table/table.css
despite it was not able to fulfill desired ordering with these modules:
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-styles/css/components/Check/check.css
   - couldn't fulfill desired order of chunk group(s) , 
   - while fulfilling desired order of chunk group(s) , , 
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/Progress/progress.css
   - couldn't fulfill desired order of chunk group(s) , 
   - while fulfilling desired order of chunk group(s) , , 
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/MultipleFileUpload/multiple-file-upload.css
   - couldn't fulfill desired order of chunk group(s) , 
   - while fulfilling desired order of chunk group(s) , , 
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-styles/css/components/Table/table-tree-view.css
   - couldn't fulfill desired order of chunk group(s) 
   - while fulfilling desired order of chunk group(s) , , , 
 * css ./node_modules/css-loader/dist/cjs.js!./node_modules/@patternfly/react-styles/css/components/Table/table-grid.css
   - couldn't fulfill desired order of chunk group(s) 
   - while fulfilling desired order of chunk group(s) , , , 

[tthvo]

I dont know if we want to granularly fix these issues or find a way to control these css chunks. Maybe we need to completely back out lazy load route modules?

https://stackoverflow.com/questions/74237008/route-based-code-splitting-causes-css-duplicate

[andrewazores]

Nice, this is becoming a pretty clear picture of what's actually happening.

Let's save backing out the lazy loaded route modules for a last resort fix. It would be nice to keep that since it means application first load is more responsive and browser caching should be more effective. Maybe there are other strategies we can use to try to ensure that the css files have the right load order or right priority.

https://stackoverflow.com/questions/60632062/webpack-chunk-styles-and-react-lazy

https://stackoverflow.com/questions/64458711/using-react-lazy-in-cra-with-cssmodules-causes-chunks-to-have-duplicate-css-in-p

[andrewazores]

@tthvo when you have some cycles, could you prepare a PR backing out the chunk splitting change? Need to verify that it fixes this whitespace bug and doesn't add any other new bugs. This whitespace bug is pretty minor if it's the only manifestation of the css load order problem right now so I don't think this is critical.

[tthvo]

Sure just opened it here: #927