fix(menu): main content is not scrollable while swiping - #26976
Merged
Conversation
|
|
Contributor
|
I'm not able to replicate the issue on the current 2023-03-16.13-28-18.mp4 |
Contributor
Author
|
It's easier to reproduce if you try it on an actual phone with touch capabilities rather than with touch simulation: #21193 (comment) I've found it easiest to repro on Chrome for Android if you try to swipe the menu open and then scroll in a circular motion like the video in the linked comment is doing. |
averyrousseau
approved these changes
Mar 16, 2023
averyrousseau
left a comment
averyrousseau
left a comment
Contributor
There was a problem hiding this comment.
Aha, just saw it. It does take a few tries haha. Looks good on the branch though. 👍
|
Will test, thanks @liamdebeasi! |
|
Looks good here on V7. |
3 tasks
2 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jan 23, 2024
…hen menu opens (#28829) Issue number: resolves #28399 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> As part of #26976 I fixed an issue where `pointer-events: none` was not applied until after the menu open gesture finishes. This resolved a bug where scrolling was latching after the menu gesture starts. However, I did not account for the edge case where scrolling latches _before_ `pointer-events: none` is applied in the DOM. Since scrolling has already latched then `pointer-events: none` does not change the scrolling behavior. This can happen if a user swipes up and to the right from the left edge of the screen. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - `overflow-y: hidden` is now applied to the scrollable content which will interrupt any scrolling when the menu is open. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Testing: This bug fixes a timing issue where scrolling latches on the main content as the menu tries to open. As a result, I am unable to write reliable automated tests for this. Reviewers should perform the following test on iOS and Android physical devices: 1. Open `src/components/menu/test/basic`. 2. Add enough elements to the main page content such that it scrolls (I added a list with items). 3. On each device, attempt to scroll the main content while also opening the menu on the starting edge of the screen. Scrolling on the main content should not happen if the menu opens. Dev build: `7.6.5-dev.11705341148.1a550d3b`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
Please check if your PR fulfills the following requirements:
ionic-docsrepo, in a separate PR. See the contributing guide for details.npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue URL: resolves #21193
The
.menu-content-openclass prevents users from scrolling the content behind a menu when it is open:ionic-framework/core/src/css/core.scss
Line 266 in 118b6ba
However, it does not account for when users accidentally scroll the content while trying to drag the menu open because it is only added once the animation is complete. As a result, it is possible to scroll the content underneath the menu and have the menu gesture activate. This also causes the menu gesture performance to degrade significantly.
What is the new behavior?
.menu-content-openclass is added before the open animation starts, preventing any accidental scrolls.Does this introduce a breaking change?
Other information