615805 - Resizing Inspect Network Request window causes window to move

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Will Roberts [:bws42]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre

Trying to resize the window by dragging the dotted triangle region in the bottom right corner causes the window to move diagonally towards that corner then away from that corner. Grabbing the edge of the window chrome causes the resize to occur normally.

Also the window will occasionally flash white when resizing regardless of which method is used, and the window chrome never indicates that the window is active; it always looks like the main Firefox window has focus.

Reproducible: Always

Comment 1

[Mihai Sucan [:msucan]]

I can confirm this issue, and I did notice it before. It's not a developer tools (Web Console) bug per se, I believe it's a more general bug with xul:panels.

Neil: can you please point me where I can find the implementation of the resize grip of the xul:panel? (the one shown on white, on the bottom right) I have looked through the C++ code and it seems the depth there... is kind of deep. :) Note that resizing the panel by its edges works - only the grip seems to be broken.

Thanks!

Comment 2

[Neil Deakin]

This is done in nsResizerFrame.cpp

Specifically, http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsResizerFrame.cpp#170 is where mouse dragging is handled, and more specifically you should see some code blocks that handle window movement and some that handle popup dragging.

Note that the size is actually just adjusted by setting the width and height attributes.

Comment 3

[Will Roberts [:bws42]]

(In reply to comment #1)
> I can confirm this issue, and I did notice it before. It's not a developer
> tools (Web Console) bug per se, I believe it's a more general bug with
> xul:panels.

Fair enough, I actually can't find any other windows in Firefox that have that grippy inside the window frame. That's the same mechanism that allows me to resize this (the one I'm typing in) entry area right? Could you just disable the ability to resize the network request widget?

Comment 4

[Mihai Sucan [:msucan]]

(In reply to comment #3)
> (In reply to comment #1)
> > I can confirm this issue, and I did notice it before. It's not a developer
> > tools (Web Console) bug per se, I believe it's a more general bug with
> > xul:panels.
> 
> Fair enough, I actually can't find any other windows in Firefox that have that
> grippy inside the window frame. That's the same mechanism that allows me to
> resize this (the one I'm typing in) entry area right? Could you just disable
> the ability to resize the network request widget?

I'm looking into fixing the actual issue.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed fix

This is the proposed patch, which fixes the issue reported.

Problem (as I understand it): the nsResizerFrame code properly resizes the nsMenuPopupFrame (the xul:panel), but while it does so, a PopupMoved event is continuously fired. This event always has a small difference for the popup location. The resizer detects the menuPopupFrame changed its location and resets it. This creates the continuous back-and-forth jump.

The jump is caused by the call to nsWindow::NativeResize() which calls gtk_window_move() with the same coordinates - practically, it does not move the xul:panel popup.

Whenever a move/resize occurs, the GdkEventConfigure event is fired from the lower levels. The nsWindow::OnConfigureEvent() event handler takes the window coordinates (x and y), which based on GTK docs, do not include the window decorations. For top level windows, the method calls WidgetToScreenOffset() to determine the right bounds. This latter method uses gdk_window_get_origin() to determine the window origin.

For xul:panel popups the window origin returned by gdk_window_get_origin() is still the same as from GdkEventConfigure, which differs from the coordinates we used when we invoked gtk_window_move(). The nsXULPopupManager::PopupMoved() event handler updates the location of the popup accordingly - with the "new" coordinates.

The fix here changes the WidgetToScreenOffset() method to determine the window coordinates together with the window decorations, using the gtk_window_get_position() when mShell is a GTK window. According to the GTK docs, gtk_window_get_position() makes best efforts to provide us with the coordinates that include the window decorations.

With this change, the xul:panel resizer behaves correctly. Additionally, whenever one manually changes the xul:panel width and height attributes, the panel no longer jumps around.

Another fix included in this patch: whenever the user does change only the width of the xul:panel (by dragging the left/right edges), the xul:panel content does not reflow/update. The change in nsXULPopupManager::PopupResized() fixes the problem of reflowing, by notifying the width attribute change as well.

I hope this is fine. Feedback for improvements is welcome!

Comment 6

[Neil Deakin]

Karl is a better reviewer for this patch.

I'm wary of changing the WidgetToScreenOffset function without a lot of testing. It was last changed by bug 552982.

Comment 7

[Neil Deakin]

I tested this patch and the mouse event coordinates (client and screen) are incorrect when over a popup with a titlebar. They are offset by the titlebar size.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Karl is a better reviewer for this patch.

Thanks! Will change the review request.

> I'm wary of changing the WidgetToScreenOffset function without a lot of
> testing. It was last changed by bug 552982.

Agreed. I saw the function is used a lot, and there can be regressions.

(In reply to comment #7)
> I tested this patch and the mouse event coordinates (client and screen) are
> incorrect when over a popup with a titlebar. They are offset by the titlebar
> size.

Haven't noticed that issue, but I'll check. Where do I find a popup with a titlebar and a tooltip? (I played with the xul:panel popups from the Web Console - network panels).

In my testing, tooltips in menus (the one button menu) show up correctly positioned. (menus are also popups, afaik)

Is there a way to make this change only for the purpose of xul:panel popups?

Also, from studying the code, it wasn't very clear if nsXULPopupManager::PopupMoved() (and friends) expect the coordinates to be with or without window decorations. The fix I did, makes resizing work correctly for xul:panels by including the window decorations.

From what I understood, in nsWindow::OnConfigureEvent() for top level windows there's a very specific call to WidgetToScreenOffset() to get "the right coordinates". It seems to suggest that the coords need to include window decorations.

Thanks for your comments, and for looking into the patch!

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 498227 [details] [diff] [review]
proposed fix

Changing review request, as suggested by Neil.

Please see comment #5. Also, please note that the mochitest uses a timeout because the panel location is updated asynchronously and there wasn't any obvious event to listen to. If you know any event I could use, please let me know. (I tried DOMAttrModified and resize, none worked.)

Comment 10

[Mihai Sucan [:msucan]]

Having looked into this more: another possible fix would be to use if (mIsTopLevel && mContainer) { gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window, &x, &y) } in WidgetToScreenOffset(), instead of the gtk_window_get_position() function I used in the proposed patch. It looks like that would work fine - in my testing, it does.

Neil: can you test if that would change anything for you? With the tooltips. Thanks!

With this code I ran all mochitests and they all pass, no failures here.

Comment 11

[Neil Deakin]

Attachment: testcase

(In reply to comment #10)
> Neil: can you test if that would change anything for you? With the tooltips.
> Thanks!

That change makes the coordinate issue I mentioned worse. Also, with this patch, the popup doesn't appear in the correct position.

Attached is a testcase that can be used to show the correct behaviour. Pressing the button should open the panel with a titlebar just underneath the button, offset horizontally by 30 pixels.

Moving the mouse should update the coordinates. The coordinates shouldn't jump when moving the mouse from over the window and the popup and vice versa.

It would be good to talk to Karl as we had a discussion about this a while back. I think he knows what this function should be doing.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to comment #11)
> Created attachment 498329 [details]
> testcase
> 
> (In reply to comment #10)
> > Neil: can you test if that would change anything for you? With the tooltips.
> > Thanks!
> 
> That change makes the coordinate issue I mentioned worse. Also, with this
> patch, the popup doesn't appear in the correct position.
> 
> Attached is a testcase that can be used to show the correct behaviour. Pressing
> the button should open the panel with a titlebar just underneath the button,
> offset horizontally by 30 pixels.
> 
> Moving the mouse should update the coordinates. The coordinates shouldn't jump
> when moving the mouse from over the window and the popup and vice versa.
> 
> It would be good to talk to Karl as we had a discussion about this a while
> back. I think he knows what this function should be doing.

Just tested the proposed patch with the TC you provided. It works as you expect it to. The coordinates do not jump when I move into/out of the popup. The popup is correctly placed beneath the button, 30px to the right.

Also, the suggestion above (to use mContainer) also works fine. Both cases behave the same, as without any patching to the code - no noticeable regression here.

One tiny problem I see: the popup panel does not have the titlebar visible, for some reason. This happens in all of the three cases mentioned above (proposed patch, proposed alternative and no patch).

What are you testing on? I test on Ubuntu 10.04, Gnome 2.30.2, Metacity, Compiz disabled.

Thanks for your testcase!

Comment 13

[Neil Deakin]

(In reply to comment #12)
> One tiny problem I see: the popup panel does not have the titlebar visible, for
> some reason.

That would be why the coordinates look correct to you.

You are running the test as a new chrome window?

Comment 14

[Mihai Sucan [:msucan]]

(In reply to comment #13)
> (In reply to comment #12)
> > One tiny problem I see: the popup panel does not have the titlebar visible, for
> > some reason.
> 
> That would be why the coordinates look correct to you.
> 
> You are running the test as a new chrome window?

I use a mochitest to open the XUL you attached:

  let chromeroot = getRootDirectory(gTestPath);
  let uri = chromeroot + "browser_bug615805_enn.xul";
  let win = openDialog(location, "_blank", "chrome,all,dialog=no,left=200,top=200,width=300,height=300", uri);

Any change I should do to make the panel titlebar show?

Comment 15

[Neil Deakin]

That doesn't open the page as a chrome window. You need to put the uri as the first argument to openDialog.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to comment #5)

Thanks for the analysis, Mihai.

See also bug 601545 comment 12 and 13 for similar issues.
I think the core issue is the menu code moving a widget in response to NS_MOVE events.
Perhaps (it got complicated and I can't recall for sure) things would be better if the confusion between decoration top-left and client window top-left could be resolved.

> The jump is caused by the call to nsWindow::NativeResize() which calls
> gtk_window_move() with the same coordinates - practically, it does not move the
> xul:panel popup.

The patch in bug 601545 may avoid the gtk_window_move() and cover over the other issues here.

> Whenever a move/resize occurs, the GdkEventConfigure event is fired from the
> lower levels. The nsWindow::OnConfigureEvent() event handler takes the window
> coordinates (x and y), which based on GTK docs, do not include the window
> decorations. For top level windows, the method calls WidgetToScreenOffset() to
> determine the right bounds. This latter method uses gdk_window_get_origin() to
> determine the window origin.

I suspect WidgetToScreenOffset() is not (in general) the right method to use in OnConfigureEvent().

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 498227 [details] [diff] [review]
proposed fix

> nsWindow::WidgetToScreenOffset()
> {
>     gint x = 0, y = 0;
> 
>-    if (mGdkWindow) {
>+    if (GTK_IS_WINDOW(mShell)) {
>+        gtk_window_get_position(GTK_WINDOW(mShell), &x, &y);
>+    }
>+    else if (mGdkWindow) {

WidgetToScreenOffset() is used to interpret mouse positions which are in client (not frame/decoration) window coordinates, which is why this change leads to the mouse event problems that Neil mentioned.

>-    popup->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);
>+    popup->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_TRUE);
>     popup->SetAttr(kNameSpaceID_None, nsGkAtoms::height, height, PR_TRUE);

I'm guessing this is correct, but I haven't checked the history.
This change deserves at least its own patch and probably its own bug.
Neil would be a better reviewer for this.

Comment 18

[Mihai Sucan [:msucan]]

Thanks for your comments and review, Neil and Karl.

Indeed, there's quite some confusion in the code related to the coordinates, which made it more complicated to me given I'm new to the codebase. I changed the WidgetToScreenOffset() believing it should include window decorations.

Bug 601545 describes the same problem, but in a different use case. The fix also made xul:panel resizing work fine now. So this bug can now be marked as fixed.

Still, the real problem in the code, I believe, is not fixed. The patch that was checked-in avoids the problem by not performing window moves during resizes - a problem which I did notice when analyzing the source code and debugging.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to comment #18)
> Indeed, there's quite some confusion in the code related to the coordinates,
> which made it more complicated to me given I'm new to the codebase.

Hey, it was complicated for me and I'm not new.

> Bug 601545 describes the same problem, but in a different use case. The fix
> also made xul:panel resizing work fine now. So this bug can now be marked as
> fixed.

OK.  I'll mark this fixed based on the reported symptoms.

> Still, the real problem in the code, I believe, is not fixed.

Agreed.