[omnibox] Implement arrow animation for AI Mode button [chromium/src : main]
Aviv Kiss (Gerrit)
Aviv Kiss voted and added 4 comments![]( "Open in Gerrit")
Votes added by Aviv Kiss
| Code-Review | +1 |
4 comments
Overall LG but maybe we should find someone with some more views expertise to take a look
}AI review comment:
Returning early here bypasses the `standard_height` enforcement below, which could lead to the button rendering smaller than the standard location bar height.
You can preserve the minimum height behavior like this:
```cpp
int preferred_height = std::max(
PageActionIconView::CalculatePreferredSize(available_size).height(),
GetLayoutConstant(LayoutConstant::kLocationBarHeight));
return ai_mode_layout::CalculatePreferredSize(available_size, label(),
preferred_height);
```
}AI review comment:
Nesting `ResetSlideAnimation(true)` inside the `if (is_visible)` block changes the behavior for the non-dynamic mode when `is_visible` is false (it used to be called unconditionally). Please verify if skipping this reset for hidden buttons was intended, or restructure the `else` to preserve the original behavior.
is_dynamic_ai_mode_(action_item_->GetActionId().value() ==AI review comment:
Calling `.value()` on `GetActionId()` before confirming `has_value()` will cause a `std::bad_optional_access` crash if it happens to be empty.
The `CHECK` on line 61 happens too late because it runs after the initializer list. You can safely compare the `std::optional` directly with the value instead:
```cpp
is_dynamic_ai_mode_(action_item_->GetActionId() == kActionAiMode &&
base::FeatureList::IsEnabled(
omnibox::kWebUIOmniboxDynamicAiModeButton)) {
```
Related details
- Caitlin Chen
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. |
Caitlin Chen (Gerrit)
Caitlin Chen added 4 comments![]( "Open in Gerrit")
Overall LG but maybe we should find someone with some more views expertise to take a look
Agreed!
AI review comment:
Returning early here bypasses the `standard_height` enforcement below, which could lead to the button rendering smaller than the standard location bar height.
You can preserve the minimum height behavior like this:
```cpp
int preferred_height = std::max(
PageActionIconView::CalculatePreferredSize(available_size).height(),
GetLayoutConstant(LayoutConstant::kLocationBarHeight));
return ai_mode_layout::CalculatePreferredSize(available_size, label(),
preferred_height);
```
Done
AI review comment:
Nesting `ResetSlideAnimation(true)` inside the `if (is_visible)` block changes the behavior for the non-dynamic mode when `is_visible` is false (it used to be called unconditionally). Please verify if skipping this reset for hidden buttons was intended, or restructure the `else` to preserve the original behavior.
Done
is_dynamic_ai_mode_(action_item_->GetActionId().value() ==AI review comment:
Calling `.value()` on `GetActionId()` before confirming `has_value()` will cause a `std::bad_optional_access` crash if it happens to be empty.
The `CHECK` on line 61 happens too late because it runs after the initializer list. You can safely compare the `std::optional` directly with the value instead:
```cpp
is_dynamic_ai_mode_(action_item_->GetActionId() == kActionAiMode &&
base::FeatureList::IsEnabled(
omnibox::kWebUIOmniboxDynamicAiModeButton)) {
```
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 2 comments![]( "Open in Gerrit")
+cc Caleb, who is working on the WebUI version of page actions
// Set up overlapping arrow and GPU layers for AI Mode fade/scale transition.
if (is_dynamic_ai_mode_) {
arrow_container_view_ = AddChildView(std::make_unique<views::ImageView>());
arrow_container_view_->SetCanProcessEventsWithinSubtree(false);
arrow_container_view_->SetPaintToLayer();
arrow_container_view_->layer()->SetFillsBoundsOpaquely(false);
ReorderChildView(arrow_container_view_.get(), 1);
image_container_view()->SetPaintToLayer();
image_container_view()->layer()->SetFillsBoundsOpaquely(false);
SetPaintToLayer();
layer()->SetFillsBoundsOpaquely(false);
layer()->SetMasksToBounds(true);
SetUpForAnimation(base::Milliseconds(300));
slide_animation_.SetTweenType(gfx::Tween::EASE_IN_OUT);
label()->SetElideBehavior(gfx::ELIDE_TAIL);
}We shouldn't be adding feature-specific logic like this into the framework.
For context, we've been migrating from the legacy page action system (e.g. all the subclasses of PageActionIconView, which will be removed soon) to a centralized framework where the View is feature-unaware. The motivation for this is that these 20+ per-feature View implementations diverging, which created an inconsistent UI and made it things like UI refreshes (which is the next phase after the migration completes) complex.
Adding AI-mode specific logic here is essentially restarting this pattern that we we've been trying to get rid of.
There are two ways forward here:
A) Removing AI Mode from the Page Actions framework. This might make sense since it hasn't fit very well from the start (e.g. allowing it to show while the omnibox is open: https://chromium-review.git.corp.google.com/c/chromium/src/+/6966699).
B) Spec out the desired behavior in such a way that it could be applied to existing and new page actions. We already had the idea of animating icons in mind, so this could be an extension of that. I think it would be good to loop in UX here.
Related details
- Caitlin Chen
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. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
// Set up overlapping arrow and GPU layers for AI Mode fade/scale transition.
if (is_dynamic_ai_mode_) {
arrow_container_view_ = AddChildView(std::make_unique<views::ImageView>());
arrow_container_view_->SetCanProcessEventsWithinSubtree(false);
arrow_container_view_->SetPaintToLayer();
arrow_container_view_->layer()->SetFillsBoundsOpaquely(false);
ReorderChildView(arrow_container_view_.get(), 1);
image_container_view()->SetPaintToLayer();
image_container_view()->layer()->SetFillsBoundsOpaquely(false);
SetPaintToLayer();
layer()->SetFillsBoundsOpaquely(false);
layer()->SetMasksToBounds(true);
SetUpForAnimation(base::Milliseconds(300));
slide_animation_.SetTweenType(gfx::Tween::EASE_IN_OUT);
label()->SetElideBehavior(gfx::ELIDE_TAIL);
}We shouldn't be adding feature-specific logic like this into the framework.
For context, we've been migrating from the legacy page action system (e.g. all the subclasses of PageActionIconView, which will be removed soon) to a centralized framework where the View is feature-unaware. The motivation for this is that these 20+ per-feature View implementations diverging, which created an inconsistent UI and made it things like UI refreshes (which is the next phase after the migration completes) complex.
Adding AI-mode specific logic here is essentially restarting this pattern that we we've been trying to get rid of.
There are two ways forward here:
A) Removing AI Mode from the Page Actions framework. This might make sense since it hasn't fit very well from the start (e.g. allowing it to show while the omnibox is open: https://chromium-review.git.corp.google.com/c/chromium/src/+/6966699).B) Spec out the desired behavior in such a way that it could be applied to existing and new page actions. We already had the idea of animating icons in mind, so this could be an extension of that. I think it would be good to loop in UX here.
Following up on B), we did just add icon animations for the bookmark star: https://chromium-review.git.corp.google.com/c/chromium/src/+/7888303.
Building off of that would be ideal.
Caitlin Chen (Gerrit)
Caitlin Chen added 1 comment![]( "Open in Gerrit")
// Set up overlapping arrow and GPU layers for AI Mode fade/scale transition.
if (is_dynamic_ai_mode_) {
arrow_container_view_ = AddChildView(std::make_unique<views::ImageView>());
arrow_container_view_->SetCanProcessEventsWithinSubtree(false);
arrow_container_view_->SetPaintToLayer();
arrow_container_view_->layer()->SetFillsBoundsOpaquely(false);
ReorderChildView(arrow_container_view_.get(), 1);
image_container_view()->SetPaintToLayer();
image_container_view()->layer()->SetFillsBoundsOpaquely(false);
SetPaintToLayer();
layer()->SetFillsBoundsOpaquely(false);
layer()->SetMasksToBounds(true);
SetUpForAnimation(base::Milliseconds(300));
slide_animation_.SetTweenType(gfx::Tween::EASE_IN_OUT);
label()->SetElideBehavior(gfx::ELIDE_TAIL);
}Kaan AlsanWe shouldn't be adding feature-specific logic like this into the framework.
For context, we've been migrating from the legacy page action system (e.g. all the subclasses of PageActionIconView, which will be removed soon) to a centralized framework where the View is feature-unaware. The motivation for this is that these 20+ per-feature View implementations diverging, which created an inconsistent UI and made it things like UI refreshes (which is the next phase after the migration completes) complex.
Adding AI-mode specific logic here is essentially restarting this pattern that we we've been trying to get rid of.
There are two ways forward here:
A) Removing AI Mode from the Page Actions framework. This might make sense since it hasn't fit very well from the start (e.g. allowing it to show while the omnibox is open: https://chromium-review.git.corp.google.com/c/chromium/src/+/6966699).B) Spec out the desired behavior in such a way that it could be applied to existing and new page actions. We already had the idea of animating icons in mind, so this could be an extension of that. I think it would be good to loop in UX here.
Following up on B), we did just add icon animations for the bookmark star: https://chromium-review.git.corp.google.com/c/chromium/src/+/7888303.
Building off of that would be ideal.
Hello @alsan! I ended up going with Option A because the animations seemed to be too much in an experimental state to go with Option B.
- I decided to make a subclass AiModePageActionView. I tried to change PageActionView minimally and have no knowledge of the subclass.
- Keep the changes is legacy so we can we don't have to enable too many flags in our LEs.
- Move all helpers to new directory.
Please let me know what you think of the current state of the CL. I think its possible to migrate out any existing Ai Mode specific stuff into this new subclass but in separate CLs.
Related details
- Kaan Alsan
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. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
In the current state of the CL, we still have feature-specific logic within the page actions framework (e.g. the new View is still owned by PageActionsContainerView), which isn't a scalable pattern: if more features just subclassed PageActionView, we'd end up in the same spot as before the migration.
If we're going the route of removing AI Mode from the page actions framework kActionAiMode action should be removed from chrome/browser/ui/page_action/action_ids.h.
Also, the view should be owned by LocationBarView instead, similar to Content Settings: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/location_bar/location_bar_view.cc;l=480;drc=6ce6f0f91493b5c3a3ce713a2ef454db5deb45a3.
Related details
- Caitlin Chen
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. |
Caitlin Chen (Gerrit)
Caitlin Chen added 1 comment![]( "Open in Gerrit")
Thanks for the feedback! Sorry I misunderstood what you meant with Option A and will explore a route closer to what you are suggesting.
I did talk with the team and I wanted to raise that we are currently implementing a full webui of the omnibox popup instance including the input bar.This would mean that all of these changes would be replaced in a few months and Views would no longer be used for the omnibox popup. This specific animation is only relevant when the omnibox is clicked into and the popup is open.
The team would like to experiment with a more dynamic looking button and I am trying to balance the amount of potential throw away work.
After explaining this, what are your thoughts about this subclass solution for the in between time instead?? Would this negatively impact the the PageActionView migration long term??
Related details
- Kaan Alsan
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. |
Caitlin Chen (Gerrit)
Just adding on that I started a prototype of Option A where we make a new button entirely
https://chromium-review.git.corp.google.com/c/chromium/src/+/7960637
Not sure if there are any regressions or big caveats here but just wanted to make aware that I am exploring options if needed.
Related details
- Kaan Alsan
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. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
The page actions migration is complete, but we have plans for the near future to apply a visual refresh to the page actions and are already working on introducing new capabilities (anchored messaging/priority chip selection), which will be directly affected by this.
I think it's also worth considering option B, since this animation is generalizable (change the icon and move it to the right/left of the label), so it may not be too much effort.
Related details
- Caitlin Chen
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. |
Caitlin Chen (Gerrit)
Caitlin Chen added 1 comment![]( "Open in Gerrit")
Hey Kaan! I took a look at exploring Option B and before I get too deep into cleaning the implementation up here is the prototype.
https://chromium-review.git.corp.google.com/c/chromium/src/+/7965783
Overall I am introducing a way to add different animations that are compatible with the slide_animation. Please let me know your initial thoughts and feel free to directly comment as I clean it up and refine it!.
Related details
- Kaan Alsan
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. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
- Caitlin Chen
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. |