search

IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

fix: iOS popup close button behaviour #2288

Closed jfmcquade closed 1 month ago

jfmcquade commented 2 months ago

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.scss file. 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)

Pop-up
chrismclarke commented 2 months ago

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 ion-toggle but we seem to be using a legacy version of the component (various deprecated upgrade warnings) - it might be worth trying to update these to see if it fixes anything without the added workarounds

chrismclarke commented 2 months ago

Looking more closely at specific comp_button - it still feels to me more an issue with how the button is coded. We're using absolute positioning to nest child rows however not explicitly stating what the absolute is relative too (should be parent container), so that leaves a bit of ambiguity to be filled in.

chrismclarke commented 2 months ago

@jfmcquade See https://github.com/IDEMSInternational/parenting-app-ui/pull/2289 I think it might be a more concrete solution (although will need more work to address other components that have similar problems).

jfmcquade commented 2 months ago

did you ever clarify which issues were on safari desktop as well as mobile app?

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.

I'm also noticing a bunch of these issues relate specifically to ion-toggle but we seem to be using a legacy version of the component (various deprecated upgrade warnings) - it might be worth trying to update these to see if it fixes anything without the added workarounds

Indeed, there's an issue, #2212, which it may be time to prioritise.

Looking more closely at specific comp_button - it still feels to me more an issue with how the button is coded. We're using absolute positioning to nest child rows however not explicitly stating what the absolute is relative too (should be parent container), so that leaves a bit of ambiguity to be filled in.

Brilliant, thanks, I've approved #2289. I'll take another look at #2274 with this position issue in mind (I did try to exhaustively set positions on various elements to resolve it, but I think worth revisiting now I understand the problem from your solution). As for #2287, I don't think the it's the same issue in terms of the position property, but I am reassured by your account of an ambiguity in the CSS that is interpreted differently across various browsers, which at least provides a possible high-level explanation.

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.

chrismclarke commented 2 months ago

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: https://mercimichel.medium.com/don-t-abuse-enabling-gpu-acceleration-on-ios-95520da2428

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 commented 1 month ago

@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.