Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover: smoothly reposition on scroll #18813

Merged
merged 12 commits into from Dec 4, 2019
Merged

Popover: smoothly reposition on scroll #18813

merged 12 commits into from Dec 4, 2019

Conversation

@ellatrix
Copy link
Member

ellatrix commented Nov 28, 2019

Description

Fixes #16643.
Blocks #18779.

This PR refactors the Popover component so that it smoothly repositions on scroll. Currently we do have logic to reposition it for every scroll frame, but the Popover is always rerendered too late (and thus the style attributes reset too late). Why is that? Because currently we set it with React state, which then rerenders the component. Since this is not done synchronously, there's a small delay.

The only way we can fix this is by avoiding setting state. Instead, we should update the DOM immediately. This is also more performant. There's no way around the DOM, unfortunately.

As a result, I think the component has also become simpler. There's no need for the many nested hooks and passing around props. I also swapped out the is-* classes in favour of data- attributes, which are much easier to update.

How has this been tested?

Open some popovers and play with scrolling and resizing. Nothing else should be different from master.

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 28, 2019

Are you sure you couldn't just use useLayoutEffect?

The signature is identical to useEffect, but it fires synchronously after all DOM mutations. Use this to read layout from the DOM and synchronously re-render. Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint.

https://reactjs.org/docs/hooks-reference.html#useref

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 28, 2019

@ZebulanStanphill How would you use that for a scroll callback? It doesn't fire in that case, it only fires when any props change. In other words, there is no re-render happening, and I'm not sure how you trigger a synchronous render.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 28, 2019

@ellatrix Good point. I didn't think that through enough.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 29, 2019

The animation when opening the block More options or the More tools & options popovers is gone.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 29, 2019

@ZebulanStanphill I don’t see the animation in master either.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 29, 2019

@ellatrix I didn't test this branch but in master I do see the animation (make sure you're not in prefers reduced mode and the "disable animation" plugin should be disabled)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 29, 2019

@youknowriad Ah, for some reason I had only that test plugin enabled... Strange :)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 29, 2019

@ZebulanStanphill For me the animation works with this branch as well, if you make sure the "disable animation" plugin is disabled.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 29, 2019

I just checked, and yep, the "Gutenberg Test Plugin, Disables the CSS animations" was enabled. I never enabled it. Is it supposed to be enabled by default when you create a dev environment? (I'm using the @wordpress/env environment.)

Anyway, it looks like there are no issues (that I know of) on this branch after all.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 29, 2019

@ZebulanStanphill I have the same issue. The plugin always gets activated for some reason, I suspect by e2e tests? Thank you for testing the PR!

@ellatrix ellatrix force-pushed the try/smooth-popover-scroll branch from f3f79b4 to 9733838 Nov 29, 2019
Copy link
Contributor

youknowriad left a comment

I'm not sure it's specific to this branch but I had an issue on mobile where I can't scroll the Inspector/Document sidebar. I know we fixed something like that recently, so maybe a rebase?

--
I noticed that the popover doesn't reposition properly on resize (mobile)

Capture d’écran 2019-12-04 à 9 12 41 AM

Copy link
Contributor

youknowriad left a comment

@gziolo mentioned https://popper.js.org/ as a great popover lib. I wonder if we could make our Popover component just a wrapper around and avoid the maintainability costs. (It's a separate topic from this PR though)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

@youknowriad I'd love us to have more use cases in Gutenberg for Popover before we decide to wrap an external component. There's a few more interesting use cases in #18779.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

@youknowriad I cannot scroll the document sidebar on mobile in master either.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

I noticed that the popover doesn't reposition properly on resize (mobile)

This is fixed too.

@ellatrix ellatrix requested a review from youknowriad Dec 4, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 4, 2019

This is working well for me. I was hoping it would fix the issue where sometimes the scrollbar in popovers don't appear (you can trigger it inconsistently in the More menu or the block settings menu) but unfortunately, it doesn't, it still happens sometimes. I was curious whether you have an idea why this happens sometimes.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

I was hoping it would fix the issue where sometimes the scrollbar in popovers don't appear (you can trigger it inconsistently in the More menu or the block settings menu) but unfortunately, it doesn't, it still happens sometimes. I was curious whether you have an idea why this happens sometimes.

I see what you mean. Looks like a bug somewhere else in the code, but I'll have a look.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 4, 2019

Capture d’écran 2019-12-04 à 12 16 28 PM

Mmm, I found another little bug here where scrolling the content area doesn't resize the popover properly (notice that the height of the popover should reach the bottom of the window in the screenshot but t seems stuck.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 4, 2019

so basically, it scales down, but doesn't scale up again.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

@youknowriad I think I fixed both those issues now.

@ellatrix ellatrix requested a review from youknowriad Dec 4, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 4, 2019

This is cool. I think there's a small refresh issue still. If you scroll or resize quickly. You'll notice that sometimes the popover is not adjusting its size, sometimes it stops doing so.

Capture d’écran 2019-12-04 à 1 41 25 PM

Notice the scrollbars in the screenshot. I do that by resizing up and down quickly.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

@youknowriad Thanks for catching that. It should be fixed now. The problem was that I didn't unset the max height if none was provided (and kept the old one instead).

Copy link
Contributor

youknowriad left a comment

LGTM 👍

There are some Github issues that will be fixed with this (the scrollbar issues)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 4, 2019

Many thanks for the review, @youknowriad!

@ellatrix ellatrix merged commit a2d0227 into master Dec 4, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix mentioned this pull request Dec 5, 2019
6 of 6 tasks complete
@ellatrix ellatrix deleted the try/smooth-popover-scroll branch Dec 6, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
* wip

* wip2

* simplify

* fix focus issue

* Adjust snapshots

* Fix CSS

* Adjust more CSS

* Fix is-mobile issue and restore IsolatedEventContainer

* Add useEffect dependencies

* Address feedback

* Fix content size calc

* Unset content width/height if none is provided
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.