fix(popover): content inside of popover scrolls correctly #28861
Conversation
There was a problem hiding this comment.
This should have been updated previously, but likely was not updated due to our diff threshold. The new screenshot aligns with screenshots in main: https://github.com/ionic-team/ionic-framework/blob/9262f7da15969706039e9d5f11ef5c7bd73ced0d/core/src/components/select-popover/test/basic/select-popover.e2e.ts-snapshots/select-popover-multiple-diff-ios-ltr-Mobile-Chrome-linux.png
There was a problem hiding this comment.
This should have been updated previously, but likely was not updated due to our diff threshold. The new screenshot aligns with screenshots in main: https://github.com/ionic-team/ionic-framework/blob/9262f7da15969706039e9d5f11ef5c7bd73ced0d/core/src/components/select-popover/test/basic/select-popover.e2e.ts-snapshots/select-popover-multiple-diff-md-ltr-Mobile-Safari-linux.png
There was a problem hiding this comment.
This should have been updated previously, but likely was not updated due to our diff threshold. The new screenshot aligns with screenshots in main: https://github.com/ionic-team/ionic-framework/blob/9262f7da15969706039e9d5f11ef5c7bd73ced0d/core/src/components/select-popover/test/basic/select-popover.e2e.ts-snapshots/select-popover-diff-ios-ltr-Mobile-Chrome-linux.png
There was a problem hiding this comment.
We previously changed the box shadow, but this screenshot was not updated due to our diff threshold.
There was a problem hiding this comment.
This screenshot was actually captured incorrectly originally. There is a footer in the test template, but due to .popover-viewport not applying the ion-content height was so large that the footer was pushed outside the viewport.
I decided not to add another screenshot because this test already captures the effects of the proposed changes and would fail if the changes were reverted in the future.
There was a problem hiding this comment.
Focus is moved to the first item when it's opened, so it looks like this screenshot is correct. They should have been captured sooner, but the diff threshold prevented them from being updated.
liamdebeasi
requested review from
thetaPC and
averyjohnston
and removed request for
a team
|
Adding Amanda for review too because she worked on some popover scrolling issues in #24294 |
Issue number: resolves #28963 --------- <!-- 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. --> In #28861 I fixed an issue that caused the wrong content inside of a popover to be scrollable. This CSS should have been applied, but it broke back when popover was converted to the Shadow DOM. Fixing this issue revealed a misconfiguration with the select-popover that caused the select-popover to no longer be scrollable. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Content inside of `ion-select-popover` can now be scrolled Note that I am considering this a bug fix instead of a regression. While scrolling used to work in select-popover, it only worked by chance. The `.popover-viewport` styles should have always applied to the select-popover, thus requiring the use of `overflow-y: auto` in select-popover. ## 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. --> Dev build: `7.7.1-dev.11706893059.1bef4b38`
Issue number: resolves #29211 --------- <!-- 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. --> In #28861 I fixed a bug that caused `.popover-viewport` to have `overflow: hidden`. In reality, this code should have always applied but due to an incorrect selector it never did. As it turns out in #29211, some developers were relying on the broken behavior to build their applications. In particular, developers were using `ion-popover` without an `ion-content`. The linked change made it so that using popovers without `ion-content` were not scrollable. After talking with @mapsandapps we think it makes sense to officially support this behavior. We support using [modals without `ion-content`](https://ionicframework.com/docs/api/modal#custom-dialogs), and we could not think of a reason to not support the same use case for popover. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - If the `.popover-viewport` element has a child content then `.popover-viewport `will not be scrollable. - If the `.popover-viewport` element does not have a child content then `.popover-viewport` will be scrollable. I implemented this behavior using progressive enhancement via `:has`. The [`:has` pseudo-class](https://caniuse.com/?search=%3Ahas) has cross-browser support. Ionic v7 supports some versions of browsers that do not have `:has` support. As a result, we fall back to the existing behavior in this environment. Developers are able to get this behavior on older browsers by explicitly setting `overflow: auto` on `.popover-viewport`. Fortunately, we will be dropping support for several of the older browsers versions in Ionic v8, so the need to do the manual opt-in should be less frequent as time goes on. ## 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. --> Dev build: `7.8.2-dev.11711383079.118d48a5` Testing: 1. Open https://codepen.io/liamdebeasi/pen/JjVJrZQ?editors=1100 (this has a dev build installed) 2. Click each button to open a popover. 3. Verify that each popover can be scrolled. I could not find a great way to automate this test, but let me know if anyone has ideas!
Issue number: resolves #28455
What is the current behavior?
The main
ion-contentinside of a popover is not scrollable. Instead, the.popover-viewportcontainer is scrolled because certain styles no longer applied once we moved popover to the Shadow DOM. This bug impacted the linked issue because it meant that the infinite scroll inside of the content never fires (since the content never scrolls).What is the new behavior?
.popover-viewportrule now targets slotted content with the.popover-viewportclass. This class is added to the root node that is slotted. This enables us to target the user's content in the light dom.Does this introduce a breaking change?
Other information
Dev build:
7.6.6-dev.11705696534.177666f8