Fix dropdown menu positions when scrolling #22916

sidp · 2023-01-02T20:55:55Z

This PR fixes #14266, an issue where dropdown positions, primarily in the compose UI, scroll together with the main timeline instead of staying fixed as expected. This is resolved by updating react-overlays from 0.9.3 to 5.2.1 (latest version as of today) and using strategy: 'fixed' in popperConfig.

Here is an outline of the changes I have made.

DropdownMenu component:

Use react-overlays’ built-in arrow positioning feature.
Re-implemented .dropdown-menu__arrow. The element needs to have a width and height be positioned correctly and this was easier than the previous approach with
Moved wrapping div (.dropdown-menu from DropdownMenu to Dropdown).
Wrap button in a span to solve issue with ref.

EmojiPickerDropdown, LanguageDropdown, PrivacyDropdown and SearchPopout components:

Update for new API (render prop as children, set popperConfig, flip and target props.
Wrap menu components in a div where react-overlays’ props can be inserted.

Everywhere:

Re-implemented the animations using css @keyframes animations to simplify the JavaScript code. No need to keep track of the mounted state to make the positioning of overlays work, or duplicate the animation parameters in multiple places in the codebase.

Two thing to consider when reviewing this PR:

The placement (top, left, right, bottom) of the overlay. It was previously stored in the redux-managed state. This is no longer the case, since react-dropdowns flips the direction if needed to fit them in the viewport, and keeps track of this state itself. I don't know if this is still needed for some edge-case but I haven't seen that in my testing.
Because the arrows (.dropdown-menu__arrow) have been changed from being drawn with border-width to an inline SVG path, this will break any third-party CSS that relies on the border-width method. Not sure if this is a problem in practice.

Any comments and feedback is very welcome!

* Use react-overlays built-in arrow positioning feature * Re-implemented `.dropdown-menu__arrow` to have a defined width and height to improve positioning * Moved wrapping div (`.dropdown-menu` from `DropdownMenu` to `Dropdown`) * Wrap button in a span to solve issue with ref * Temporarily remove animations

* Wrap EmojiPickerMenu in a div where react-overlays’ ref is added

ClearlyClaire · 2023-01-03T11:25:50Z

This will take a while to properly review, but it looks good, thanks for your contribution!

ClearlyClaire

I think there's an opportunity to simplify some of the code further.

I'm also running into an issue where the compose dropdowns are not flipped for some reason I cannot understand:

ClearlyClaire · 2023-01-03T12:19:58Z

app/javascript/mastodon/components/dropdown_menu.js

+        <span ref={this.setTargetRef}>
+          {button}
+        </span>
+        <Overlay show={open} offset={[5, 5]} placement={dropdownPlacement} flip target={this.findTarget} popperConfig={{ strategy: 'fixed' }}>


I wonder if it would make sense just removing the placement logic and let react-overlays handle it through the flip prop.

I've removed the placement state from the individual components and also from the redux actions, so react-overlays is now handling all the positioning state.

ClearlyClaire · 2023-01-03T12:38:45Z

app/javascript/mastodon/features/compose/components/privacy_dropdown.js

+        <Overlay show={open} placement={placement} flip target={this.findTarget} container={container} popperConfig={{ strategy: 'fixed' }}>
+          {({ props, placement }) => (
+            <div {...props} style={{ ...props.style, width: 350, maxWidth: '100vw' }}>
+              <div className={`dropdown-animation ${placement}`}>
+                <PrivacyDropdownMenu
+                  items={this.options}
+                  value={value}
+                  onClose={this.handleClose}
+                  onChange={this.handleChange}
+                  placement={placement}
+                />
+              </div>
+            </div>
+          )}


There is another regression here in that the dropdown is displayed below the boost modal:

From my understanding, this is because the dropdown div is now wrapped by two more divs, so while it has a z-index: 9999, the wrapper doesn't.

I resolved this by removing the z-index from .modal-root__modal. z-index is already set to 9999 for .modal-root so this does not appear to make any practical difference other than that the overlays are now working properly in the modals.

And remove the placement state from props of the menu components.

sidp · 2023-01-03T20:40:22Z

@ClearlyClaire Thanks for the review! I've solved the issues you commented on, but discovered an additional one when I got the dropdowns in the compose UI to flip properly. This issue is that the button for the privacy dropdown should take on the color of the adjacent option, and now that the react-overlays is handling the positioning we don't have the state available to apply the correct color to the button anymore.

I'll request another review once I've fixed this.

Copy the placement state into the `PrivacyDropdown` and `LanguageDropdown` components, to apply correct styling to the buttons depending on which placement the Overlay has.

ClearlyClaire · 2023-01-05T09:57:43Z

I'll request another review once I've fixed this.

Is this ready for another round of review?

sidp · 2023-01-05T17:02:43Z

Is this ready for another round of review?

Yes, please go ahead! I've done another round of testing here and I think it's ready for another review. CI fails on lint, but from what I can see it's not on lines touched by this PR.

ClearlyClaire

I have found no issue when testing, and I could only find one minor code issue. It should be fixed, but otherwise you can consider the PR approved.
Thank you for your contribution!

app/javascript/mastodon/features/compose/components/language_dropdown.js

ClearlyClaire · 2023-01-12T11:11:34Z

app/javascript/mastodon/features/compose/components/language_dropdown.js

-          />
+        <Overlay show={open} placement={'bottom'} flip target={this.findTarget} popperConfig={{ strategy: 'fixed', onFirstUpdate: this.handleOverlayEnter }}>
+          {({ props, placement }) => (
+            <div {...props} style={{ ...props.style, width: 280 }}>


Sorry I just noticed that, but is there a way we can avoid such hardcoded values? The ideal width of the components depend on their text content, which depends on language, so it really should not be hardcoded.

sidp added 10 commits January 2, 2023 15:21

Update react-overlays to latest version

fc8aabb

Fix breaking changes in emoji picker

fcb274c

* Wrap EmojiPickerMenu in a div where react-overlays’ ref is added

Fix breaking changes in language dropdown

8be052e

Fix breaking changes in privacy dropdown

0f573ff

Fix breaking changes in search form

9ae4d2a

Add animations back using @keyframes

fe9457d

Fix arrow color in light theme

ce87b3e

Fix linting issue

89d56e3

Remove unused mounted state

8bc4f12

ClearlyClaire self-requested a review January 3, 2023 11:07

ClearlyClaire requested changes Jan 3, 2023

View reviewed changes

ClearlyClaire reviewed Jan 3, 2023

View reviewed changes

sidp added 3 commits January 3, 2023 21:12

Remove placement state from components and redux

f2761ca

And remove the placement state from props of the menu components.

Remove abolution position to fix flip issue

9b6683f

Remove z-index to fix modals and overlay positions

669dbf6

sidp added 2 commits January 4, 2023 15:40

Fix lint issues

8a996d7

Set placement in privacy and language components

0151211

Copy the placement state into the `PrivacyDropdown` and `LanguageDropdown` components, to apply correct styling to the buttons depending on which placement the Overlay has.

sidp requested a review from ClearlyClaire January 5, 2023 16:59

ClearlyClaire approved these changes Jan 6, 2023

View reviewed changes

app/javascript/mastodon/features/compose/components/language_dropdown.js Outdated Show resolved Hide resolved

Move placement state to correct component

e5c6c05

ClearlyClaire approved these changes Jan 9, 2023

View reviewed changes

Gargron merged commit fd33bcb into mastodon:main Jan 11, 2023

ClearlyClaire reviewed Jan 12, 2023

View reviewed changes

ClearlyClaire mentioned this pull request Jan 12, 2023

Remove hardcoded width from dropdown overlays #23062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dropdown menu positions when scrolling #22916

Fix dropdown menu positions when scrolling #22916

sidp commented Jan 2, 2023

ClearlyClaire commented Jan 3, 2023

ClearlyClaire left a comment

ClearlyClaire Jan 3, 2023

sidp Jan 3, 2023

ClearlyClaire Jan 3, 2023

sidp Jan 3, 2023

sidp commented Jan 3, 2023

ClearlyClaire commented Jan 5, 2023

sidp commented Jan 5, 2023

ClearlyClaire left a comment

ClearlyClaire Jan 12, 2023

Fix dropdown menu positions when scrolling #22916

Fix dropdown menu positions when scrolling #22916

Conversation

sidp commented Jan 2, 2023

ClearlyClaire commented Jan 3, 2023

ClearlyClaire left a comment

Choose a reason for hiding this comment

ClearlyClaire Jan 3, 2023

Choose a reason for hiding this comment

sidp Jan 3, 2023

Choose a reason for hiding this comment

ClearlyClaire Jan 3, 2023

Choose a reason for hiding this comment

sidp Jan 3, 2023

Choose a reason for hiding this comment

sidp commented Jan 3, 2023

ClearlyClaire commented Jan 5, 2023

sidp commented Jan 5, 2023

ClearlyClaire left a comment

Choose a reason for hiding this comment

ClearlyClaire Jan 12, 2023

Choose a reason for hiding this comment