[iOS] Fix Gemini Floaty Visibility on Keyboard Appearance [chromium/src : main]
Tarek Makkouk (Gerrit)
Tarek Makkouk voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Adam Arcaro
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
Can we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
Can we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
The KeyboardProvider uses ```UIKeyboardDidShowNotification``` and ```UIKeyboardDidHideNotification``` while we use ```UIKeyboardWillShowNotification``` and ```UIKeyboardWillHideNotification```.
This causes fullscreen progress updates to happen before KeyboardProvider receives the DidShow notification, which will cause the floaty to disappear for a sec before reappearing (https://drive.google.com/file/d/16jff6iyBXqOaQc8vMEjGrIJqquqegKXj/view?usp=sharing).
Related details
- Adam Arcaro
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
Tarek MakkoukCan we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
The KeyboardProvider uses ```UIKeyboardDidShowNotification``` and ```UIKeyboardDidHideNotification``` while we use ```UIKeyboardWillShowNotification``` and ```UIKeyboardWillHideNotification```.
This causes fullscreen progress updates to happen before KeyboardProvider receives the DidShow notification, which will cause the floaty to disappear for a sec before reappearing (https://drive.google.com/file/d/16jff6iyBXqOaQc8vMEjGrIJqquqegKXj/view?usp=sharing).
Can we add UIKeyboardWillShowNotification listeners in `KeyboardProvider` https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=101-110 this is more to keep keyboard logic together so that if other features add keyboard listeners, they're aware that it's also changing the floaty behavior if anything.
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
Tarek MakkoukCan we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
Joemer RamosThe KeyboardProvider uses ```UIKeyboardDidShowNotification``` and ```UIKeyboardDidHideNotification``` while we use ```UIKeyboardWillShowNotification``` and ```UIKeyboardWillHideNotification```.
This causes fullscreen progress updates to happen before KeyboardProvider receives the DidShow notification, which will cause the floaty to disappear for a sec before reappearing (https://drive.google.com/file/d/16jff6iyBXqOaQc8vMEjGrIJqquqegKXj/view?usp=sharing).
Can we add UIKeyboardWillShowNotification listeners in `KeyboardProvider` https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=101-110 this is more to keep keyboard logic together so that if other features add keyboard listeners, they're aware that it's also changing the floaty behavior if anything.
- Adam Arcaro
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 2 comments![]( "Open in Gerrit")
BwgBrowserAgent* bwgAgent = BwgBrowserAgent::FromBrowser(_browser.get());Behind the `IsGeminiCopresenceEnabled()` flag, and we should also add a check somewhere to check if the floaty is invoked before updating the fullscreen progress value. There's a class var for that `is_floaty_invoked_`.
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
BwgBrowserAgent* bwgAgent = BwgBrowserAgent::FromBrowser(_browser.get());Behind the `IsGeminiCopresenceEnabled()` flag, and we should also add a check somewhere to check if the floaty is invoked before updating the fullscreen progress value. There's a class var for that `is_floaty_invoked_`.
- Adam Arcaro
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 4 comments![]( "Open in Gerrit")
- (void)keyboardWillShow {Comment above since this is a private method.
bwgAgent->SetKeyboardVisible(true);This is an obj-c file so this should be `YES` (and `NO` below)
- (void)keyboardWillHide {Comment above since this is a private method.
void SetKeyboardVisible(bool visible);nit:`is_visible` or `is_keyboard_visible` for parity with our class var.
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 4 comments![]( "Open in Gerrit")
Comment above since this is a private method.
Done
This is an obj-c file so this should be `YES` (and `NO` below)
Done
Comment above since this is a private method.
Done
nit:`is_visible` or `is_keyboard_visible` for parity with our class var.
- Adam Arcaro
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 1 comment![]( "Open in Gerrit")
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Code-Review | +0 |
1 comment
Thanks Tarek! Looks great!
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
- (void)keyboardWillShow {I'm not sure how I feel about Gemini-specific logic in the key commands provider. It might be better for the BwgBrowserAgent to register its own observers for keyboard notifications
Maybe @rohi...@chromium.org could chime in here, since I do see some feature code in this file
Related details
- Rohit Rao
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
Tarek MakkoukCan we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
Joemer RamosThe KeyboardProvider uses ```UIKeyboardDidShowNotification``` and ```UIKeyboardDidHideNotification``` while we use ```UIKeyboardWillShowNotification``` and ```UIKeyboardWillHideNotification```.
This causes fullscreen progress updates to happen before KeyboardProvider receives the DidShow notification, which will cause the floaty to disappear for a sec before reappearing (https://drive.google.com/file/d/16jff6iyBXqOaQc8vMEjGrIJqquqegKXj/view?usp=sharing).
Tarek MakkoukCan we add UIKeyboardWillShowNotification listeners in `KeyboardProvider` https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=101-110 this is more to keep keyboard logic together so that if other features add keyboard listeners, they're aware that it's also changing the floaty behavior if anything.
Done
Just saw this comment, I don't mean to cause too much back and forth but let's just make sure it's the right approach
Rohit Rao (Gerrit)
Rohit Rao added 1 comment![]( "Open in Gerrit")
- (void)keyboardWillShow {I'm not sure how I feel about Gemini-specific logic in the key commands provider. It might be better for the BwgBrowserAgent to register its own observers for keyboard notifications
Maybe @rohi...@chromium.org could chime in here, since I do see some feature code in this file
Seems better to register separate observers, unless BWG is specifically hooking into the existing code to add gemini-specific keyboard shortcuts. I don't see that here, it looks like this code could live anywhere?)
Related details
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro added 2 comments![]( "Open in Gerrit")
Tarek MakkoukCan we loop into [KeyboardProvider](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657;bpv=1;bpt=1) instead? It takes a browser so we can definitely initialize it in our browser agent. We would just need to add a browser agent call [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=657-664;bpv=1;bpt=1) for show/hide keyboard
Joemer RamosThe KeyboardProvider uses ```UIKeyboardDidShowNotification``` and ```UIKeyboardDidHideNotification``` while we use ```UIKeyboardWillShowNotification``` and ```UIKeyboardWillHideNotification```.
This causes fullscreen progress updates to happen before KeyboardProvider receives the DidShow notification, which will cause the floaty to disappear for a sec before reappearing (https://drive.google.com/file/d/16jff6iyBXqOaQc8vMEjGrIJqquqegKXj/view?usp=sharing).
Tarek MakkoukCan we add UIKeyboardWillShowNotification listeners in `KeyboardProvider` https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/browser_view/ui_bundled/key_commands_provider.mm;l=101-110 this is more to keep keyboard logic together so that if other features add keyboard listeners, they're aware that it's also changing the floaty behavior if anything.
Adam ArcaroDone
Just saw this comment, I don't mean to cause too much back and forth but let's just make sure it's the right approach
Acknowledged
- (void)keyboardWillShow {Rohit RaoI'm not sure how I feel about Gemini-specific logic in the key commands provider. It might be better for the BwgBrowserAgent to register its own observers for keyboard notifications
Maybe @rohi...@chromium.org could chime in here, since I do see some feature code in this file
Seems better to register separate observers, unless BWG is specifically hooking into the existing code to add gemini-specific keyboard shortcuts. I don't see that here, it looks like this code could live anywhere?)
They're new observers, and they do feel Gemini-specific so I think the BwgBrowserAgent could register them
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
- (void)keyboardWillShow {Rohit RaoI'm not sure how I feel about Gemini-specific logic in the key commands provider. It might be better for the BwgBrowserAgent to register its own observers for keyboard notifications
Maybe @rohi...@chromium.org could chime in here, since I do see some feature code in this file
Adam ArcaroSeems better to register separate observers, unless BWG is specifically hooking into the existing code to add gemini-specific keyboard shortcuts. I don't see that here, it looks like this code could live anywhere?)
They're new observers, and they do feel Gemini-specific so I think the BwgBrowserAgent could register them
Ok sounds good, sorry for the hold up Tarek, I think if you revert the CL to your first patchset, we should be good to go
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
Rohit RaoI'm not sure how I feel about Gemini-specific logic in the key commands provider. It might be better for the BwgBrowserAgent to register its own observers for keyboard notifications
Maybe @rohi...@chromium.org could chime in here, since I do see some feature code in this file
Adam ArcaroSeems better to register separate observers, unless BWG is specifically hooking into the existing code to add gemini-specific keyboard shortcuts. I don't see that here, it looks like this code could live anywhere?)
Joemer RamosThey're new observers, and they do feel Gemini-specific so I think the BwgBrowserAgent could register them
Ok sounds good, sorry for the hold up Tarek, I think if you revert the CL to your first patchset, we should be good to go
No worries, thank you for making sure we use the right approach!
Related details
- Adam Arcaro
- Joemer Ramos
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |