601545 - Clear Recent History dialog jumps downwards and to the right each time details panel is opened/closed

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Ctrl+Shift+Del  (or Tools | Clear Recent History)
 2. Click the arrow next to "Details" to toggle details open/closed.
 3. Repeat step 2.

ACTUAL RESULTS: Each time I toggle "Details", the dialog moves downwards by about the height of the titlebar.

REGRESSION RANGE:
Works:   20100727030231 0ed73d56efd5
Broken:  20100728030233 beab39d49de9http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0ed73d56efd5&tochange=beab39d49de9
From that range, bug 552982 strikes me as most likely to be able to have caused this.  Tentatively marking as blocking that bug, and filing this in that bug's component.

Comment 1

[Daniel Holbert [:dholbert]]

> REGRESSION RANGE:
> Works:   20100727030231 0ed73d56efd5
> Broken:  20100728030233
> beab39d49de9http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0ed73d56efd5&tochange=beab39d49de9

[Darn it... In the textbox where I typed that comment, there was definitely no newline before "beab...", and there *was* a newline before the "http://hg.mozilla...", but they got stripped in the posting somehow.]

That should have appeared like so:
Works:   20100727030231 0ed73d56efd5
Broken:  20100728030233 beab39d49de9
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0ed73d56efd5&tochange=beab39d49de9

Comment 2

[Daniel Holbert [:dholbert]]

Might also have been caused by bug 412773...

Comment 3

[Daniel Holbert [:dholbert]]

Nope, I was right in comment 0 -- this is from bug 552982.

I confirmed through targeted local builds that the first affected changeset is:
 http://hg.mozilla.org/mozilla-central/rev/60fa04517a6f

Comment 4

[Karl Tomlinson (:karlt)]

Is this with compiz?
I don't seem to be able to reproduce with kwin.
Can you check metacity, please?

Comment 5

[Daniel Holbert [:dholbert]]

I can reproduce both with & without compiz.  (Restarted firefox in between switching window managers, just in case)

Comment 6

[Daniel Holbert [:dholbert]]

That is to say -- I can reproduce this with Compiz & also with Metacity.

Comment 7

[Daniel Holbert [:dholbert]]

Did a little GDB experimentation (with desktop effects disabled - i.e. using Metacity).

After toggling the arrow, I get a series calls to nsWindow::GetScreenBounds(), the fourth of which is the interesting one.  All of them (up to that point) take the gdk_window_get_root_origin() path, and here's what gdk_window_get_root_origin() does in each case...
 - The first returns with:  x=202 y=486
 - The second has a different |this|, and returns: x=0 y=0
 - The third returns with:  x=202 y=486
 - The fourth returns with: x=203 y=513

Also: the fourth "gdk_window_get_root_origin" call is what synchronously moves the dialog.  (That is: I'm stepping through GDB, with no paints happening in Firefox.  When I step over that gdk_window_get_root_origin() line, the dialog snaps to its new position.)

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #7)
> Also: the fourth "gdk_window_get_root_origin" call is what synchronously moves
> the dialog.  (That is: I'm stepping through GDB, with no paints happening in
> Firefox.  When I step over that gdk_window_get_root_origin() line, the dialog
> snaps to its new position.)

I'm guessing what's happening here is that a previous async move request is being flushed when gdk_window_get_root_origin() makes a synchronous request.
i.e. it could well be an earlier operation that moved the window.

Comment 9

[Karl Tomlinson (:karlt)]

Running with --sync makes all requests synchronous.

Comment 10

[Karl Tomlinson (:karlt)]

I can reproduce with metacity.
Once it reaches the bottom of the screen it no longer jumps down even when shrinking, but continues to jump right until it reaches the right-hand edge.

If moved to overlap either border of the screen it continues to move in both directions.

Comment 11

[Karl Tomlinson (:karlt)]

There seems to be general confusion in the GTK port about whether mBounds is
client coordinates or window-manager frame coordinates.

Part of this confusion may be due to the different interpretation of position
and size in move/resize requests: positions are window-manager frame origins,
but sizes are client dimensions.

http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.5

In http://hg.mozilla.org/mozilla-central/rev/60fa04517a6f

the origin of mBounds was changed to that of the client window (which for
non-popup-with-titlebar windows was a result of my review request) for
consistency with other platforms (bug 552982 comment 41), but that is not what
is wanted for gtk_window_move.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)
> In http://hg.mozilla.org/mozilla-central/rev/60fa04517a6f
> 
> the origin of mBounds was changed to that of the client window (which for
> non-popup-with-titlebar windows was a result of my review request) for
> consistency with other platforms (bug 552982 comment 41) ...

