Fix wrong popup offset when changing popup bounds.

Fix wrong popup offset when changing popup bounds.

The popup's aura::Window's bounds are relative to the native widget. However,
when we change the bounds from Blink, they get applied to the aura::Window and,
through various observers, end up resizing and moving the host widget in
DesktopNativeWidgetTopLevelHandler. But the aura::Window's bounds origin isn't
ever reset. This is problematic in the bug as the following happens:

1. RenderWidgetHostViewAura gets the bounds change message from Blink.
2. It resizes the popup menu's aura::Window and moves it up -20 pixels along
the y-axis.
3. DesktopNativeWidgetTopLevelHandler observes the aura::Window for changes and
sets the native widget's screen bounds to match the aura::Window
4.  The native widget is now pushed up 20 pixels, but the aura::Window still
has a -20 y-offset so the content is shifted up 40 pixels.

This used to appear to work because Blink would send the bounds change message
without regard for whether the bounds actually changed. These multiple messages
moving the popup to the same location would end up clearing the aura::Window's
offset. In r389984 I made WebWidgetPopupImpl::resize only call into
setWindowRect if the bounds actually change which exposed this bug.

The fix here is to make DesktopNativeWidgetTopLevelHandler clear the
aura::Window's origin when it sets the bounds on the native widget. I also
fixed a bug I noticed in WebWidgetPopupImpl::resize in the condition used to
check whether the bounds changed.


BUG=633140

[bokan, 2016-08-09 20:38:50]

bokan@chromium.org changed reviewers:
+ ananta@chromium.org

[bokan, 2016-08-09 20:38:51]

Hi ananta@,

It looks like you've touched this code quite a bit; please punt to someone else
if you're not the right reviewer here.

Does DesktopNativeWidgetTopLevelHandler get used for anything other than popups?
Can popups ever have a non-0 offset from the widget? Also, what's the best way
to test this? Sorry for all the questions, these are unfamiliar parts for me.

Thanks.

[bokan, 2016-08-09 21:01:31]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 21:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-09 21:24:05]

ananta@chromium.org changed reviewers:
+ sky@chromium.org

[ananta, 2016-08-09 21:24:07]

+sky

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:154:
top_level_widget_->SetBounds(window->GetBoundsInScreen());
The Widget::SetBounds should make it all the way to
HWNDMessageHandler::SetBounds, which should then resize the WindowTreeHost and
with it the underlying aura::Window? Please check why that is not working.

[commit-bot: I haz the power, 2016-08-09 21:51:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 21:51:13]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-08-11 17:04:08]

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:154:
top_level_widget_->SetBounds(window->GetBoundsInScreen());
On 2016/08/09 21:24:07, ananta wrote:
> The Widget::SetBounds should make it all the way to
> HWNDMessageHandler::SetBounds, which should then resize the WindowTreeHost and
> with it the underlying aura::Window? Please check why that is not working.

The aura::Window *is* resized, the problem is that its offset is set relative to
where it used to be. In the case of this bug its offset is set to (0, -20) since
the popup grows and needs to be repositioned. This causes us to call back in
here and set bounds on the top level widget which sees the -20 in the origin and
moves the widget up by 20 pixels on the screen. But the aura::Window is still
positioned at (0, -20) so the aura::Window is offset by -20 within the widget.

Are you saying there's already code that's supposed to deal with this? I'm
building on Windows now so I can step through HWNDMessageHandler but on Linux it
seems like everything assumes we should be keeping the bounds' origin.

[bokan, 2016-08-11 18:14:24]

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:154:
top_level_widget_->SetBounds(window->GetBoundsInScreen());
On 2016/08/11 17:04:08, bokan wrote:
> On 2016/08/09 21:24:07, ananta wrote:
> > The Widget::SetBounds should make it all the way to
> > HWNDMessageHandler::SetBounds, which should then resize the WindowTreeHost
and
> > with it the underlying aura::Window? Please check why that is not working.
> 
Stepping through on Win, we do get through to HWNDMessageHandler::SetBounds but
that only sets the bounds on the native window. It seems that the relationship
is the other way around, resizing/moving the aura::Window causes this callback
which resizes/moves the Widget/native window.

[bokan, 2016-08-15 05:18:58]

ping

[sky, 2016-08-15 15:01:30]

I'm assuming you're pinging Ananta, who had the most recent question. Can you
please add test coverage?

[sky, 2016-08-15 15:05:48]

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:159:
window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));
I'm nervous about changing the bounds of the same window that has it's bound
changing. That can easily lead to observers seeing the wrong thing.

This should be done using an aura::LayoutManager. Requests to change a windows
bounds flow through the layoutmanager, so it's the ideal place to change things
like this.

[bokan, 2016-08-18 22:51:06]

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:159:
window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));
On 2016/08/15 15:05:48, sky wrote:
> I'm nervous about changing the bounds of the same window that has it's bound
> changing. That can easily lead to observers seeing the wrong thing.
> 
> This should be done using an aura::LayoutManager. Requests to change a windows
> bounds flow through the layoutmanager, so it's the ideal place to change
things
> like this.

