fix: Prevent Flashbar from delaying render when not using animations #1656

jperals · 2023-10-16T10:48:46Z

Description

This fixes two issues:

Ticket AWSUI-22525: The collapsible flashbar was making use of TransitionGroup even when motion is disabled, although without defining any animation in that case, so there would not be any perceivable issue for users, but since TransitionGroup delays the removal of items by one frame, this would break tests that remove items. This is fixed by ~~adopting the same approach as the non-collapsible flashbar, with separate list rendering for motion enabled and motion disabled, where TransitionGroup is only used when motion is enabled.~~ replacing the wrapping TransitionGroup with a React fragment when motion is disabled.
An additional issue was found and fixed by working on this: the logic for scrolling the expand/collapse button when collapsing the flashbar was only called when motion is enabled.

How has this been tested?

Added unit tests to cover flash dismissal for collapsible and non-collapsible flashbar (with motion disabled)
Reorganized unit tests so that as much as possible of the collapsible flashbar functionality is tested with motion enabled and disabled (this uncovered the second issue mentioned above)
Manually tested on local dev pages, with motion enabled and disabled, and compared with mainline deployment

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2023-10-16T11:05:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7fa1db8) 94.19% compared to head (44c41ba) 94.20%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1656   +/-   ##
=======================================
  Coverage   94.19%   94.20%           
=======================================
  Files         650      650           
  Lines       17530    17539    +9     
  Branches     5771     5776    +5     
=======================================
+ Hits        16513    16522    +9     
  Misses        948      948           
  Partials       69       69

Files	Coverage Δ
src/flashbar/collapsible-flashbar.tsx	`98.05% <100.00%> (+0.02%)`	⬆️
src/flashbar/non-collapsible-flashbar.tsx	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/flashbar/__tests__/flashbar.test.tsx

jperals · 2023-10-17T07:58:42Z

src/flashbar/collapsible-flashbar.tsx

@@ -93,6 +93,10 @@ export default function CollapsibleFlashbar({ items, ...restProps }: FlashbarPro
        focusFlashById(ref.current, mostRecentItem.id);
      }
    }
