fix: AppLayout horizontal scroll #2839

georgylobko · 2024-10-10T10:25:06Z

Description

When 3 drawers are open on a page with the app layout toolbar, resizing the page eventually causes a horizontal scrollbar to appear (see the picture below), even though all drawers are resized to their minimum size. A solution for that is to check for the horizontal scroll on resize and close the drawers following the priority: navigation bar first, then the first opened drawer.

Related links, issue #, if available: n/a

How has this been tested?

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.

This reverts commit 227dd63.

codecov · 2024-10-10T10:33:54Z

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.21%. Comparing base (a0670ee) to head (1f1ca5f).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
src/app-layout/visual-refresh-toolbar/index.tsx	96.87%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2839   +/-   ##
=======================================
  Coverage   96.20%   96.21%           
=======================================
  Files         761      761           
  Lines       21447    21521   +74     
  Branches     6940     6968   +28     
=======================================
+ Hits        20633    20706   +73     
- Misses        806      807    +1     
  Partials        8        8

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

georgylobko · 2024-10-10T14:13:08Z

src/app-layout/__integ__/runtime-drawers.test.ts

@@ -154,7 +154,8 @@ describe('Visual refresh toolbar only', () => {
      })
    );

-    test('first opened drawer should be closed when active drawers can not be shrunk to accommodate it (1400px)', () => {
+    test(


I just realized that this test suite didn't run earlier due to an extra wrap with an arrow function

georgylobko · 2024-10-10T14:14:42Z

src/app-layout/visual-refresh-toolbar/index.tsx

@@ -227,6 +223,14 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
    const navigationFocusControl = useFocusControl(navigationOpen);
    const splitPanelFocusControl = useSplitPanelFocusControl([splitPanelPreferences, splitPanelOpen]);

+    const onNavigationToggle = useCallback(


Moved this handler below because it relies on navigationFocusControl, which is declared earlier

We should use our useStableCallback util.

There is no promise that onNavigationChange is a stable callback, so your code does not really do what you expect.

To an extent, the same applies to other useCallback additions here. Make sure their dependencies are really stable (drawers array, for example is not)

just-boris · 2024-10-10T14:18:12Z

src/app-layout/__integ__/runtime-drawers.test.ts

@@ -77,7 +77,7 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme =>
 });

 describe('Visual refresh toolbar only', () => {
-  function setupTest(testFn: (page: BasePageObject) => Promise<void>) {
+  function setupTest(testFn: (page: BasePageObject, browser: WebdriverIO.Browser) => Promise<void>) {


This typing is not cross-version compatible. You will make Andrei's life upgrading webdriverio version easier, if you use a more portable version

what exactly is not portable? the type WebdriverIO.Browser?

Yes, this type is not even imported here in this file

because this type is globally available: https://webdriver.io/docs/api/globals/

what exactly do you want me to change to make it forward compatible?

Follow this pattern: https://github.com/search?q=repo%3Acloudscape-design%2Fcomponents%20browser%3A%20ConstructorParameters&type=code

georgylobko · 2024-10-10T14:23:16Z

src/app-layout/visual-refresh-toolbar/index.tsx

+        return;
+      }
+      const throttledOnPageResize = throttle(onPageResize, 500);
+      window.addEventListener('resize', throttledOnPageResize);


You might wonder why I didn't use useResizeObserver—the reason is that this approach is easier to mock and test

Sounds like a potential for future issues.

Also, this makes app layout depending on user interactions – open and then resize renders a different UI than opening on a small screen initially. We should not allow such deviations.

What is your suggestion here?

Do something similar to computeLayout function. The decision for drawers visibility should be a pure function too.

…' into fix/app-layout-horizontal-scroll

georgylobko · 2024-10-11T09:51:00Z

src/app-layout/__integ__/runtime-drawers.test.ts

-  function setupTest(testFn: (page: BasePageObject) => Promise<void>) {
+  class PageObject extends BasePageObject {
+    getBrowser() {
+      return this.browser;


the browser property is protected in the BasePageObject class and it needs to be public here. get browser does not work as typescript considers it as overriding and throws TS2611

This is by design, to limit accidental use of the browser field directly and stick to high level methods.

For this task, you could introduce page.hasHorizontalScroll utility here.

If you think browser should not be private, we can simply expose it in browser test tools after discussion with our team

just-boris · 2024-10-11T12:24:27Z

src/app-layout/__integ__/runtime-drawers.test.ts

@@ -154,7 +154,8 @@ describe('Visual refresh toolbar only', () => {
      })
    );

-    test('first opened drawer should be closed when active drawers can not be shrunk to accommodate it (1400px)', () => {
+    test(


just-boris · 2024-10-11T12:34:44Z

src/app-layout/visual-refresh-toolbar/index.tsx

+        return;
+      }
+      const throttledOnPageResize = throttle(onPageResize, 500);
+      window.addEventListener('resize', throttledOnPageResize);


Sounds like a potential for future issues.

Also, this makes app layout depending on user interactions – open and then resize renders a different UI than opening on a small screen initially. We should not allow such deviations.

just-boris · 2024-10-11T12:36:13Z

src/app-layout/__integ__/runtime-drawers.test.ts

+      const hasHorizontalScroll = () =>
+        page.getBrowser().execute(() => document.body.scrollWidth - document.body.clientWidth > 0);


NB, we have another similar check here:

components/src/app-layout/__integ__/mobile.test.ts

Lines 52 to 53 in 50a9c9e

const [documentWidth, windowWidth] = await page.getViewportWidths();

expect(documentWidth).toEqual(windowWidth);

After merging this, we should follow up and unify detection logic. (Discussed it DM, your version is better, because it properly counts for the scrollbar widths)

just-boris · 2024-10-11T12:37:16Z

src/app-layout/__tests__/runtime-drawers-layout.test.tsx

+      expect(globalDrawersWrapper.findDrawerById('global-drawer-2')!.isActive()).toBe(true);
+
+      act(() => {
+        Object.defineProperty(HTMLElement.prototype, 'scrollWidth', {


If we check only the document values, can we only override these values?

Why do I not see a reset of this value after the test?

…ntal-scroll

…existing params

georgylobko · 2024-10-17T15:49:26Z

src/app-layout/visual-refresh-toolbar/index.tsx

+      const totalActiveDrawersMinSize = getActiveDrawersTotalMinSize();
+      const activeNavigationWidth = navigationOpen ? navigationWidth : 0;
+      // collapsed content width is $space-layout-content-horizontal * 2 = 48px
+      const minContentVisibleWidth = 48;


There’s no way to calculate this directly from the DOM tree since $space-layout-content-horizontal is applied in the grid-template to . area, specifically in this line: https://github.com/cloudscape-design/components/blob/main/src/app-layout/visual-refresh-toolbar/skeleton/styles.scss#L56

If you have a better idea, feel free to share it in this thread.

We already have this constant:

components/src/app-layout/visual-refresh-toolbar/compute-layout.ts

Line 32 in cc75463

const contentPadding = 2 * 24; // space-xl

I suggest to reuse it somehow

georgylobko · 2024-10-18T08:39:47Z

src/app-layout/visual-refresh-toolbar/index.tsx

@@ -116,27 +111,15 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
      //   and compare a given number with the new drawer id min size

      // the total size of all global drawers resized to their min size
-      let totalActiveDrawersMinSize = activeGlobalDrawersIds
-        .map(
-          activeDrawerId => combinedDrawers.find(drawer => drawer.id === activeDrawerId)?.defaultSize ?? MIN_DRAWER_SIZE


I realized that this calculation works incorrectly, as it returns min size only if defaultSize is not defined, but it should compare them and return min of them. Fixed, see below.

just-boris

All good overall, only minor comments

just-boris · 2024-10-18T13:25:30Z

src/app-layout/visual-refresh-toolbar/index.tsx

@@ -227,6 +223,14 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa
    const navigationFocusControl = useFocusControl(navigationOpen);
    const splitPanelFocusControl = useSplitPanelFocusControl([splitPanelPreferences, splitPanelOpen]);

+    const onNavigationToggle = useCallback(


We should use our useStableCallback util.

There is no promise that onNavigationChange is a stable callback, so your code does not really do what you expect.

To an extent, the same applies to other useCallback additions here. Make sure their dependencies are really stable (drawers array, for example is not)

just-boris · 2024-10-18T13:28:42Z

src/app-layout/visual-refresh-toolbar/index.tsx

+      const totalActiveDrawersMinSize = getActiveDrawersTotalMinSize();
+      const activeNavigationWidth = navigationOpen ? navigationWidth : 0;
+      // collapsed content width is $space-layout-content-horizontal * 2 = 48px
+      const minContentVisibleWidth = 48;


We already have this constant:

components/src/app-layout/visual-refresh-toolbar/compute-layout.ts

Line 32 in cc75463

const contentPadding = 2 * 24; // space-xl

I suggest to reuse it somehow

just-boris · 2024-10-18T13:30:48Z

src/app-layout/visual-refresh-toolbar/index.tsx

+          return;
+        }
+
+        closeFirstDrawer();


Didn't we want to have an iteration here, "close until hasHorizontalScroll is false"?

No, we don't, because closing a drawer triggers a change in the dependency array, which leads to recalculations until the horizontal scrollbar is no longer present

Not instantly obvious, but okay

georgylobko · 2024-10-18T14:35:13Z

#2839 (comment)

Tried. It leads to unexpected results

georgylobko added 3 commits October 8, 2024 16:38

Revert "fix: Partially controllable tools (#2793)"

eda4449

This reverts commit 227dd63.

Merge remote-tracking branch 'origin/main'

dbef2ea

fix: AppLayout horizontal scroll

2abe2c6

georgylobko deployed to dev-pages October 10, 2024 10:33 — with GitHub Actions View deployment

chore: unit / integration tests

5ed1267

georgylobko commented Oct 10, 2024

View reviewed changes

fix: Remove unnecessary console.log

1e43660

just-boris reviewed Oct 10, 2024

View reviewed changes

georgylobko commented Oct 10, 2024

View reviewed changes

Merge branch 'main' into fix/app-layout-horizontal-scroll

583a017

georgylobko deployed to dev-pages October 10, 2024 15:43 — with GitHub Actions View deployment

georgylobko added 2 commits October 10, 2024 18:06

chore: Integ tests refactoring

3063e45

Merge remote-tracking branch 'origin/fix/app-layout-horizontal-scroll…

5d89c93

…' into fix/app-layout-horizontal-scroll

georgylobko deployed to dev-pages October 10, 2024 16:14 — with GitHub Actions View deployment

chore: Small refactoring

240f7c5

georgylobko deployed to dev-pages October 11, 2024 08:18 — with GitHub Actions View deployment

fix: Browser getter

9268f74

georgylobko deployed to dev-pages October 11, 2024 09:39 — with GitHub Actions View deployment

georgylobko commented Oct 11, 2024

View reviewed changes

georgylobko marked this pull request as ready for review October 11, 2024 09:54

georgylobko requested a review from a team as a code owner October 11, 2024 09:54

georgylobko requested review from abdhalees and just-boris and removed request for a team October 11, 2024 09:54

just-boris reviewed Oct 11, 2024

View reviewed changes

chore: Expose hasHorizontalScroll from pageObject

ba9a6c8

georgylobko deployed to dev-pages October 11, 2024 12:49 — with GitHub Actions View deployment

Merge remote-tracking branch 'origin/main' into fix/app-layout-horizo…

1501c3b

…ntal-scroll

georgylobko deployed to dev-pages October 17, 2024 08:27 — with GitHub Actions View deployment

chore: Calculate horizontal scroll based on placement.inlineSize and …

f3dfaf7

…existing params

georgylobko deployed to dev-pages October 17, 2024 15:47 — with GitHub Actions View deployment

georgylobko commented Oct 17, 2024

View reviewed changes

chore: Additional test suite

6717805

georgylobko commented Oct 18, 2024

View reviewed changes

georgylobko deployed to dev-pages October 18, 2024 08:48 — with GitHub Actions View deployment

georgylobko requested a review from just-boris October 18, 2024 09:02

just-boris reviewed Oct 18, 2024

View reviewed changes

chore: Small refactoring

cb92ed3

georgylobko deployed to dev-pages October 18, 2024 14:54 — with GitHub Actions View deployment

chore: Small refactoring

1f1ca5f

georgylobko deployed to dev-pages October 18, 2024 15:39 — with GitHub Actions View deployment

just-boris approved these changes Oct 18, 2024

View reviewed changes

georgylobko added this pull request to the merge queue Oct 21, 2024

Merged via the queue into main with commit ec38d6e Oct 21, 2024
38 checks passed

georgylobko deleted the fix/app-layout-horizontal-scroll branch October 21, 2024 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: AppLayout horizontal scroll #2839

fix: AppLayout horizontal scroll #2839

georgylobko commented Oct 10, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

georgylobko Oct 10, 2024

just-boris Oct 11, 2024

georgylobko Oct 10, 2024

just-boris Oct 18, 2024

just-boris Oct 10, 2024

georgylobko Oct 10, 2024

just-boris Oct 10, 2024

georgylobko Oct 10, 2024

just-boris Oct 10, 2024

georgylobko Oct 10, 2024

georgylobko Oct 10, 2024

just-boris Oct 11, 2024

georgylobko Oct 11, 2024

just-boris Oct 11, 2024

georgylobko Oct 11, 2024

just-boris Oct 11, 2024

georgylobko Oct 11, 2024

just-boris Oct 11, 2024

just-boris Oct 11, 2024

just-boris Oct 11, 2024

just-boris Oct 11, 2024

georgylobko Oct 17, 2024 •

edited

Loading

just-boris Oct 18, 2024

georgylobko Oct 18, 2024

just-boris left a comment

just-boris Oct 18, 2024

just-boris Oct 18, 2024

just-boris Oct 18, 2024

georgylobko Oct 18, 2024

just-boris Oct 18, 2024

georgylobko commented Oct 18, 2024

		const hasHorizontalScroll = () =>
		page.getBrowser().execute(() => document.body.scrollWidth - document.body.clientWidth > 0);

	const [documentWidth, windowWidth] = await page.getViewportWidths();
	expect(documentWidth).toEqual(windowWidth);

fix: AppLayout horizontal scroll #2839

fix: AppLayout horizontal scroll #2839

Conversation

georgylobko commented Oct 10, 2024

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Oct 10, 2024 • 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

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

georgylobko Oct 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

georgylobko commented Oct 18, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

georgylobko Oct 17, 2024 •

edited

Loading