Closed Bug 601545 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- -

People

(Reporter: dholbert, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(4 files)

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.
> 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
Might also have been caused by bug 412773...
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
blocking2.0: --- → ?
Is this with compiz?
I don't seem to be able to reproduce with kwin.
Can you check metacity, please?
I can reproduce both with & without compiz.  (Restarted firefox in between switching window managers, just in case)
That is to say -- I can reproduce this with Compiz & also with Metacity.
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.)
(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.
Running with --sync makes all requests synchronous.
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.
Summary: Clear Recent History dialog jumps downwards each time details panel is opened/closed → Clear Recent History dialog jumps downwards and to the right each time details panel is opened/closed
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.
Assignee: nobody → karlt
Component: XP Toolkit/Widgets: XUL → Widget: Gtk
QA Contact: xptoolkit.xul → gtk
blocking2.0: ? → final+
(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.
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.
Attached patch mochitestSplinter Review
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.
Attachment #498912 - Flags: review?(roc)
Currently things are confusing with 3 variables mNeedsResize, mNeedsMove, and mPlaced having overlapping meanings.  This patch reduces this to only mNeedsResize and mNeedsMove.
Attachment #498914 - Flags: review?(roc)
This is a documentation-only change to clarify some of the confusion leading to the issue described in comment 12.
Attachment #498919 - Flags: review?(roc)
Blocks: 615805
You need to log in before you can comment on or make changes to this bug.