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

Fix unresponsive navigation menu on mobile #32488

Closed
wants to merge 1 commit into from
Closed

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented May 19, 2022

Fix #28613

Snapper open and close function calls were deferred to catch events correctly after dragging to set the animating boolean value correctly so that the snapper would be opened and closed correctly on the next interaction

server/core/src/init.js

Lines 265 to 277 in cf97a8e

const _snapperOpen = () => {
if (animating || snapper.state().state !== 'closed') {
return
}
oldSnapperOpen('left')
}
const _snapperClose = () => {
if (animating || snapper.state().state === 'closed') {
return
}
oldSnapperClose()
}

These functions were deferred as a part of #23785

There does not seem to be an issue with catching the events and setting the animating boolean value correctly so that the snapper opens and closes as expected

Remarks

The animating event will fire every millisecond ad infinitum when tapping to close the menu. The UI still works as expected. This would be fixed by setting the tapToClose option https://github.com/JoeyAndres/Snap.js#settings-and-defaults on snap.js to false but this would introduce another issue which causes swipe to close to fail on mobile, so this issue remains

The issue may be fully addressed as a part of #28063

@Pytal Pytal added bug 3. to review Waiting for reviews labels May 19, 2022
@Pytal Pytal added this to the Nextcloud 25 milestone May 19, 2022
@Pytal Pytal requested review from PVince81 and a team May 19, 2022 01:20
@Pytal Pytal self-assigned this May 19, 2022
@Pytal Pytal requested review from artonge and vanpertsch and removed request for a team May 19, 2022 01:20
@PVince81
Copy link
Member

will fire every millisecond ad infinitum

my previous work was intended to prevent this
I remember that this caused performance issues during calls because of too many events, so I think we should avoid bringing this back if possible

@Pytal
Copy link
Member Author

Pytal commented May 19, 2022

my previous work was intended to prevent this I remember that this caused performance issues during calls because of too many events, so I think we should avoid bringing this back if possible

The situation is that the defer from the previous PR did not resolve this specific issue either and may be reproduced on master when tapping/clicking and not when hitting enter to close the navigation menu with a console.log('animating') in this block

server/core/src/init.js

Lines 240 to 244 in eb720b9

snapper.on('animating', () => {
// we need this because the trigger button
// is also implicitly wired to close by snapper
animating = true
})

From my analysis with the performance profiler in Chrome DevTools there does not seem to be any unexpected performance impact. Only the listeners continue to increase but this is only because garbage collection is disabled for listeners while recording the performance timeline, as per https://stackoverflow.com/a/46745023, this behaviour may be tested by clicking the trash icon to manually trigger garbage collection during recording. Therefore there does not seem to be any runtime performance impact without the defer, any further insights are welcome @PVince81

@Pytal Pytal closed this May 25, 2022
@szaimen szaimen deleted the fix/mobile-nav-menu branch August 29, 2022 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nextcloud 22.0.1 - Mobile menu
2 participants