I don't think that was making the behaviour consistent with other platforms.
nsView::DoResetWidgetBounds uses nsIWidget::GetBounds to obtain the current position for comparing directly with new bounds passed to nsIWidget::Move.

AFAICT nsGlobalWindow and nsXULWindow use the same coordinates for nsIWidget::Move as come from GetScreenBounds, so Move must take the frame top-left, and so GetBounds should return the frame top-left.

Comment 13

[Karl Tomlinson (:karlt)]

It took me a little while to understand why (noautohide) panels with titlebars
did not respond well to making GetBounds return the window-frame position.

When an NS_MOVE event is dispatched, nsXULPopupManager::PopupMoved responds by
moving the box frame and view.  The key thing preventing an infinite loop
is that moving the view does not move the widget when widget->GetBounds
returns the same position as the new coordinates.

That doesn't work out for context menus because
nsMenuPopupFrame::SetPopupPosition adds offsetForContextMenu to the
coordinates from the NS_MOVE event when calculating the new position for the
view.  This would cause the context menu to always iterate ad infinitum towards the bottom right of the screen, except that NS_MOVE events are not dispatched for context menus.  This is due to three conditions being satisfied:

1) When OnConfigureEvent does not dispatch NS_MOVE events if the position in
   the event matches the position recorded in mBounds from the last Move or
   Create.

2) We must be either only positioning context menus once or always receiving
   the configure event for the first position before changing the position.
   If the ConfigureNotify event for the first position of the window were
   received after the window was moved, the context menu would be moved back
   to the first position + offsetForContextMenu, with potential subsequent
   iterations.

3) The context menu is an override-redirect window so the window manager will
   not move the window.  All moves are at Gecko's direction.

Things are even more snazzy for noautohide titlebar panels.  When a
ConfigureNotify event is received for these (after the window manager has
positioned the window),

1. mBounds is changed to point to the client window (not frame) top-left.

2. The NS_MOVE event is dispatched with client top-left coords.

3. The view top-left is aligned with these client window coords.

4. The view tries to move the widget frame top-left to the current client
   window coords.  This would move the window right and down but for
   window->GetBounds now lying to say that it's frame top-left has already
   moved.

5. The box frame is positioned in the client window coords indicated by the
   NS_MOVE event.  This is why the NS_MOVE event is necessary for these
   popups.

I think we can implement something for GetClientOffset() (used for step 5, but
currently returns zero), and so hopefully the NS_MOVE event can carry the
window-frame top-left and the widget does not need to lie about its position
to give the box frame its appropriate offset.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: mochitest

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: don't move windows during resize

The patches I'm attaching are as far as I had got before this bug was removed from the blocking2.0 list.  They do not address the core issues described in comment 12 and 13 but I think this is the right thing to do and it remedies the reported symptoms.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: use mNeedsResize/mNeedsMove on toplevel windows to make mPlaced unnecessary

Currently things are confusing with 3 variables mNeedsResize, mNeedsMove, and mPlaced having overlapping meanings.  This patch reduces this to only mNeedsResize and mNeedsMove.

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: clarify the meaning of coordinates passed to nsIWidget::Move

This is a documentation-only change to clarify some of the confusion leading to the issue described in comment 12.

Comment 18

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/ec338170e5f7
http://hg.mozilla.org/mozilla-central/rev/71d2d1c6d12d
http://hg.mozilla.org/mozilla-central/rev/944010571e9d
http://hg.mozilla.org/mozilla-central/rev/0489b54d5ce8