Call OnViewVisibilityChanged when an ancestor's visibility changes

ViewObserver::OnViewVisibilityChanged is currently only called when
the observed view's visible() state changes. This is different than
View::VisibilityChanged, which is called whenever SetVisible() is
called on the observed view or any of its ancestors, or whenever the
parent widget's visibility changes.

This makes OnViewVisibilityChanged's behavior and interface match
VisibilityChanged.

In doing so, this fixes an InkDrop bug where InkDrop state incorrectly
remained after a view was no longer drawn. This bug resulted from
switching to the ViewObserver interface.

Bug: 943979, 949067
Change-Id: I35cb4db5625060e2564f6c295dd047ca57c7a8ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1553708
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650441}

ash/assistant/ui/main_stage/assistant_main_stage.cc

@@ -251,7 +251,8 @@ void AssistantMainStage::OnViewPreferredSizeChanged(views::View* view) {
   PreferredSizeChanged();
 }
 
-void AssistantMainStage::OnViewVisibilityChanged(views::View* view) {
+void AssistantMainStage::OnViewVisibilityChanged(views::View* view,
+                                                 views::View* starting_view) {
   PreferredSizeChanged();
 }
 

ash/assistant/ui/main_stage/assistant_main_stage.h

@@ -50,7 +50,8 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantMainStage
   // views::ViewObserver:
   void OnViewBoundsChanged(views::View* view) override;
   void OnViewPreferredSizeChanged(views::View* view) override;
-  void OnViewVisibilityChanged(views::View* view) override;
+  void OnViewVisibilityChanged(views::View* view,
+                               views::View* starting_view) override;
 
   // AssistantInteractionModelObserver:
   void OnCommittedQueryChanged(const AssistantQuery& query) override;

chrome/browser/ui/views/media_router/presentation_receiver_window_view_browsertest.cc

@@ -139,7 +139,8 @@ IN_PROC_BROWSER_TEST_F(PresentationReceiverWindowViewBrowserTest,
     }
 
    private:
-    void OnViewVisibilityChanged(views::View* observed_view) override {
+    void OnViewVisibilityChanged(views::View* observed_view,
+                                 views::View* starting_view) override {
       bool fullscreen = !observed_view->visible();
       EXPECT_EQ(fullscreen, receiver_view_->IsFullscreen());
       if (fullscreen == (await_type_ == AwaitType::kIntoFullscreen))

ui/views/animation/ink_drop_event_handler.cc

@@ -11,6 +11,7 @@
 #include "ui/views/animation/ink_drop.h"
 #include "ui/views/animation/ink_drop_state.h"
 #include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
 
 namespace views {
 InkDropEventHandler::InkDropEventHandler(View* host_view, Delegate* delegate)
@@ -115,9 +116,15 @@ void InkDropEventHandler::OnMouseEvent(ui::MouseEvent* event) {
   }
 }
 
-void InkDropEventHandler::OnViewVisibilityChanged(View* observed_view) {
+void InkDropEventHandler::OnViewVisibilityChanged(View* observed_view,
+                                                  View* starting_view) {
   DCHECK_EQ(host_view_, observed_view);
-  if (!host_view_->visible() && delegate_->HasInkDrop()) {
+  // A View is *actually* visible if its visible flag is set, all its ancestors'
+  // visible flags are set, it's in a Widget, and the Widget is
+  // visible. |View::IsDrawn()| captures the first two conditions.
+  const bool is_visible = host_view_->IsDrawn() && host_view_->GetWidget() &&
+                          host_view_->GetWidget()->IsVisible();
+  if (!is_visible && delegate_->HasInkDrop()) {
     delegate_->GetInkDrop()->AnimateToState(InkDropState::HIDDEN);
     delegate_->GetInkDrop()->SetHovered(false);
   }

ui/views/animation/ink_drop_event_handler.h

@@ -50,7 +50,8 @@ class VIEWS_EXPORT InkDropEventHandler : public ui::EventHandler,
   void OnMouseEvent(ui::MouseEvent* event) override;
 
   // ViewObserver:
-  void OnViewVisibilityChanged(View* observed_view) override;
+  void OnViewVisibilityChanged(View* observed_view,
+                               View* starting_view) override;
   void OnViewHierarchyChanged(
       View* observed_view,
       const ViewHierarchyChangedDetails& details) override;

ui/views/view.cc

@@ -440,9 +440,6 @@ void View::SetVisible(bool visible) {
                                         false);
     }
 
-    for (ViewObserver& observer : observers_)
-      observer.OnViewVisibilityChanged(this);
-
     // This notifies all sub-views recursively.
     PropagateVisibilityNotifications(this, visible_);
     UpdateLayerVisibility();
@@ -2236,6 +2233,8 @@ void View::PropagateVisibilityNotifications(View* start, bool is_visible) {
 
 void View::VisibilityChangedImpl(View* starting_from, bool is_visible) {
   VisibilityChanged(starting_from, is_visible);
+  for (ViewObserver& observer : observers_)
+    observer.OnViewVisibilityChanged(this, starting_from);
 }
 
 void View::SnapLayerToPixelBoundary(const LayerOffsetData& offset_data) {

ui/views/view_observer.h

@@ -22,9 +22,11 @@ class VIEWS_EXPORT ViewObserver {
   // Called when |child| is removed as a child of |observed_view|.
   virtual void OnChildViewRemoved(View* observed_view, View* child) {}
 
-  // Called when View::SetVisible() is called with a new value. See
-  // View::IsDrawn() for details on how visibility and drawn differ.
-  virtual void OnViewVisibilityChanged(View* observed_view) {}
+  // Called when |observed_view|, an ancestor, or its Widget has its visibility
+  // changed. |starting_view| is who |View::SetVisible()| was called on (or null
+  // if the Widget visibility changed).
+  virtual void OnViewVisibilityChanged(View* observed_view,
+                                       View* starting_view) {}
 
   // Called from View::PreferredSizeChanged().
   virtual void OnViewPreferredSizeChanged(View* observed_view) {}

ui/views/view_unittest.cc

@@ -5032,8 +5032,9 @@ class ViewObserverTest : public ViewTest, public ViewObserver {
     child_view_removed_parent_ = parent;
   }
 
-  void OnViewVisibilityChanged(View* view) override {
+  void OnViewVisibilityChanged(View* view, View* starting_view) override {
     view_visibility_changed_ = view;
+    view_visibility_changed_starting_ = starting_view;
   }
 
   void OnViewBoundsChanged(View* view) override { view_bounds_changed_ = view; }
@@ -5073,6 +5074,9 @@ class ViewObserverTest : public ViewTest, public ViewObserver {
   const View* view_visibility_changed() const {
     return view_visibility_changed_;
   }
+  const View* view_visibility_changed_starting() const {
+    return view_visibility_changed_starting_;
+  }
   const View* view_bounds_changed() const { return view_bounds_changed_; }
   const View* view_reordered() const { return view_reordered_; }
 
@@ -5085,6 +5089,7 @@ class ViewObserverTest : public ViewTest, public ViewObserver {
   View* child_view_removed_ = nullptr;
   View* child_view_removed_parent_ = nullptr;
   View* view_visibility_changed_ = nullptr;
+  View* view_visibility_changed_starting_ = nullptr;
   View* view_bounds_changed_ = nullptr;
   View* view_reordered_ = nullptr;
 
@@ -5123,10 +5128,26 @@ TEST_F(ViewObserverTest, ViewParentChanged) {
 }
 
 TEST_F(ViewObserverTest, ViewVisibilityChanged) {
-  std::unique_ptr<View> view = NewView();
+  std::unique_ptr<View> parent(new View);
+  View* view = parent->AddChildView(NewView());
+
+  // Ensure setting |view| itself not visible calls the observer.
   view->SetVisible(false);
-  EXPECT_EQ(view.get(), view_visibility_changed());
-  EXPECT_FALSE(view->visible());
+  EXPECT_EQ(view, view_visibility_changed());
+  EXPECT_EQ(view, view_visibility_changed_starting());
+  reset();
+
+  // Ditto for setting it visible.
+  view->SetVisible(true);
+  EXPECT_EQ(view, view_visibility_changed());
+  EXPECT_EQ(view, view_visibility_changed_starting());
+  reset();
+
+  // Ensure setting |parent| not visible also calls the
+  // observer. |view->visible()| should still return true however.
+  parent->SetVisible(false);
+  EXPECT_EQ(view, view_visibility_changed());
+  EXPECT_EQ(parent.get(), view_visibility_changed_starting());
 }
 
 TEST_F(ViewObserverTest, ViewBoundsChanged) {