fix: iOS popup close button behaviour #2288
Conversation
There was a problem hiding this comment.
Nice you were able to figure out a workaround, although still a bit strange as I thought most of these type of hacks usually effected animations more than static content, but guessing it has some knock-on here for the root issue too.
Given that these hacks are currently being applied to all devices/browsers it might be a bit cleaner to create a separate overrides.ios.scss file that keeps all of these in one clear file instead of spread out across various components.
This would likely require populating a data-attribute to the document body when the main component loads to reference the platform <body data-platform='ios'> (reading the platform from capacitor).
That way the override file can scope accordingly, e.g.
body[data-platform='ios']{
ion-button[data-variant~="card"]{
@include mixins.hack-webkit-reset;
}
}I'd also probably recommend renaming the mixing to be more illustrative of what it does e.g. force-gpu-accelearation
|
Also interestingly, I'm now running the webkit browser using playwright and can replicate a bunch of the ion-toggle issues but I don't see the browser popup close (so that might be mobile-browser specific? - did you ever clarify which issues were on safari desktop as well as mobile app?)... I'm also noticing a bunch of these issues relate specifically to |
|
Looking more closely at specific |
|
@jfmcquade |
This prompted me to check again: I can't replicate #2274 on desktop Safari, but the #2287 I can, which should possibly allow for easier debugging.
Indeed, there's an issue, #2212, which it may be time to prioritise.
Brilliant, thanks, I've approved #2289. I'll take another look at #2274 with this I'll see again if I can address the issues with the other two components directly with that in mind, otherwise I will implement the refactoring of this PR that you suggest above and will suggest that we go ahead with this as a workaround. |
Yeah the box-shadow issues seem very weird generally, although maybe it's the background color getting 'lost' somehow (and hardcoding might fix?). I can't replicate either of the outstanding issues on webkit browser so possibly is more a mobile browser issue, which may be fixed through a combination of tidying up ambiguous style rules and enabling hardware acceleration. I think it would still be preferable if we can fix via regular CSS as it does seem that GPU acceleration may have trade-offs: But I expect for scroll-specific bugs it may well be the only way to fix until we better-optimize components and rendering logic more generally |
jfmcquade
changed the title
| @@ -103,6 +109,7 @@ export class AppComponent { | |||
|
|
|||
| private async initializeApp() { | |||
| this.platform.ready().then(async () => { | |||
| this.platforms = this.platform.platforms().join(" "); | |||
There was a problem hiding this comment.
Originally I planned to use Capacitor.getPlatform(), which just returns "web", "ios" or "android". But there are two potential reasons to prefer Platform.platforms():
- In the future we may want to to extend our platform specific CSS to apply rules based on platform types within this hierachy (e.g. "mobile")
- When running on a web browser with device mode active,
Capacitor.getPlatform()returns "web", butplatform.platforms()returns a list representing the emulated device. E.g. in Chrome with device mode active and set to iPhone SE:
This seems desirable as it allows us to test functionality when running on web, although there may be issues with this that would make the simple platform more suitable
There was a problem hiding this comment.
Makes a lot of sense, also appreciate storing it as string instead of array to use with the css selectors and providing the option for various levels of platform-specific override. I've just added an extra line to the comment as a reminder of this with 6b49c25
|
@chrismclarke I've requested a re-review as I've refactored according to your suggestions, and updated the PR description to explain that this workaround fix is now applied in just one case, but could be extended to cover other similar cases as they occur. |
There was a problem hiding this comment.
Thanks @jfmcquade - looks really good to me, appreciate being able to just apply the styles for a specific platform subset and the separation of core from override scss.
I haven't tested anything on ios, but happy to merge here and just review any further follow-ups for specific issues as they come up
PR Checklist
Description
Resolves a bug where the popup close button would appear behind the popup window in the case that the popup contained scrollable content.
Testing
Appetize build available here.
Dev notes
A bug on iOS causes some CSS rules to not be applied to some elements under certain conditions, manifesting in the bugs detailed in the linked issues. This PR provides a workaround in the form of a CSS mixin that can be applied to any problematic element, within a new
overrides.ios.scssfile. Originally, this approach was used to provide a workaround for issues #2262 and #2287. These have both now been closed by standalone PRs which address their specific issues more directly: #2289 and #2302 respectively.Git Issues
Closes #2274
Screenshots/Videos
The close button on a scrollable popup rendering correctly, resolving #2274 (example_pop_ups template)
