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(popover): viewport can be scrolled if no content present #29215
Conversation
core/src/css/core.scss
Outdated
* we should fallback to the old behavior for environments that do not support :has. | ||
* Developers can explicitly enable this behavior by setting overflow: visible | ||
* on .popover-viewport if they know they are not going to use an ion-content. | ||
* TODO FW-XXXX Remove this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the team is on board with this approach, I'll make a tech debt ticket and populate the ticket number here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also good with this approach 👍
As for tests, could we do one of the following:
|
Good idea! I added a similar test to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue number: resolves #29211
What is the current behavior?
In #28861 I fixed a bug that caused
.popover-viewport
to haveoverflow: 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 anion-content
. The linked change made it so that using popovers withoution-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
, and we could not think of a reason to not support the same use case for popover.What is the new behavior?
.popover-viewport
element has a child content then.popover-viewport
will not be scrollable..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 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 settingoverflow: 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?
Other information
Dev build:
7.8.2-dev.11711383079.118d48a5
Testing:
I could not find a great way to automate this test, but let me know if anyone has ideas!