Commit 9731e55e authored by David Black's avatar David Black Committed by Commit Bot

Revert "Fix UI jank caused by visibility change."

This reverts commit 9bc4c7ef.

Reason for revert: CL fixed UI jank but regressed accessibility. Will re-implement with another approach.

Original change's description:
> Fix UI jank caused by visibility change.
> 
> Previously, when animating the footer view and/or its children, we
> modified visibility so as not to receive focus/accessibility events on
> hidden views.
> 
> Unfortunately, changing the visibility of the footer view has a side
> effect of freeing up space for UiElementContainerView. When this
> happens, there is a bit of UI jank when the UiElementContainerView
> takes that available space when really that space should permanently be
> reserved for the footer. This becomes more obvious in a follow up CL in
> which we add a shadow above the footer.
> 
> Now, we don't modify view visibility. Instead, we update focus behavior
> as appropriate before animations start and after they end.
> 
> See bug for repro steps.
> 
> Bug: b:113871610
> Change-Id: Ic8aa509c722ace44675a3ab7c7e0453c0aa4a352
> Reviewed-on: https://chromium-review.googlesource.com/1197906
> Commit-Queue: David Black <dmblack@google.com>
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#588926}

TBR=xiaohuic@chromium.org,dmblack@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: b:113871610
Change-Id: I81f93ff5b641fab87e9b00fffc1c6c428ff44388
Reviewed-on: https://chromium-review.googlesource.com/1216812Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#590083}
parent eba60742
...@@ -75,24 +75,6 @@ void AssistantFooterView::ChildVisibilityChanged(views::View* child) { ...@@ -75,24 +75,6 @@ void AssistantFooterView::ChildVisibilityChanged(views::View* child) {
PreferredSizeChanged(); PreferredSizeChanged();
} }
void AssistantFooterView::SetFocusBehavior(FocusBehavior focus_behavior) {
switch (focus_behavior) {
case FocusBehavior::NEVER:
// When focus behavior is NEVER, we update both views.
suggestion_container_->SetFocusBehavior(FocusBehavior::NEVER);
opt_in_view_->SetFocusBehavior(FocusBehavior::NEVER);
break;
case FocusBehavior::ALWAYS:
// When focus behavior is ALWAYS, we need to respect opt in state.
UpdateFocusBehavior();
break;
case FocusBehavior::ACCESSIBLE_ONLY:
// We don't currently use ACCESSIBLE_ONLY focus behavior.
NOTREACHED();
break;
}
}
void AssistantFooterView::InitLayout() { void AssistantFooterView::InitLayout() {
SetLayoutManager(std::make_unique<views::FillLayout>()); SetLayoutManager(std::make_unique<views::FillLayout>());
...@@ -100,6 +82,8 @@ void AssistantFooterView::InitLayout() { ...@@ -100,6 +82,8 @@ void AssistantFooterView::InitLayout() {
const bool setup_completed = const bool setup_completed =
Shell::Get()->voice_interaction_controller()->setup_completed(); Shell::Get()->voice_interaction_controller()->setup_completed();
LOG(ERROR) << "eyor: setup_completed: " << setup_completed;
// Suggestion container. // Suggestion container.
suggestion_container_ = new SuggestionContainerView(assistant_controller_); suggestion_container_ = new SuggestionContainerView(assistant_controller_);
suggestion_container_->set_can_process_events_within_subtree(setup_completed); suggestion_container_->set_can_process_events_within_subtree(setup_completed);
...@@ -108,6 +92,7 @@ void AssistantFooterView::InitLayout() { ...@@ -108,6 +92,7 @@ void AssistantFooterView::InitLayout() {
suggestion_container_->SetPaintToLayer(); suggestion_container_->SetPaintToLayer();
suggestion_container_->layer()->SetFillsBoundsOpaquely(false); suggestion_container_->layer()->SetFillsBoundsOpaquely(false);
suggestion_container_->layer()->SetOpacity(setup_completed ? 1.f : 0.f); suggestion_container_->layer()->SetOpacity(setup_completed ? 1.f : 0.f);
suggestion_container_->SetVisible(setup_completed);
AddChildView(suggestion_container_); AddChildView(suggestion_container_);
...@@ -120,11 +105,9 @@ void AssistantFooterView::InitLayout() { ...@@ -120,11 +105,9 @@ void AssistantFooterView::InitLayout() {
opt_in_view_->SetPaintToLayer(); opt_in_view_->SetPaintToLayer();
opt_in_view_->layer()->SetFillsBoundsOpaquely(false); opt_in_view_->layer()->SetFillsBoundsOpaquely(false);
opt_in_view_->layer()->SetOpacity(setup_completed ? 0.f : 1.f); opt_in_view_->layer()->SetOpacity(setup_completed ? 0.f : 1.f);
opt_in_view_->SetVisible(!setup_completed);
AddChildView(opt_in_view_); AddChildView(opt_in_view_);
// Initialize focus behavior.
UpdateFocusBehavior(setup_completed);
} }
void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) { void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) {
...@@ -132,6 +115,8 @@ void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) { ...@@ -132,6 +115,8 @@ void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) {
using assistant::util::CreateOpacityElement; using assistant::util::CreateOpacityElement;
using assistant::util::StartLayerAnimationSequence; using assistant::util::StartLayerAnimationSequence;
LOG(ERROR) << "eyor: completed: " << completed;
// When the consent state changes, we need to hide/show the appropriate views. // When the consent state changes, we need to hide/show the appropriate views.
views::View* hide_view = views::View* hide_view =
completed ? static_cast<views::View*>(opt_in_view_) completed ? static_cast<views::View*>(opt_in_view_)
...@@ -141,15 +126,14 @@ void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) { ...@@ -141,15 +126,14 @@ void AssistantFooterView::OnVoiceInteractionSetupCompleted(bool completed) {
completed ? static_cast<views::View*>(suggestion_container_) completed ? static_cast<views::View*>(suggestion_container_)
: static_cast<views::View*>(opt_in_view_); : static_cast<views::View*>(opt_in_view_);
// Views should not be focusable while they're animating. // Reset visibility to enable animation.
suggestion_container_->SetFocusBehavior(FocusBehavior::NEVER); hide_view->SetVisible(true);
opt_in_view_->SetFocusBehavior(FocusBehavior::NEVER); show_view->SetVisible(true);
// Hide the view for the previous consent state by fading to 0% opacity. // Hide the view for the previous consent state by fading to 0% opacity.
StartLayerAnimationSequence(hide_view->layer()->GetAnimator(), StartLayerAnimationSequence(hide_view->layer()->GetAnimator(),
CreateLayerAnimationSequence(CreateOpacityElement( CreateLayerAnimationSequence(CreateOpacityElement(
0.f, kAnimationFadeOutDuration)), 0.f, kAnimationFadeOutDuration)),
// Observe the animation.
animation_observer_.get()); animation_observer_.get());
// Show the view for the next consent state by fading to 100% opacity with // Show the view for the next consent state by fading to 100% opacity with
...@@ -182,25 +166,12 @@ bool AssistantFooterView::OnAnimationEnded( ...@@ -182,25 +166,12 @@ bool AssistantFooterView::OnAnimationEnded(
// Only the view relevant to our consent state should process events. // Only the view relevant to our consent state should process events.
suggestion_container_->set_can_process_events_within_subtree(setup_completed); suggestion_container_->set_can_process_events_within_subtree(setup_completed);
suggestion_container_->SetVisible(setup_completed);
opt_in_view_->set_can_process_events_within_subtree(!setup_completed); opt_in_view_->set_can_process_events_within_subtree(!setup_completed);
opt_in_view_->SetVisible(!setup_completed);
UpdateFocusBehavior(setup_completed);
// Return false to prevent the observer from destroying itself. // Return false to prevent the observer from destroying itself.
return false; return false;
} }
void AssistantFooterView::UpdateFocusBehavior(
base::Optional<bool> setup_completed) {
if (!setup_completed.has_value()) {
setup_completed =
Shell::Get()->voice_interaction_controller()->setup_completed();
}
suggestion_container_->SetFocusBehavior(
setup_completed.value() ? FocusBehavior::ALWAYS : FocusBehavior::NEVER);
opt_in_view_->SetFocusBehavior(
setup_completed.value() ? FocusBehavior::NEVER : FocusBehavior::ALWAYS);
}
} // namespace ash } // namespace ash
...@@ -9,7 +9,6 @@ ...@@ -9,7 +9,6 @@
#include "ash/public/interfaces/voice_interaction_controller.mojom.h" #include "ash/public/interfaces/voice_interaction_controller.mojom.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/optional.h"
#include "mojo/public/cpp/bindings/binding_set.h" #include "mojo/public/cpp/bindings/binding_set.h"
#include "ui/views/view.h" #include "ui/views/view.h"
...@@ -46,17 +45,12 @@ class AssistantFooterView : public views::View, ...@@ -46,17 +45,12 @@ class AssistantFooterView : public views::View,
mojom::AssistantAllowedState state) override {} mojom::AssistantAllowedState state) override {}
void OnLocaleChanged(const std::string& locale) override {} void OnLocaleChanged(const std::string& locale) override {}
void SetFocusBehavior(FocusBehavior focus_behavior);
private: private:
void InitLayout(); void InitLayout();
void OnAnimationStarted(const ui::CallbackLayerAnimationObserver& observer); void OnAnimationStarted(const ui::CallbackLayerAnimationObserver& observer);
bool OnAnimationEnded(const ui::CallbackLayerAnimationObserver& observer); bool OnAnimationEnded(const ui::CallbackLayerAnimationObserver& observer);
void UpdateFocusBehavior(
base::Optional<bool> setup_completed = base::nullopt);
AssistantController* const assistant_controller_; // Owned by Shell. AssistantController* const assistant_controller_; // Owned by Shell.
SuggestionContainerView* suggestion_container_; // Owned by view hierarchy. SuggestionContainerView* suggestion_container_; // Owned by view hierarchy.
......
...@@ -648,8 +648,8 @@ void AssistantMainStage::UpdateFooter() { ...@@ -648,8 +648,8 @@ void AssistantMainStage::UpdateFooter() {
// When it is not visible, it should not process events. // When it is not visible, it should not process events.
bool visible = !committed_query_view_ && !pending_query_view_; bool visible = !committed_query_view_ && !pending_query_view_;
// The footer should not be focusable while it's animating. // Reset visibility to enable animation.
footer_->SetFocusBehavior(FocusBehavior::NEVER); footer_->SetVisible(true);
if (visible) { if (visible) {
// The footer will animate up into position so we need to set an initial // The footer will animate up into position so we need to set an initial
...@@ -704,8 +704,7 @@ bool AssistantMainStage::OnFooterAnimationEnded( ...@@ -704,8 +704,7 @@ bool AssistantMainStage::OnFooterAnimationEnded(
// there is no committed or pending query view. // there is no committed or pending query view.
bool visible = !committed_query_view_ && !pending_query_view_; bool visible = !committed_query_view_ && !pending_query_view_;
footer_->set_can_process_events_within_subtree(visible); footer_->set_can_process_events_within_subtree(visible);
footer_->SetFocusBehavior(visible ? FocusBehavior::ALWAYS footer_->SetVisible(visible);
: FocusBehavior::NEVER);
// Return false so that the observer does not destroy itself. // Return false so that the observer does not destroy itself.
return false; return false;
......
...@@ -91,16 +91,6 @@ void AssistantOptInView::OnBoundsChanged(const gfx::Rect& previous_bounds) { ...@@ -91,16 +91,6 @@ void AssistantOptInView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
label_->SizeToFit(width()); label_->SizeToFit(width());
} }
void AssistantOptInView::ButtonPressed(views::Button* sender,
const ui::Event& event) {
if (delegate_)
delegate_->OnOptInButtonPressed();
}
void AssistantOptInView::SetFocusBehavior(FocusBehavior focus_behavior) {
container_->SetFocusBehavior(focus_behavior);
}
void AssistantOptInView::InitLayout() { void AssistantOptInView::InitLayout() {
views::BoxLayout* layout_manager = views::BoxLayout* layout_manager =
SetLayoutManager(std::make_unique<views::BoxLayout>( SetLayoutManager(std::make_unique<views::BoxLayout>(
...@@ -113,17 +103,18 @@ void AssistantOptInView::InitLayout() { ...@@ -113,17 +103,18 @@ void AssistantOptInView::InitLayout() {
views::BoxLayout::MainAxisAlignment::MAIN_AXIS_ALIGNMENT_CENTER); views::BoxLayout::MainAxisAlignment::MAIN_AXIS_ALIGNMENT_CENTER);
// Container. // Container.
container_ = new AssistantOptInContainer(/*listener=*/this); AssistantOptInContainer* container =
new AssistantOptInContainer(/*listener=*/this);
layout_manager = layout_manager =
container_->SetLayoutManager(std::make_unique<views::BoxLayout>( container->SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kHorizontal, views::BoxLayout::Orientation::kHorizontal,
gfx::Insets(0, kPaddingDip))); gfx::Insets(0, kPaddingDip)));
layout_manager->set_cross_axis_alignment( layout_manager->set_cross_axis_alignment(
views::BoxLayout::CrossAxisAlignment::CROSS_AXIS_ALIGNMENT_CENTER); views::BoxLayout::CrossAxisAlignment::CROSS_AXIS_ALIGNMENT_CENTER);
AddChildView(container_); AddChildView(container);
// Label. // Label.
label_ = new views::StyledLabel(base::string16(), /*listener=*/nullptr); label_ = new views::StyledLabel(base::string16(), /*listener=*/nullptr);
...@@ -155,10 +146,16 @@ void AssistantOptInView::InitLayout() { ...@@ -155,10 +146,16 @@ void AssistantOptInView::InitLayout() {
gfx::Range(offsets.at(1), offsets.at(1) + get_started.length()), gfx::Range(offsets.at(1), offsets.at(1) + get_started.length()),
CreateStyleInfo(gfx::Font::Weight::BOLD)); CreateStyleInfo(gfx::Font::Weight::BOLD));
container_->AddChildView(label_); container->AddChildView(label_);
container->SetFocusForPlatform();
container->SetAccessibleName(label_text);
}
container_->SetFocusForPlatform(); void AssistantOptInView::ButtonPressed(views::Button* sender,
container_->SetAccessibleName(label_text); const ui::Event& event) {
if (delegate_)
delegate_->OnOptInButtonPressed();
} }
} // namespace ash } // namespace ash
...@@ -41,12 +41,9 @@ class AssistantOptInView : public views::View, public views::ButtonListener { ...@@ -41,12 +41,9 @@ class AssistantOptInView : public views::View, public views::ButtonListener {
void set_delegate(AssistantOptInDelegate* delegate) { delegate_ = delegate; } void set_delegate(AssistantOptInDelegate* delegate) { delegate_ = delegate; }
void SetFocusBehavior(FocusBehavior focus_behavior);
private: private:
void InitLayout(); void InitLayout();
views::Button* container_; // Owned by view hierarchy.
views::StyledLabel* label_; // Owned by view hierarchy. views::StyledLabel* label_; // Owned by view hierarchy.
AssistantOptInDelegate* delegate_ = nullptr; AssistantOptInDelegate* delegate_ = nullptr;
......
...@@ -137,7 +137,6 @@ void SuggestionContainerView::OnSuggestionsChanged( ...@@ -137,7 +137,6 @@ void SuggestionContainerView::OnSuggestionsChanged(
app_list::SuggestionChipView* suggestion_chip_view = app_list::SuggestionChipView* suggestion_chip_view =
new app_list::SuggestionChipView(params, /*listener=*/this); new app_list::SuggestionChipView(params, /*listener=*/this);
suggestion_chip_view->SetAccessibleName(params.text); suggestion_chip_view->SetAccessibleName(params.text);
suggestion_chip_view->SetFocusBehavior(focus_behavior_);
// Given a suggestion chip view, we need to be able to look up the id of // Given a suggestion chip view, we need to be able to look up the id of
// the underlying suggestion. This is used for handling press events. // the underlying suggestion. This is used for handling press events.
...@@ -190,12 +189,6 @@ void SuggestionContainerView::ButtonPressed(views::Button* sender, ...@@ -190,12 +189,6 @@ void SuggestionContainerView::ButtonPressed(views::Button* sender,
suggestion); suggestion);
} }
void SuggestionContainerView::SetFocusBehavior(FocusBehavior focus_behavior) {
focus_behavior_ = focus_behavior;
for (int i = 0; i < content_view()->child_count(); ++i)
content_view()->child_at(i)->SetFocusBehavior(focus_behavior);
}
void SuggestionContainerView::OnUiVisibilityChanged( void SuggestionContainerView::OnUiVisibilityChanged(
AssistantVisibility new_visibility, AssistantVisibility new_visibility,
AssistantVisibility old_visibility, AssistantVisibility old_visibility,
......
...@@ -62,8 +62,6 @@ class SuggestionContainerView : public AssistantScrollView, ...@@ -62,8 +62,6 @@ class SuggestionContainerView : public AssistantScrollView,
// views::ButtonListener: // views::ButtonListener:
void ButtonPressed(views::Button* sender, const ui::Event& event) override; void ButtonPressed(views::Button* sender, const ui::Event& event) override;
void SetFocusBehavior(FocusBehavior focus_behavior);
private: private:
void InitLayout(); void InitLayout();
...@@ -76,8 +74,6 @@ class SuggestionContainerView : public AssistantScrollView, ...@@ -76,8 +74,6 @@ class SuggestionContainerView : public AssistantScrollView,
AssistantController* const assistant_controller_; // Owned by Shell. AssistantController* const assistant_controller_; // Owned by Shell.
FocusBehavior focus_behavior_ = FocusBehavior::NEVER;
views::BoxLayout* layout_manager_; // Owned by view hierarchy. views::BoxLayout* layout_manager_; // Owned by view hierarchy.
// Cache of suggestion chip views owned by the view hierarchy. The key for the // Cache of suggestion chip views owned by the view hierarchy. The key for the
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment