-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(android): add adjustMarginsForEdgeToEdge configuration option #7885
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Just a note that the
|
ba56bb9
to
f764a38
Compare
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
@dpogue and other interested parties:
Capacitor 8 will for sure deal with that directly, however, setting |
3b03771
to
af59e2a
Compare
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.
Added two minor comments.
But I think setting the default to auto in a minor release is still a breaking change as it will change the original behavior of Capacitor 7.0.0 apps, which while it might not have been the desired behavior for some users, other that might have adjusted the bottom inset by using CSS already.
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
I'm with you there - seems that we're gonna have to fix a group of people no matter what. Either the people that are currently broken, or the people that fixed it with some hacky-ness. I'm of two minds. I feel weird not default fixing a bug because people might have fixed it a weird other way, but I also feel weird breaking people who thought they were fixed. I think we might want to use disable for 7 just because it is a behavior change, but I don't love it. |
Is there anything here that will break if the WebView does start handling safe-area-insets properly? My guess is that because this is using LinearLayout margins outside the WebView it'll be fine, but wanted to ask |
I completely agree with you, that error should not have appeared with capacitor v7, it is an unexpected behavior. In a project I managed to solve it with CSS using a third-party plugin but I did not release it to the public. I'm still waiting for the fix from the ionic team to update a month after the stable release of v7, this error is still present. |
yeah... I'm asking because I'm trying to get this fixed in Android WebView, and there are concerns about breaking apps that are already working around these problems (weighed against apps that are already broken because of these problems). So I want to make sure that workarounds like the changes in this PR won't cause more trouble if the WebView starts behaving properly. |
I would assume that people using their own css variables for safe areas would be doing something like I would also assume that your fix would return 0 for the safe areas when the WebView is avoiding the safe areas as when using |
Yes, please disable it by default on Capacitor 7, as it would be a breaking change now and change that with Capacitor 8. |
android/capacitor/src/main/java/com/getcapacitor/CapacitorWebView.java
Outdated
Show resolved
Hide resolved
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've noticed that force
also adjusts margins on Android 14 and probably other older versions, not sure if that's expected/desired.
It's not clear by the definition since it only talks about "settings" and not about Android versions, but I personally like that it does it as we have had complaints in the past about devices with notches or similar not behaving correctly (it was related to status bar plugin, but good to have this option in the core)
Thanks! Yeah, I though "force" should do what it implies. All the time. |
No description provided.