[terra-clinical-result] Fix flowsheet overflow infinite render #881
Conversation
|
Looks good. Thanks, @RayGunY! |
|
@RayGunY Were you able to reproduce the flickering issue and do you have any screenshots/screen recordings validating that the screen flicker is fixed? |
Yes I was able to reproduce the issue by adding examples for numeric overflow to the existing semantic table example - the rows for 'numeric with overflow' and 'numeric without overflow'. Both flickered after adding them to the semantic table and after adding my fixing changes, the screen recordings I have currently show that the flicker doesn't appear. I can record a video of me recreating the issue in the table without my fixing changes if that would be helpful. |
Yes, I think having the "before" screen recording would be good for documenting the fix! |
|
@sycombs here is that screen recording of the recreated issue: Screen.Recording.2023-07-10.at.8.15.42.AM.mov |
|
I'm noticing an issue where when loading the page with the semantic table we are getting a very quick flash of the numeric overflow result (one that's too long and we shouldn't see) before it changes to say "View Result". It's not visible from the main site example page because of how that page reloads, but we can see it on the test example page. I noticed this as I'm getting the WDIO screenshots added finally, we can see in the snapshot for the flowsheet semantic table test that it's showing the overflow number instead of "View Result", it's doing that because of this initial flash issue. Seems like we will need some sort of way to ensure the page has been fully loaded in regards to this section of logic: https://github.com/cerner/terra-clinical/blob/main/packages/terra-clinical-result/src/flowsheet-result-cell/FlowsheetResultCell.jsx#L397-L426 Screen.Recording.2023-07-10.at.2.25.05.PM.mov |
|
In regards to my comment above - I think this flash issue is something existing that hasn't been caught yet. Looking at the non-semantic test examples I'm seeing the flash as well (with and without my changes). It's really hard to see in that example so that's probably why it hasn't been noticed, now that we have a working overflow example in the semantic table it's more noticeable. I had to take a recording of a slowed down screen recording to show it slow enough in the non-semantic example. slowed_overflow_test_refresh.movLooking into the code I think it's because we have this useEffect to check the container width/content width which then sets numericOverflow to true or false accordingly. It does this check after the initial render, and because numericOverflow is set to false to start it is showing the actual result data the first time it renders. From the useEffect React docs: "If your Effect wasn’t caused by an interaction (like a click), React will let the browser paint the updated screen first before running your Effect." So we need to hold off on rendering the page visually until after the checks in the useEffect() have been done. Off the top of my head I'm not 100% sure how we will need to do this as I'm still a bit unfamiliar with React states/hooks. Poking around online I think maybe we need to use something more along the lines of componentDidMount since that doesn't allow the browser to update the screen until it's run through that logic. I'm not sure if this is something we want to fix right now, my biggest issue in relationship to this current PR is that the snapshots being taken are for the initial render which is showing the long number and not "View Result". @sycombs @sdadn do you have any further thoughts about this? |
@RayGunY thank you for the slowed down video, it helped a lot to visualize what's happening! I agree, it does look like the usage of useEffect (or at least the if condition in it) seems to be the cause that triggers the rerender flash. Since the changes already pre-existed, I don't think we have to fix them with this PR and can log a Jira to investigate and fix this separately since it may require rewriting the useEffect logic. For your wdio tests, you can use browser.waitUntil() to add a few seconds delay so that you can take the wdio screenshots with "View Result" visible. |
Since this seems to be a fundamental issue with the useEffect hook that simply wasn't exposed until the recent accessibility changes, do you think this PR is still needed or should we focus on fixing the root cause, i.e. the useEffect? Is this PR just a bandage fix for the flickering issue or are there other improvements for clinical result that should be preserved? |
@sycombs I think this PR is still needed, the changes I have here not only fixes the flickering defect (the really obvious one) but also handles window resizing better than before. I do think the useEffect hook is going to need to be changed as part of improvements to flowsheet, but that should definitely be a separate story. It's not something that is breaking any workflow or overly obvious right now. @sdadn I'm looking into waitUntil() and I don't think it adds a simple delay, it just waits until a condition is met and times out if that doesn't happen. In our case I'm not sure what condition we'd wait for to ensure that the page has properly rendered. My thought was something like a check for if the text of the overflow cell equals 'View Results' since that's what we know the cell text should be. However getting that text seems like it will be complicated with how everything is structured, I'm trying to look into that right now. Also looking to see if there is a true delay we can add instead of waitUntil(). |
@RayGunY oh sorry, I misread and thought you could just pass in the timeout prop. How about browser.pause? |
|
@sdadn I tried using pause and I might just be using it incorrectly but that hasn’t fixed the snapshot issues, still getting the same one I have been. I think for browser.pause the issue is that it’s pausing execution and therefore the browser from doing anything. So while we’re paused, no re-render is happening...I’m pretty sure it's a bit confusing in the docs. I’ve also tried .waitForDisplayed() which Kol suggested, that didn’t work because it waits for the first display rendered which will be the one we don’t want. I’m looking for other potential solutions but might circle back around to waitUntil() if I can figure out a good condition to check to ensure we’ve re-rendered. |
|
I'm having a hard time getting this test to wait properly. Part of the issue with using things like 'await' and .waitUntil() is that they need to run the test asynchronously - that causes issues with how we fundamentally have these tests set up, specifically related to accessibility checks I believe. I'm also not seeing any other examples within any of the terra libraries where something similar has been used, so I'm not sure if there's a workaround but I'm not familiar enough yet to find it easily. I'm getting this error when trying out any implementation that uses async in the test file: So I turned my attention back to the root cause of the issue - the usage of useEffect. Because this is a layout change happening upon a re-render it seems like useLayoutEffect might be what we want. Here's an excerpt of how that works: "React’s useLayoutEffect hook is almost identical to the useEffect hook. A function called effect and an array of dependencies are the first and second arguments, respectively, for the useLayoutEffect hook. After all DOM mutations have been completed by React, useLayoutEffect executes synchronously. If you need to measure the DOM (for example, to determine the scroll position or other styles for an element) and then modify the DOM or cause a synchronous re-render by changing the state, this can be helpful." Sources:
I tried out that change and it appears to fix our issue! I slowed down some recordings of the semantic and non-semantic tests. That's good but I'm still having some issues rendering the proper snapshots - that might be some cleaning up I need to do though so I'm working on that right now. @sdadn @sycombs what do you guys think about changing to useLayoutEffect? It now looks like this: non-semantic_example_using_useLayoutEffect.movsemantic_table_using_useLayoutEffect.mov |
No problem at all! Still waiting on wdio to cooperate locally for me so I can re-generate the proper snapshot, will likely have someone else on the team generate it for me and push to this branch. I'll tag you guys again once that's been pushed. |
|
Commented out the test for the semantic table in the wdio flowsheet-result-cell-spec.js file. I added a small comment there and referenced the jira I just logged related to this issue. Jira found here: https://jira2.cerner.com/browse/UXPLATFORM-9327 |
| * Fixed | ||
| * Fixed flickering issue related to numeric overflow in FlowsheetResultCell. |
There was a problem hiding this comment.
Note: Most of the original intended changes in this PR got merged as part of #877 . This PR adds some of the remaining work such as wdio tests and this CHANGELOG entry.
| @@ -65,9 +65,18 @@ Terra.describeViewports('FlowsheetResultCell', ['medium'], () => { | |||
| Terra.validates.element('entered in error'); | |||
| }); | |||
|
|
|||
| // This test is causing issues when generating the snapshot, commenting out for now so we can cicle back | |||
There was a problem hiding this comment.
UXPLATFORM-9327 was logged to fix these tests in the future.
|
Build failure is due to outdated snapshots in |
Summary
What was changed:
Removed floats from the styling and wrapped the primary and end displays in one container. This allows us to still have both on one line without using floats and it handles window resize better than before.
Added test cases in the existing semantic table test for numeric overflow when there is overflow and when there is not. Also added some additional testing for entered-in-error result and partial error result, just to make sure we've covered everything.
Added a completely new test for paddingstyles in a semantic table since that was not added before.
In general I improved upon the semantic tests by using only flowsheet result cells instead of just the basic terra cell.
Why it was changed:
The FlowsheetResultCell subcomponent is triggering an infinite rerender due to a change in value of the numericOverflow state variable. This is causing cells to "flicker" between the cell value and "View Results".
numericOverflow is updated in the useEffect hook based on the relative widths of the cell container and cell content (if the cell content width is larger, it will overflow).
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-9260
I've done validation in several different ways.
First were some screenreader checks, nothing should have changed but just wanted to poke around to make sure. Also added a recording of the new test cases I added related to numeric overflow.
test_general_screenreader_output.mov
screenreader_test_numeric_overflow.mov
Resizing tests, showing with no zoom and with full zoom.
no_zoom_resize_window_chrome.mov
full_zoom_resize_window_edge.mov
resize_window_no_side_scroll.mov
Also added a test for the paddingstyles so here is a screenshot of that
