Prevent Select listbox to grow out of viewport #1746

acelaya · 2024-10-21T13:26:50Z

Part of hypothesis/product-backlog#1561

This PR update the logic that calculates styles for the Select's listbox when it is displayed, so that it takes into consideration the body width when calculating its position.

When we detect the listbox would render outside of the viewport, we let it "grow" in the oposite direction instead, to prevent horizontal scrolling.

listbox-in-viewport-2024-10-24_10.46.58.mp4

listbox-in-viewport-right-2024-10-25_11.25.21.mp4

As a side effect, the listbox can now be smaller than its content in some circumstances. For that, we now provide a new prop to decide if the content should be truncated, or wrap multiple lines.

Truncate:

listbox-in-viewport-truncate-2024-10-24_10.40.23.mp4

Wrap:

listbox-in-viewport-wrap-2024-10-24_10.39.49.mp4

Additionally, when the options have a more complex content, consumers can decide how to deal with content overflow.

TODO

Add tests.
Make sure right listboxes render properly.
Dynamically add title to items that get cropped.
Consider using available space on the opposite side when possible.
Document listboxOverflow prop. -> Document the Select component listboxOverflow #1754
Document listbox behavior when content is big

EDIT The missing points there ☝🏼 will be addressed separately to easy reviews.

acelaya · 2024-10-21T13:28:43Z

src/components/input/Select.tsx

-          // Make items stretch so that all have the same height. This is
-          // important for multi-selects, where the checkbox actionable surface
-          // should span to the very edges of the option containing it.
-          'flex justify-between items-stretch',


Truncating text did not properly work when nesting flex containers, so I simplified this to be a single flex container again, and making the checkbox for multi-selects be self-stretch.

This not only solves the problem, but also simplifies the overall layout making it more clear.

codecov · 2024-10-22T10:43:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c8ee777) to head (336956f).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1746   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           67        67           
  Lines         1195      1202    +7     
  Branches       449       452    +3     
=========================================
+ Hits          1195      1202    +7

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

acelaya · 2024-10-25T09:02:51Z

src/components/input/Select.tsx

