-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
UI does not change theme while in Settings when system theme changes #9801
Comments
Since BrowserViewController is in charge of handling system theme changes, and view controllers being presented in full screen delays underlying view controllers from changing their interface style, the UI does not switch. Changing the presentation style to form sheet does allow underlying view controllers to change their trait collections immediately, but this is not ideal. Doing this also fixes #9788. |
Upon further examination, this bug occurs because some views are being presented with .fullScreen instead of .overFullScreen. There's a particular comment that explains this decision: firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/AppMenu.swift Lines 66 to 69 in e2356c7
Do you know if this is still necessary? @lmarceau |
If you open Settings by tapping the "Customize Homescreen" button, it is presented using .overFullScreen and not .fullScreen. If you navigate to the account picker in the current version of Firefox, I can confirm it doesn't crash when uploading an image. |
Interesting! And that was on iOS 13? And what about if you remove the lines: firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/AppMenu.swift Lines 66 to 69 in e2356c7
Does it work as well by opening the settings from the bottom toolbar (and not from the "Customize Homescreen" button) - has something changed? If it crashes in that case still, then we would need to find the answer as to why opening from "Customize Homescreen" button with .overFullScreen works and not from the bottom toolbar 🤔 |
No, the current version of iOS 15.2.1 and the current App Store version of Firefox. But since all devices that run iOS 13 can be updated to iOS 15, I was wondering if this workaround was still necessary. You can guard it so the code only runs on iOS 14 or earlier.
I also tried changing the modal presentation style from .fullScreen to .overFullScreen in the action sheet and it doesn't crash when uploading a photo or file. This is on the iOS 15 Simulator though. |
@junbio Ok well like you say we can maybe enclose this code in a version check with |
I tested the current App Store version of Firefox on iOS 14.8.1 and it doesn't crash when selecting a new profile picture from Settings. Do you think iOS 14.0 is a good cutoff version? |
@junbio Yeah, like in your PR I think it's ok! I'll test later this week. Thanks a lot! |
This bug is fixed as part of #11825. I'll close it here and ask for this to be tested as part of that issue. |
Steps to reproduce
Expected behavior
UI switches to dark mode
Actual behavior
UI remains light
Device & build information
Attachments:
┆Issue is synchronized with this Jira Task
The text was updated successfully, but these errors were encountered: