[HandoffButton] Use Omnibox popup state for split view visibility [chromium/src : main]
Michelle Abreo (Gerrit)
Michelle Abreo added 2 comments![]( "Open in Gerrit")
void ActorUiTabController::OnOmniboxFocusChanged(
OmniboxFocusState state,
OmniboxFocusChangeReason reason) {
bool has_focus = state != OmniboxFocusState::OMNIBOX_FOCUS_NONE;
if (is_focusing_omnibox_ == has_focus) {
return;
}
is_focusing_omnibox_ = has_focus;
if (auto* window_controller =
ActorUiWindowController::From(tab_->GetBrowserWindowInterface())) {
// Notify the window controller about the focus change. This will trigger UI
// updates on all panes.
window_controller->OnOmniboxFocusChanged(this, is_focusing_omnibox_);
} else {
UpdateUi(
base::BindOnce(&LogAndIgnoreCallbackError, "OnOmniboxFocusChanged"));
}
}Michelle Abreothe flow of the updates here feels odd. We make each and every tab controller (we'll have 1 for each tab, so potentially tens if not hundreds) observe on the omnibox update, just to send the update up to the window controller, and have it then update the contentcontainer controller's tab controllers.
Instead, why don't we observe on the contentContainer controller level (since those would be mapped to the visible tabs anyway - which we'll have 2 of at most in case of split view), and update the relevant tab controllers then?
updated to handle observation in the contents container controller. also updated the omnibox signal to be the popup opened vs just the omnibox focus, because this is a more accurate fix for the issue of overlap.
if (window_controller->IsOmniboxFocusedInOtherPane(this)) {Michelle Abreothis becomes IsOmniboxFocused in general. right?
yes, updated with the new check for popup visibility.
Related details
- Abe Boujane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abe Boujane (Gerrit)
Abe Boujane added 5 comments![]( "Open in Gerrit")
#include "components/omnibox/common/omnibox_focus_state.h"is this import needed?
#include "base/timer/timer.h"is this import needed?
// If the WebView's current WebContents is not the one we are observing,
// we have been detached and should stop processing.
if (contents_container_view_->GetWebContents() != web_contents()) {
return;
}
auto* tab_controller = GetActorUiTabController();
if (!tab_controller) {
return;
}
Consider passing `ActorUiWindowController*` directly to the `ActorUiContentsContainerController` constructor. This will allow you to access it directly.
**Current Flow:**
`ActorUiContentsContainerController` -> `WebContents` -> `TabInterface` -> `BrowserWindowInterface` -> `ActorUiWindowController::From(...)`
**Proposed Flow (Direct Access):**
`ActorUiContentsContainerController` (holds `window_controller_` ptr) -> `window_controller_->OnOmniboxPopupVisibilityChanged(...)`
This would leverage the direct ownership relationship.
if (omnibox_focused_tab_controller_ &&
omnibox_focused_tab_controller_.get() == sender.get()) {It's unclear when the `omnibox_focused_tab_controller_` is cleared/changed etc.
Is a better approach to never store the sender at all at the window controller level, and simply keep the bits we get at the contentsContainer level?
You can still have an aggregator function at the WindowController level, its logic could be something like:
```
IsAnyOmniboxPopupOpened(){
for(contentContainerCOntroller){
if(contentCOntainerController->IsOmniboxOpen() return true;
}
return false;
```
for (const auto& container_controller : contents_container_controllers_) {
if (auto* contents = container_controller->web_contents()) {
if (auto* tab = tabs::TabInterface::MaybeGetFromContents(contents)) {
if (auto* tab_controller =
actor::ui::ActorUiTabControllerInterface::From(tab)) {
tab_controller->OnWindowOmniboxPopupVisibilityChanged();
}
}
}
}I think we should just delegate to the conteintsContainerVOntroller here:
```
for(contenteCOntainer: containers) {
container->NotifyWindowOmniboxPopupVisibilityCHanged();
}
```
and then have the contents container controllers implement NotifyWindowOmniboxPopupVisibilityChanged() :
```
ContentsContainerCOntroller::NotifyWindowOmniboxPopupVisibilityChanged(){
if (auto* tab_controller = GetActorUiTabController()) {
tab_controller->OnWindowOmniboxPopupVisibilityChanged();
}
}
```
Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo added 5 comments![]( "Open in Gerrit")
#include "components/omnibox/common/omnibox_focus_state.h"Michelle Abreois this import needed?
this one was leftover from a prev patchset, removed!
#include "base/timer/timer.h"Michelle Abreois this import needed?
It looks like it is necessary. I got a build error (no type named 'RetainingOneShotTimer') after removing omnibox_tab_helper.h.
It seems omnibox_tab_helper.h was the only remaining header transitively providing the timer definition. so I added #include "base/timer/timer.h" to fix the break.
// If the WebView's current WebContents is not the one we are observing,
// we have been detached and should stop processing.
if (contents_container_view_->GetWebContents() != web_contents()) {
return;
}
auto* tab_controller = GetActorUiTabController();
if (!tab_controller) {
return;
}
Consider passing `ActorUiWindowController*` directly to the `ActorUiContentsContainerController` constructor. This will allow you to access it directly.
**Current Flow:**
`ActorUiContentsContainerController` -> `WebContents` -> `TabInterface` -> `BrowserWindowInterface` -> `ActorUiWindowController::From(...)`**Proposed Flow (Direct Access):**
`ActorUiContentsContainerController` (holds `window_controller_` ptr) -> `window_controller_->OnOmniboxPopupVisibilityChanged(...)`This would leverage the direct ownership relationship.
good idea, updated!
if (omnibox_focused_tab_controller_ &&
omnibox_focused_tab_controller_.get() == sender.get()) {It's unclear when the `omnibox_focused_tab_controller_` is cleared/changed etc.
Is a better approach to never store the sender at all at the window controller level, and simply keep the bits we get at the contentsContainer level?You can still have an aggregator function at the WindowController level, its logic could be something like:
```
IsAnyOmniboxPopupOpened(){
for(contentContainerCOntroller){
if(contentCOntainerController->IsOmniboxOpen() return true;
}
return false;
```
Done. I've refactored this to use the aggregation pattern as suggested.
some context on the implementation: To support the popup fade-out animation (preventing the button from snapping back while the popup is still visible), we need the system to report 'Open' for a few hundred milliseconds after the logical close.
Since the window controller no longer stores the state in `omnibox_focused_tab_controller_`, I moved this delay logic into ActorUiContentsContainerController. It now uses a PostDelayedTask and a cancellable closure to maintain the is_popup_open_ state during the fade-out.
for (const auto& container_controller : contents_container_controllers_) {
if (auto* contents = container_controller->web_contents()) {
if (auto* tab = tabs::TabInterface::MaybeGetFromContents(contents)) {
if (auto* tab_controller =
actor::ui::ActorUiTabControllerInterface::From(tab)) {
tab_controller->OnWindowOmniboxPopupVisibilityChanged();
}
}
}
}I think we should just delegate to the conteintsContainerVOntroller here:
```
for(contenteCOntainer: containers) {
container->NotifyWindowOmniboxPopupVisibilityCHanged();
}
```and then have the contents container controllers implement NotifyWindowOmniboxPopupVisibilityChanged() :
```
ContentsContainerCOntroller::NotifyWindowOmniboxPopupVisibilityChanged(){
if (auto* tab_controller = GetActorUiTabController()) {
tab_controller->OnWindowOmniboxPopupVisibilityChanged();
}
}
```
- Abe Boujane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abe Boujane (Gerrit)
Abe Boujane added 5 comments![]( "Open in Gerrit")
bool is_focusing_omnibox_ = false;no longer needed.
class ActorUiTabController : public ActorUiTabControllerInterface,
public ImmersiveModeController::Observer {perhaps we should have this chained after your other ImmersiveMode CL? otherwise you'll have some significat merge conflicts I think.
bool IsOmniboxPopupOpen() const;
void NotifyWindowOmniboxPopupVisibilityChanged();please add documentation for these methoda
close_popup_task_ = std::make_unique<base::CancelableOnceClosure>(
base::BindOnce(&ActorUiContentsContainerController::HandlePopupClosed,
weak_ptr_factory_.GetWeakPtr()));
content::GetUIThreadTaskRunner({})->PostDelayedTask(
FROM_HERE, close_popup_task_->callback(),
base::Milliseconds(kOmniboxPopupHandoffButtonDelay));not sure this is the correct approach. I think if you want to know for sure that the omnibox is closed ( no longer appearing on the screen), then Widget Observer is what we want. it has a method you can override: OnWidgetVisibilityChanged. You can get the OmniboxPopup via BrowserElementsViews::From(bwi)
->GetViewAs<WhateverTheOmniboxPopupViewIs>(kWhateverTheElementIdIs) (or via other ways)
is_popup_open_ = false;Popup terminology is a bit unclear. Can we call it OmniboxPopup?
Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo added 5 comments![]( "Open in Gerrit")
bool is_focusing_omnibox_ = false;Michelle Abreono longer needed.
Done
class ActorUiTabController : public ActorUiTabControllerInterface,
public ImmersiveModeController::Observer {perhaps we should have this chained after your other ImmersiveMode CL? otherwise you'll have some significat merge conflicts I think.
Done
bool IsOmniboxPopupOpen() const;
void NotifyWindowOmniboxPopupVisibilityChanged();please add documentation for these methoda
Done
close_popup_task_ = std::make_unique<base::CancelableOnceClosure>(
base::BindOnce(&ActorUiContentsContainerController::HandlePopupClosed,
weak_ptr_factory_.GetWeakPtr()));
content::GetUIThreadTaskRunner({})->PostDelayedTask(
FROM_HERE, close_popup_task_->callback(),
base::Milliseconds(kOmniboxPopupHandoffButtonDelay));not sure this is the correct approach. I think if you want to know for sure that the omnibox is closed ( no longer appearing on the screen), then Widget Observer is what we want. it has a method you can override: OnWidgetVisibilityChanged. You can get the OmniboxPopup via BrowserElementsViews::From(bwi)
->GetViewAs<WhateverTheOmniboxPopupViewIs>(kWhateverTheElementIdIs) (or via other ways)
I looked into the WidgetObserver idea, but it feels like a bit of overkill for this one edge case.
the main blocker is that there isn't a clean way to get that popup widget. it doesn't have a public element ID, and the OmniboxView interface doesn't expose it, so i’d have to include omnibox_view_views.h and explicitly downcast to the concrete OmniboxViewViews implementation. it feels weird to break that abstraction and couple this controller to the internal omnibox UI code just to catch the end of an animation.
I also checked LocationBarView::OnPopupStateChanged, and it handles this exact asynchronous transition issue by simply posting a task with a 100ms delay. so sticking with a PostDelayedTask (i'm using 150ms to be safe, [the actual fade-out duration is hardcoded to 82ms](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc;l=222-223;drc=c8fb498259ce5c219f957dbe367da21a82a361c0)) seems to follow the established pattern for handling these fade-outs, rather than introducing new dependencies.
since this is purely for visual smoothing in a rare edge case (focusing the actuating tab's omnibox and then immediately clicking the other split pane) I feel like sticking with the timer is a cleaner trade-off.
Does that sound reasonable? happy to add the observer if you feel strongly, but wanted to double check.
Popup terminology is a bit unclear. Can we call it OmniboxPopup?
- Abe Boujane
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. |
Abe Boujane (Gerrit)
Abe Boujane added 1 comment![]( "Open in Gerrit")
close_popup_task_ = std::make_unique<base::CancelableOnceClosure>(
base::BindOnce(&ActorUiContentsContainerController::HandlePopupClosed,
weak_ptr_factory_.GetWeakPtr()));
content::GetUIThreadTaskRunner({})->PostDelayedTask(
FROM_HERE, close_popup_task_->callback(),
base::Milliseconds(kOmniboxPopupHandoffButtonDelay));Michelle Abreonot sure this is the correct approach. I think if you want to know for sure that the omnibox is closed ( no longer appearing on the screen), then Widget Observer is what we want. it has a method you can override: OnWidgetVisibilityChanged. You can get the OmniboxPopup via BrowserElementsViews::From(bwi)
->GetViewAs<WhateverTheOmniboxPopupViewIs>(kWhateverTheElementIdIs) (or via other ways)
I looked into the WidgetObserver idea, but it feels like a bit of overkill for this one edge case.
the main blocker is that there isn't a clean way to get that popup widget. it doesn't have a public element ID, and the OmniboxView interface doesn't expose it, so i’d have to include omnibox_view_views.h and explicitly downcast to the concrete OmniboxViewViews implementation. it feels weird to break that abstraction and couple this controller to the internal omnibox UI code just to catch the end of an animation.
I also checked LocationBarView::OnPopupStateChanged, and it handles this exact asynchronous transition issue by simply posting a task with a 100ms delay. so sticking with a PostDelayedTask (i'm using 150ms to be safe, [the actual fade-out duration is hardcoded to 82ms](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc;l=222-223;drc=c8fb498259ce5c219f957dbe367da21a82a361c0)) seems to follow the established pattern for handling these fade-outs, rather than introducing new dependencies.
since this is purely for visual smoothing in a rare edge case (focusing the actuating tab's omnibox and then immediately clicking the other split pane) I feel like sticking with the timer is a cleaner trade-off.
Does that sound reasonable? happy to add the observer if you feel strongly, but wanted to double check.
Thanks for looking into it. Do we actutally need new dependencies here? Instead of GetViewAs<> you can simply use GetView(ElementId). That way the only dependency is on the view.h (Which we're already pulling in since we depend on webview.h). You can then get the widget from the view etc.
Other thing is, I think we can just add the element Id property to the OmniboxPopupViewViews in chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc. Something like SetProperty(views::kElementIdentifierKey, kOmniboxPopupView);
At the end of the day, with a 150ms delay or with a direct dependency on visibility, the dependency is there. this component cares about when the OmniboxPopup is truly no longer visible. the explicit dependency in this case (without actual new imports) is preferred compared to the implicit temporal dependency.
Having these (seemingly random) timeouts in the code is still an intrinsic dependency that is less robust for a number of reasons ( the fade-out duration of the omnibox popup could change at any moment, and it's not actually guaranteed that the it would take 82 seconds to disappear)
does this make sense/seem reasonable?
Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michelle Abreo
| Commit-Queue | +1 |
1 comment
close_popup_task_ = std::make_unique<base::CancelableOnceClosure>(
base::BindOnce(&ActorUiContentsContainerController::HandlePopupClosed,
weak_ptr_factory_.GetWeakPtr()));
content::GetUIThreadTaskRunner({})->PostDelayedTask(
FROM_HERE, close_popup_task_->callback(),
base::Milliseconds(kOmniboxPopupHandoffButtonDelay));Michelle Abreonot sure this is the correct approach. I think if you want to know for sure that the omnibox is closed ( no longer appearing on the screen), then Widget Observer is what we want. it has a method you can override: OnWidgetVisibilityChanged. You can get the OmniboxPopup via BrowserElementsViews::From(bwi)
->GetViewAs<WhateverTheOmniboxPopupViewIs>(kWhateverTheElementIdIs) (or via other ways)
Abe BoujaneI looked into the WidgetObserver idea, but it feels like a bit of overkill for this one edge case.
the main blocker is that there isn't a clean way to get that popup widget. it doesn't have a public element ID, and the OmniboxView interface doesn't expose it, so i’d have to include omnibox_view_views.h and explicitly downcast to the concrete OmniboxViewViews implementation. it feels weird to break that abstraction and couple this controller to the internal omnibox UI code just to catch the end of an animation.
I also checked LocationBarView::OnPopupStateChanged, and it handles this exact asynchronous transition issue by simply posting a task with a 100ms delay. so sticking with a PostDelayedTask (i'm using 150ms to be safe, [the actual fade-out duration is hardcoded to 82ms](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc;l=222-223;drc=c8fb498259ce5c219f957dbe367da21a82a361c0)) seems to follow the established pattern for handling these fade-outs, rather than introducing new dependencies.
since this is purely for visual smoothing in a rare edge case (focusing the actuating tab's omnibox and then immediately clicking the other split pane) I feel like sticking with the timer is a cleaner trade-off.
Does that sound reasonable? happy to add the observer if you feel strongly, but wanted to double check.
Thanks for looking into it. Do we actutally need new dependencies here? Instead of GetViewAs<> you can simply use GetView(ElementId). That way the only dependency is on the view.h (Which we're already pulling in since we depend on webview.h). You can then get the widget from the view etc.
Other thing is, I think we can just add the element Id property to the OmniboxPopupViewViews in chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc. Something like SetProperty(views::kElementIdentifierKey, kOmniboxPopupView);
At the end of the day, with a 150ms delay or with a direct dependency on visibility, the dependency is there. this component cares about when the OmniboxPopup is truly no longer visible. the explicit dependency in this case (without actual new imports) is preferred compared to the implicit temporal dependency.
Having these (seemingly random) timeouts in the code is still an intrinsic dependency that is less robust for a number of reasons ( the fade-out duration of the omnibox popup could change at any moment, and it's not actually guaranteed that the it would take 82 seconds to disappear)
does this make sense/seem reasonable?
- Abe Boujane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
Please consult the Omnibox team before introducing a dependency on the Omnibox Popup state. We are in the process of re-implementing the Omnibox and refactoring the code.
Related details
- Abe Boujane
- Michelle Abreo
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. |