+    if (!asPopover) {
+      // Set styles for non-popover mode
+      if (shouldListboxDropUp) {
+        return setListboxCSSProps({
+          bottom: '100%',
+          marginBottom: LISTBOX_TOGGLE_GAP,
+        });
+      }
+
+      return setListboxCSSProps({ top: '100%', marginTop: LISTBOX_TOGGLE_GAP });


The logic for popover listboxes is now more complex, so I decided to flip this condition and move the rest of the logic down.

acelaya · 2024-10-25T09:44:50Z

The right prop was originally introduce to let the listbox align with the right side of the toggle button, for when the Select is located close to the right of the viewport.

I'm just now realizing that with the logic introduce here, we could get rid of it entirely, as a listbox would grow to the left "naturally" if it doesn't fit the viewport.

This is a comparison of how the same Select looks like, using the logic implemented here and removing the right prop from it.

acelaya · 2024-10-25T12:21:18Z

Example of how these changes look when used in the LMS dashboard filters:

listbox-in-viewport-dashboard-2024-10-25_14.19.58.mp4

Since the options have complex content, it just needs a small change to truncate assignment names:

diff --git a/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx b/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx
index 0cc31a9e0..d684abb69 100644
--- a/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx
+++ b/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx
@@ -92,7 +92,7 @@ function AssignmentOption({
   return (
     <MultiSelect.Option value={`${assignment.id}`} elementRef={elementRef}>
       <div className="flex flex-col gap-0.5">
-        {assignment.title}
+        <div className="truncate">{assignment.title}</div>
         <div className="text-grey-6 text-xs">
           {formatDateTime(assignment.created)}
         </div>

src/components/input/Select.tsx

robertknight · 2024-10-25T13:04:48Z

src/components/input/Select.tsx

+    // sure it never increases the body size, which could cause horizontal
+    // scrollbars to appear
+    const regularAvailableSpace =
+      (right ? buttonLeft + buttonWidth : bodyWidth - buttonLeft) -


The meaning of the existing right function parameter is as clear as it could be. This ultimately comes from right in BaseSelectProps. That has some documentation but it could be improved. The summary reads "Align the listbox to the right." To the right of what?

I'm not sure if I follow. Are you suggesting to update the comment somewhere?

src/components/input/Select.tsx

src/components/input/test/Select-test.js

src/pattern-library/examples/select-right.tsx

robertknight · 2024-10-25T13:30:04Z

I'm just now realizing that with the logic introduce here, we could get rid of it entirely, as a listbox would grow to the left "naturally" if it doesn't fit the viewport.

I think I would be inclined to keep it. The UI looks tidier if dropdowns on the right edge of the screen have their right edges exactly aligned with the toggle button, when there is space.

src/components/input/Select.tsx

acelaya · 2024-10-25T14:21:17Z

I'm just now realizing that with the logic introduce here, we could get rid of it entirely, as a listbox would grow to the left "naturally" if it doesn't fit the viewport.

I think I would be inclined to keep it. The UI looks tidier if dropdowns on the right edge of the screen have their right edges exactly aligned with the toggle button, when there is space.

Yeah, I tend to agree

robertknight

I had a query about the asymmetry in tests for left and right-aligned list boxes.

src/components/input/Select.tsx

src/components/input/test/Select-test.js

robertknight · 2024-10-28T11:29:57Z

src/components/input/test/Select-test.js

+    });
+
+    [
+      { name: 'long name'.repeat(50), shouldGrow: true },


This string is about 2300px wide on my system (using CanvasRenderingContext2D.measureText). If the viewport happens to be really large in the test browser, it is possible the listbox might not need to grow.

src/components/input/test/Select-test.js

acelaya · 2024-10-28T13:24:53Z

I had a query about the asymmetry in tests for left and right-aligned list boxes.

I have consolidated the tests for left-aligned and right-aligned listboxes in a single test case with a dataset that covers 6 scenarios:

Left-aligned listbox with small content: the listbox has the same size as the button.
Right-aligned listbox with small content: the listbox has the same size as the button.
Left-aligned listbox with slightly bigger content: The listbox matches the toggle button's left side and grows further to the right.
Right-aligned listbox with slightly bigger content: The listbox matches the toggle button's right side and grows further to the left.
Left-aligned listbox with much bigger content: The listbox matches the body's right side (minus the gap) and grows to the left, further than the button's side.
Right-aligned listbox with much bigger content: The listbox matches the body's left side (minus the gap) and grows to the right, further than the button's side.

acelaya commented Oct 21, 2024

View reviewed changes

acelaya force-pushed the listbox-in-viewport branch from 6734f66 to 20afc2f Compare October 22, 2024 10:41

acelaya force-pushed the listbox-in-viewport branch 14 times, most recently from b21f473 to ba1d1dc Compare October 25, 2024 09:01

acelaya commented Oct 25, 2024

View reviewed changes

acelaya force-pushed the listbox-in-viewport branch 5 times, most recently from 2f4acf6 to e4df7e0 Compare October 25, 2024 09:20

robertknight reviewed Oct 25, 2024

View reviewed changes

acelaya commented Oct 25, 2024

View reviewed changes

src/components/input/Select.tsx Outdated Show resolved Hide resolved

acelaya force-pushed the listbox-in-viewport branch 2 times, most recently from 62a1f82 to 51ceff5 Compare October 25, 2024 14:20

acelaya force-pushed the listbox-in-viewport branch 2 times, most recently from 2ce951e to ad252d3 Compare October 28, 2024 10:41

acelaya requested a review from robertknight October 28, 2024 10:50

acelaya marked this pull request as ready for review October 28, 2024 10:52

acelaya force-pushed the listbox-in-viewport branch from ad252d3 to 0d43c7a Compare October 28, 2024 10:57

robertknight reviewed Oct 28, 2024

View reviewed changes

acelaya force-pushed the listbox-in-viewport branch from 0d43c7a to 71e303b Compare October 28, 2024 13:21

acelaya requested a review from robertknight October 28, 2024 13:25

acelaya force-pushed the listbox-in-viewport branch from 71e303b to 85dd597 Compare October 28, 2024 13:25

Prevent Select listbox to grow out of viewport

336956f

acelaya force-pushed the listbox-in-viewport branch from 85dd597 to 336956f Compare October 28, 2024 13:27

robertknight approved these changes Oct 28, 2024

View reviewed changes

acelaya merged commit 90cfe28 into main Oct 28, 2024
4 checks passed

acelaya deleted the listbox-in-viewport branch October 28, 2024 13:37

This was referenced Oct 28, 2024

Document the Select component listboxOverflow #1754

Merged

Make dashboard filters listboxes render always inside the viewport hypothesis/lms#6819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent Select listbox to grow out of viewport #1746

Prevent Select listbox to grow out of viewport #1746

acelaya commented Oct 21, 2024 •

edited

Loading

acelaya Oct 21, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

acelaya Oct 25, 2024

acelaya commented Oct 25, 2024 •

edited

Loading

acelaya commented Oct 25, 2024 •

edited

Loading

robertknight Oct 25, 2024

acelaya Oct 25, 2024

robertknight commented Oct 25, 2024

acelaya commented Oct 25, 2024

robertknight left a comment

robertknight Oct 28, 2024

acelaya commented Oct 28, 2024 •

edited

Loading

Prevent Select listbox to grow out of viewport #1746

Prevent Select listbox to grow out of viewport #1746

Conversation

acelaya commented Oct 21, 2024 • edited Loading

TODO

acelaya Oct 21, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

acelaya Oct 25, 2024

Choose a reason for hiding this comment

acelaya commented Oct 25, 2024 • edited Loading

acelaya commented Oct 25, 2024 • edited Loading

robertknight Oct 25, 2024

Choose a reason for hiding this comment

acelaya Oct 25, 2024

Choose a reason for hiding this comment

robertknight commented Oct 25, 2024

acelaya commented Oct 25, 2024

robertknight left a comment

Choose a reason for hiding this comment

robertknight Oct 28, 2024

Choose a reason for hiding this comment

acelaya commented Oct 28, 2024 • edited Loading

acelaya commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

acelaya commented Oct 25, 2024 •

edited

Loading

acelaya commented Oct 25, 2024 •

edited

Loading

acelaya commented Oct 28, 2024 •

edited

Loading