fun. Normally LocationBarView::OnBoundsChanged() would invoke >UpdatePopupAppearance(), but if only the window height changes, the LocationBarView bounds relative to its parent doesn't need to change.

 void LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   OmniboxPopupView* popup = GetOmniboxPopupView();
   if (popup->IsOpen())
     popup->UpdatePopupAppearance();
   RefreshBackground();
   RefreshFocusRing();
 }

tab-modal dialogs update correctly. They move themselves at the end of BrowserViewLayout::Layout(), so we could add a method like LocationBarView::NotifyPositionRequiresUpdate() which invokes UpdatePopupAppearance(), and call it after re positioning dialogs.

Or we could do what other platforms do and just dismiss the popup.. but I think that's a bug. E.g. on Linux I can resize the bottom and right-edge of the window _without_ the popup dismissing, but resizing the top or left edges causes it to dismiss.

Also doing it on Mac would be a regression. The Cocoa omnibox does not dismiss when the window is moved or resized.

In MacViews, we currently _do_ dismiss when the entire window moves, but not when it's resized. That's because the dismissal hooks in at

 void BrowserView::OnWidgetMove() {
   ..

   // Close the omnibox popup, if any.
   LocationBarView* location_bar_view = GetLocationBarView();
   if (location_bar_view)
     location_bar_view->GetOmniboxView()->CloseOmniboxPopup();
 }

and Aura has this funky hack:

 void NativeWidgetAura::OnBoundsChanged(const gfx::Rect& old_bounds,
                                        const gfx::Rect& new_bounds) {
   // Assume that if the old bounds was completely empty a move happened. This
   // handles the case of a maximize animation acquiring the layer (acquiring a
   // layer results in clearing the bounds).
   if (old_bounds.origin() != new_bounds.origin() ||
       (old_bounds == gfx::Rect(0, 0, 0, 0) && !new_bounds.IsEmpty())) {
     delegate_->OnNativeWidgetMove();
   }
   if (old_bounds.size() != new_bounds.size())
     delegate_->OnNativeWidgetSizeChanged(new_bounds.size());
 }

which is repeated in desktop_window_tree_host_x11.cc

      if (origin_changed)
        OnHostMovedInPixels(bounds_in_pixels_.origin());


but... seemingly not on on windows - maybe the OS generates WM_MOVE automatically.


So.. I think it's a bug that we don't generate OnWidgetMove() on MacViews - I'll own fixing that.

(I guess I just need to add the same origin-changed check to void BridgedNativeWidget::OnSizeChanged() :/ and add some cross-platform tests).


Separately, we should decide whether we should delete `CloseOmniboxPopup()` from BrowserView::OnWidgetMoved() and instead invoke UpdatePopupAppearance() in BrowserViewLayout::Layout()

Adding "RBS" label as this is P1 and blocking MacViews launch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/910a421680802dc4b7fdf6a470d9e4274d81f6dd

commit 910a421680802dc4b7fdf6a470d9e4274d81f6dd
Author: Trent Apted <tapted@chromium.org>
Date: Wed Jul 18 23:40:21 2018

MacViews: Ensure WidgetDelegate::OnWidgetMove() is invoked consistently.

Since AppKit coordinates are flipped, NSWindowDelegate may not send
move changes when the top-left corner of the window moves relative to
the screen. Similarly, it will send a move when resizing a window
vertically via the bottom edge, which other platforms do not class as a
move.

To fix, track the top-left window corner relative to the top-left of the
screen and ensure the toolkit is notified when it changes.

OnWidgetMove() had no test coverage, so add a cross-platform widget test.
The test exposes a discrepancy on Windows only, which is likely a bug to
investigate in a follow-up.

Bug:  862217 ,  864938 
Change-Id: I1fb0a6c2b463ea7cbd823fbe4d65545991564d50
Reviewed-on: https://chromium-review.googlesource.com/1141555
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576255}
[modify] https://crrev.com/910a421680802dc4b7fdf6a470d9e4274d81f6dd/ui/views/cocoa/bridged_native_widget.h
[modify] https://crrev.com/910a421680802dc4b7fdf6a470d9e4274d81f6dd/ui/views/cocoa/bridged_native_widget.mm
[modify] https://crrev.com/910a421680802dc4b7fdf6a470d9e4274d81f6dd/ui/views/widget/widget_unittest.cc

Fixed.

I created Issue 865301 to discuss whether we should try keeping the omnibox open in more circumstances.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.

M69 branch #3497, branched at chromium revision #576753. No merge needed. 

Able to reproduce the issue on Mac 10.13.5 on reported version tested 69.0.3487.0.

Verified the fix on Mac 10.13.5, as per comment#0 on latest chrome version #70.0.3500.0.
Attaching screen cast for reference.
Observed that Omnibox result list is attached to the Omnibox.
Hence, the fix is working as expected.
Adding the verified labels.

Thanks...!!

Deleted: 862217.mp4 888 KB

	862217.mp4 888 KB View Download