Thanks, I'll give that a try. Any advice on where a test for this might go
(sorry, I'm new in this area).

[sky, 2016-08-19 03:19:39]

That depends. If you can write the test entirely in terms of widget,
then in widget_test is good. OTOH if you need to poke at
DesktopNativeWidgetAura, then desktop_native_widget_aura_unittest.cc
is like the place.

On Thu, Aug 18, 2016 at 3:51 PM,  <bokan@chromium.org> wrote:
>
>
https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
>
>
https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:159:
> window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));
> On 2016/08/15 15:05:48, sky wrote:
>> I'm nervous about changing the bounds of the same window that has it's
> bound
>> changing. That can easily lead to observers seeing the wrong thing.
>>
>> This should be done using an aura::LayoutManager. Requests to change a
> windows
>> bounds flow through the layoutmanager, so it's the ideal place to
> change things
>> like this.
>
> Thanks, I'll give that a try. Any advice on where a test for this might
> go (sorry, I'm new in this area).
>
> https://codereview.chromium.org/2228093003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bokan, 2016-08-19 17:57:47]

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:159:
window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));
On 2016/08/15 15:05:48, sky wrote:
> I'm nervous about changing the bounds of the same window that has it's bound
> changing. That can easily lead to observers seeing the wrong thing.
> 
> This should be done using an aura::LayoutManager. Requests to change a windows
> bounds flow through the layoutmanager, so it's the ideal place to change
things
> like this.

I don't think this'll work since the flow through LayoutManager happens before
the NativeWidget gets the bounds from the Window. It seems like there's a
strange circular dependency here where the aura::Window location is relative to
the Widget but the Widget's location is dependent on the Window location.

Put another way, we want to move the popup up 20px so we do this:

1. RWHVA::SetBounds called with new screen coordinates
2. Call Window::SetBounds with parent-relative coordinates y: -20px
3. Call Layer::SetBounds
4. Calls Layer::SetBoundsFromAnimation
5. Calls Window::OnWindowBoundsChanged (this is where the Window::bounds_ is
actually set)
6. Calls Window observer DesktopNativeWidgetAura::OnWindowBoundsChanged
7. Calls Widget::SetBounds passing Window::GetBoundsInScreen which because of
the -20px will move Widget up by 20px on the screen but the Window is still
-20px relative to the Widget.

The LayoutManager path is inserted between steps 2 and 3 so clearing the
location coordinates there means the Widget wouldn't be moved. If the Widget
gets its location from the Window, then changing the Widget's location should
counter move the Window to keep it in the same screen location.

Perhaps the right thing is to not move the Window at all? Can we move the Widget
from the RWHVA::SetBounds and Window::SetBounds using only the size?

[sky, 2016-08-19 20:06:52]

On 2016/08/19 17:57:47, bokan wrote:
>
https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/2228093003/diff/1/ui/views/widget/desktop_aur...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:159:
> window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));
> On 2016/08/15 15:05:48, sky wrote:
> > I'm nervous about changing the bounds of the same window that has it's bound
> > changing. That can easily lead to observers seeing the wrong thing.
> > 
> > This should be done using an aura::LayoutManager. Requests to change a
windows
> > bounds flow through the layoutmanager, so it's the ideal place to change
> things
> > like this.
> 
> I don't think this'll work since the flow through LayoutManager happens before
> the NativeWidget gets the bounds from the Window.

If you make DesktopNativeWidgetTopLevelHandler a LayoutManager (or have a
LayoutManager), DesktopNativeWidgetTopLevelHandler should no longer need to be a
WindowObserver. What is in OnWindowBoundsChanged() should move to the
layoutmanager. You'll likely need to track when the bounds change should be
processed, and when it should be mapped to a change in size/bounds of the
widget.

 It seems like there's a
> strange circular dependency here where the aura::Window location is relative
to
> the Widget but the Widget's location is dependent on the Window location.
> 
> Put another way, we want to move the popup up 20px so we do this:
> 
> 1. RWHVA::SetBounds called with new screen coordinates
> 2. Call Window::SetBounds with parent-relative coordinates y: -20px
> 3. Call Layer::SetBounds
> 4. Calls Layer::SetBoundsFromAnimation
> 5. Calls Window::OnWindowBoundsChanged (this is where the Window::bounds_ is
> actually set)
> 6. Calls Window observer DesktopNativeWidgetAura::OnWindowBoundsChanged
> 7. Calls Widget::SetBounds passing Window::GetBoundsInScreen which because of
> the -20px will move Widget up by 20px on the screen but the Window is still
> -20px relative to the Widget.
> 
> The LayoutManager path is inserted between steps 2 and 3 so clearing the
> location coordinates there means the Widget wouldn't be moved. If the Widget
> gets its location from the Window, then changing the Widget's location should
> counter move the Window to keep it in the same screen location.
> 
> Perhaps the right thing is to not move the Window at all? Can we move the
Widget
> from the RWHVA::SetBounds and Window::SetBounds using only the size?