+    // When collapsing, scroll up if necessary to avoid losing track of the focused button
+    else if (wasFlashbarStackExpanded && !isFlashbarStackExpanded && notificationBarRef.current) {
+      scrollElementIntoView(notificationBarRef.current);


By moving this here, this will be called regardless of motion being enabled or not. Before, it was under a condition if (initialAnimationState), and initialAnimationState is only initialized if motion is enabled.

Let's not use dnd-kit code in unrelated use-cases. We have our own version of it by the way:

components/src/internal/hooks/use-previous/index.ts

Line 8 in 2a073a2

export const usePrevious = <T>(value: T) => {

I think, this code does not need this value at all, because the hook already depends on isFlashbarStackExpanded, so if isFlashbarStackExpanded === false it automatically means it was true before. If you want to skip initial render, there is useEffectOnUpdate which is more idiomatic approach to solve this:

components/src/internal/hooks/use-effect-on-update.ts

Line 6 in 2a073a2

export function useEffectOnUpdate(callback: EffectCallback, deps: DependencyList) {

Oh, imported from the wrong place, thanks for spotting this.

Ideally I'd like to keep using a layout effect instead of a "normal" effect to prevent flickering, i.e, a frame where the flashbar collapsed but the scrolling hasn't happened yet.

Fixed the import

Why not to simplify it like this?

useEffectOnUpdate(() => { if(!isFlashbarStackExpanded && notificationBarRef.current) { scrollElementIntoView(notificationBarRef.current); } }, [isFlashbarStackExpanded])

I see no need to track previous value

Because useEffectOnUpdate uses useEffect and not useLayoutEffect, the scroll is delayed to the next render. See test failure: https://github.com/cloudscape-design/components/actions/runs/6576775598/job/17866925600

findNotificationBar().click() uses vanilla HTMLElement.click function instead of operating via our test utils.

Use our test utils and I expect it will work fine

just-boris · 2023-10-17T13:09:00Z

src/flashbar/__tests__/collapsible.test.tsx

+  for (const withAnimations of [false, true]) {
+    describe(withAnimations ? 'with animations' : 'without animations', () => {


Optional: Could save one indentation level by using describe.each([true, false])('withAnimations=%s')

just-boris · 2023-10-17T13:10:14Z

src/flashbar/__tests__/collapsible.test.tsx

+    // Test this feature only without animations because TransitionGroup delays item removals by one frame.
+    // Customers should disable animations in their tests too:
+    // https://cloudscape.design/foundation/visual-foundation/motion/#implementation
+    disableMotion(true);


If you manipulate with motion inside a single test, should the beforeAll hook convert to beforeEach to ensure the clean state between tests?

Do you have any response on this comment?

Sorry I missed this comment. This test is outside the describe block that contains the beforeAll hook, so this call should not affect the tests in that block.

This code contains a hidden danger, why not to resolve it by converting beforeAll to beforeEach

just-boris · 2023-10-17T13:13:07Z

src/flashbar/__tests__/common.tsx

+    );
+  };
+  const appWrapper = createWrapper(render(<App />).container);
+  expect(appWrapper.findFlashbar()?.findItems()).toHaveLength(0);


Using ? makes tests less solid. All test code should use !

src/flashbar/__tests__/flashbar.test.tsx

just-boris · 2023-10-17T13:25:46Z

src/flashbar/collapsible-flashbar.tsx

@@ -93,6 +93,10 @@ export default function CollapsibleFlashbar({ items, ...restProps }: FlashbarPro
        focusFlashById(ref.current, mostRecentItem.id);
      }
    }
+    // When collapsing, scroll up if necessary to avoid losing track of the focused button
+    else if (wasFlashbarStackExpanded && !isFlashbarStackExpanded && notificationBarRef.current) {
+      scrollElementIntoView(notificationBarRef.current);


Let's not use dnd-kit code in unrelated use-cases. We have our own version of it by the way:

components/src/internal/hooks/use-previous/index.ts

Line 8 in 2a073a2

export const usePrevious = <T>(value: T) => {

I think, this code does not need this value at all, because the hook already depends on isFlashbarStackExpanded, so if isFlashbarStackExpanded === false it automatically means it was true before. If you want to skip initial render, there is useEffectOnUpdate which is more idiomatic approach to solve this:

components/src/internal/hooks/use-effect-on-update.ts

Line 6 in 2a073a2

export function useEffectOnUpdate(callback: EffectCallback, deps: DependencyList) {

just-boris · 2023-10-17T13:27:23Z

src/flashbar/collapsible-flashbar.tsx

-      }
-    >
-      <TransitionGroup component={null}>
+  const renderListWithoutAnimations = () => {


This is too much code copy paste isn't it? It can get our of sync very easily

Any options how reduce this duplication?

The tests, which are now run with motion enabled and disabled, should prevent breakage of any of the two, but yes, there is some stuff that can be done, for example at the very least reusing some static attributes like the ARIA ones.

Grouped the common props together

Even if the props are reused, the HTML markup is different. Reminder that in our screenshot tests we use no-motion mode, so the animated markup does not go through screenshot testing. This is a big risk for our coverage.

Would you like to make one more pass, making sure the actual HTML is the same (except transition wrapper) or do you want me to look into it and come with the code?

I added a test which compares the HTML output. Note:

For this I needed the test to remove some a11y-related attributes that are not relevant to the visual result, because they have dynamic values

Only for the collapsed state. For the expanded state, it gets trickier since we do need to add classes for the motion case, for the animations to work

If we want to be even more precise here, my suggestion would be to consider actually making screenshot tests with motion enabled too for this component.

jperals · 2023-10-19T07:46:41Z

pages/flashbar/interactive.page.tsx

@@ -13,7 +13,8 @@ export default function InteractiveFlashbar() {
  };

  const add = (type: FlashbarProps.Type, hasHeader = false) => {
-    setItems(items => [generateItem({ type, id: nextId.current.toString(), dismiss, hasHeader }), ...items]);


This makes sure that the new item has the correct id and not the next one. Since React batches state updates, the old logic could lead to two items having the same id.

Or you could just embed this logic inside generateItem, so it always produces items with unique ids

This reverts commit de6a191.

This reverts commit d009b6c.

jperals · 2023-10-20T15:40:38Z

src/flashbar/__tests__/collapsible.test.tsx

+    testFlashDismissal({ stackItems: true });
+  });
+
+  test('produces the same visual HMTL output with motion enabled and disabled', () => {


This test is less relevant after last commit, but it helped me catch that if using <TransitionGroup> without component={null}, it adds a wrapping div. This could actually break styles, eventually.

When using with removeInvisibleAttributes the test is not very trust worthy.

Feel free to keep it, but I do not find it very useful

jperals · 2023-10-20T15:46:44Z

src/flashbar/collapsible-flashbar.tsx

@@ -369,3 +368,6 @@ const NotificationTypeCount = ({
    </span>
  );
 };
+
+const ListWrapper = ({ children, withMotion }: { children: ReactNode; withMotion: boolean }) =>
+  withMotion ? <TransitionGroup component={null}>{children}</TransitionGroup> : <>{children}</>;


Added this small component because TransitionGroup adds an extra div by default unless component is null, but using this same prop in React.Fragment triggers runtime (not build time) errors.

An alternative would be to have this inline inside the main component:

const ListWrapper = isReducedMotion ? TransitionGroup : React.Fragment; (...) <ListWrapper {...isReducedMotion && { component: null } }> (...) </ListWrapper>

Equally valid, just a bit uglier IMO.

Not just uglier but less type safe too, actually.

just-boris

describe.only is important to fix, I will approve after this

just-boris · 2023-10-23T09:58:22Z

src/flashbar/__tests__/collapsible.test.tsx

+    testFlashDismissal({ stackItems: true });
+  });
+
+  test('produces the same visual HMTL output with motion enabled and disabled', () => {


When using with removeInvisibleAttributes the test is not very trust worthy.

Feel free to keep it, but I do not find it very useful

just-boris · 2023-10-23T09:58:57Z

src/flashbar/__tests__/flashbar.test.tsx

-      beforeEach(() => {
-        useAnimations = withAnimations;
-      });
+describe.only('Flashbar component', () => {


Suggested change

describe.only('Flashbar component', () => {

describe('Flashbar component', () => {

Thanks for spotting, removed

just-boris · 2023-10-23T09:59:25Z

src/flashbar/collapsible-flashbar.tsx

@@ -369,3 +368,6 @@ const NotificationTypeCount = ({
    </span>
  );
 };
+
+const ListWrapper = ({ children, withMotion }: { children: ReactNode; withMotion: boolean }) =>
+  withMotion ? <TransitionGroup component={null}>{children}</TransitionGroup> : <>{children}</>;


…motion" This reverts commit 55b65a7.

jperals · 2023-10-23T11:10:55Z

When using with removeInvisibleAttributes the test is not very trust worthy.
Feel free to keep it, but I do not find it very useful

Removed it

jperals force-pushed the fix/flashbar-delayed-render branch from ad5c878 to df0b4f6 Compare October 16, 2023 13:12

jperals deployed to preview October 16, 2023 13:34 — with GitHub Actions View deployment

jperals force-pushed the fix/flashbar-delayed-render branch 3 times, most recently from 2d4df9c to c0a9765 Compare October 16, 2023 16:35

jperals deployed to preview October 16, 2023 16:56 — with GitHub Actions View deployment

jperals added 3 commits October 17, 2023 08:48

fix: Prevent Flashbar from delaying render when not using animations

8f206b3

Remove unnecessary statement in test

699d6f0

Increase coverage for collapsible flashbar with animations

c85eb92

jperals force-pushed the fix/flashbar-delayed-render branch from 8b64eb7 to c85eb92 Compare October 17, 2023 07:17

jperals deployed to preview October 17, 2023 07:32 — with GitHub Actions View deployment

Use disableMotion to disable motion in tests instead of mocking

1e090d4

jperals deployed to preview October 17, 2023 07:49 — with GitHub Actions View deployment

jperals commented Oct 17, 2023

View reviewed changes

src/flashbar/__tests__/flashbar.test.tsx Show resolved Hide resolved

jperals commented Oct 17, 2023

View reviewed changes

jperals marked this pull request as ready for review October 17, 2023 08:00

jperals requested a review from a team as a code owner October 17, 2023 08:00

jperals requested review from just-boris and removed request for a team October 17, 2023 08:00

just-boris reviewed Oct 17, 2023

View reviewed changes

jperals added 2 commits October 17, 2023 17:32

Fix import

dca93ac

Remove some code duplication

8b2b52e

jperals deployed to preview October 18, 2023 08:56 — with GitHub Actions View deployment

Fix async state update issues in interactive flashbar page

6af9665

jperals commented Oct 19, 2023

View reviewed changes

jperals added 2 commits October 19, 2023 09:48

Replace ? with ! in test

315370a

Merge branch 'main' into fix/flashbar-delayed-render

a46df27

jperals requested a review from just-boris October 19, 2023 07:52

jperals deployed to preview October 19, 2023 08:05 — with GitHub Actions View deployment

jperals added 7 commits October 19, 2023 16:51

Replace usePrevious with useEffectOnUpdate

de6a191

Refactor test suite to sue describe.each

06f9be3

Replace beforeAll and afterAll with beforeEach and afterEach

b30f6b9

Revert "Replace usePrevious with useEffectOnUpdate"

d009b6c

This reverts commit de6a191.

Do not animate flash items in collapsible flashbar in Classic

acbdb5c

Compare HTML output of collapsible flashbar with and without motion

55b65a7

Fix non-collapsible flashbar test not covering version with motion

96acd62

jperals deployed to preview October 20, 2023 10:20 — with GitHub Actions View deployment

jperals added 2 commits October 20, 2023 12:21

Revert "Revert "Replace usePrevious with useEffectOnUpdate""

d959459

This reverts commit d009b6c.

Use findToggleButton to find button in scroll test

4238618

jperals deployed to preview October 20, 2023 10:47 — with GitHub Actions View deployment

jperals added 2 commits October 20, 2023 16:44

Use custom list wrapper instead of duplicating code

240e352

Add missing attributes back

a7bb8f7

jperals deployed to preview October 20, 2023 15:11 — with GitHub Actions View deployment

jperals commented Oct 20, 2023

View reviewed changes

just-boris reviewed Oct 23, 2023

View reviewed changes

jperals added 2 commits October 23, 2023 13:07

Remove leftover .only

ba5c348

Revert "Compare HTML output of collapsible flashbar with and without …

44c41ba

…motion" This reverts commit 55b65a7.

jperals deployed to preview October 23, 2023 11:24 — with GitHub Actions View deployment

just-boris approved these changes Oct 23, 2023

View reviewed changes

jperals merged commit a86eadc into main Oct 23, 2023
31 checks passed

jperals deleted the fix/flashbar-delayed-render branch October 23, 2023 11:44

jperals mentioned this pull request Oct 25, 2023

fix: Fix last item of collapsible flashbar not being dismissible #1688

Merged

		for (const withAnimations of [false, true]) {
		describe(withAnimations ? 'with animations' : 'without animations', () => {

	describe.only('Flashbar component', () => {
	describe('Flashbar component', () => {

fix: Prevent Flashbar from delaying render when not using animations #1656

fix: Prevent Flashbar from delaying render when not using animations #1656

Conversation

jperals commented Oct 16, 2023 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

just-boris Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

just-boris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jperals commented Oct 23, 2023

jperals commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

just-boris Oct 17, 2023 •

edited

Loading