I've been having a discussion with Neil Deakin about this bug that I'm experiencing. His debugging led to this explanation: ...two mousemove events are being fired. One is fired on the coloured square with the right coordinates, followed by a synthetic mousemove event with coordinates that are off by the height of the titlebar which ends up firing on the numbered labels below. I've attached a patch which puts a simple set of markup/CSS (with :hover rules) into an XUL popup (like used for devtool panels), which illustrates the problem, which is that flickering occurs with the :hover rules. Also, here's a standalone test URL with the same markup/CSS (but obviously not in an XUL popup), and it works fine (no flickering): http://test.getify.com/test-nested-css-hover-2.html

So this testcase should reproduce the problem, correct?

No, the panel needs a titlebar: <panel noautohide="true" titlebar="normal"/>

Also, note, this problem happens even if the markup/CSS is not in an iframe and is just directly in the popup/panel's markup. Dunno if that implies different code paths than for handling a child iframe, but... Another thing to note... not sure here (being unfamiliar with XUL), but the <iframe> you have there, I have a suspicion that it needs to be in the XHTML namespace rather than in the XML namespace. At least, that was true when I was creating the iframe programmatically, that I had to use `document.createElementNS(...)`. But again, I'm not entirely sure whether that's relevant or not, just figured I'd mention it.

(In reply to comment #2) > No, the panel needs a titlebar: > > <panel noautohide="true" titlebar="normal"/> I tried adding those two attributes but the panel still doesn't have a titlebar. (I'm loading my testcase as a content document in Firefox that allows XUL via the pref override.)

You'll need to open it in a chrome window. I just did debugging by applying the patch, set the preference 'devtools.inspector.enabled' to true, and use open the Inspector from the Tools menu.

Pasting this js into the error console I'm able to reproduce the problem on Windows (but not Linux).

So the problem is that we expand the view bounds for the popup to include the titlebar and other chrome that the OS adds to the popup window. This is unexpected as usually the view bounds represent what we are responsible for painting and handling events on, not the parts the OS is responsible for. We just need to teach views to handle the case where their widget's bounds are a different size from their bounds, which shouldn't be too hard.

tn, would you mind posting a roll up patch merged to tip?

Here is a rollup patch of all the patches. All the patches still applied cleanly so no merging was needed.

Comment on attachment 554537 [details] [diff] [review] Part 4. Add an API to widgets for resizing/moving the client area. Review of attachment 554537 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me. Technically though roc needs to approve base widget / interface changes.

Comment on attachment 554537 [details] [diff] [review] Part 4. Add an API to widgets for resizing/moving the client area. Review of attachment 554537 [details] [diff] [review]: ----------------------------------------------------------------- You somehow want to make it clear in the documentation that these methods resize/move the entire widget, it's just that the coordinates are provided for the client area instead of the window frame.

Comment on attachment 554535 [details] [diff] [review] Part 2. When placing popup widgets check if the client offset of the window has changed in addition to the top left of the window. > PRInt32 mXPos; > PRInt32 mYPos; > PRInt32 mScreenXPos; > PRInt32 mScreenYPos; >+ // The value of the client offset of our widget the last time we positioned >+ // outselves. We store this so that we can detect when it changes but the >+ // position of our widget didn't change. outselves -> ourselves

Comment on attachment 554538 [details] [diff] [review] Part 5. Make the view bounds of a popup coincide with the client area of the widget. The nsMenuPopupFrame changes look ok. You should probably have roc review the view changes, although they seem ok to me.

(In reply to Neil Deakin from comment #20) > outselves -> ourselves Fixed. Thanks.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > Comment on attachment 554537 [details] [diff] [review] > Part 4. Add an API to widgets for resizing/moving the client area. > > Review of attachment 554537 [details] [diff] [review]: > ----------------------------------------------------------------- > > You somehow want to make it clear in the documentation that these methods > resize/move the entire widget, it's just that the coordinates are provided > for the client area instead of the window frame. This is what I changed them to: MoveClient - "Reposition this widget so that the client area has the given offset." ResizeClient(width,height) - "Resize the widget so that the inner client area has the given size." ResizeClient(x,y,width,height) - "Resize and reposition the widget so tht inner client area has the given offset and size.:

Comment on attachment 554539 [details] [diff] [review] Part 6. Implement nsIWidget::GetClientOffset on GTK2. >+#if GTK_CHECK_VERSION(2,0,0) >+ GdkAtom cardinal_atom = gdk_x11_xatom_to_atom(XA_CARDINAL); >+#else >+ GdkAtom cardinal_atom = (GdkAtom) XA_CARDINAL; >+#endif I think you only need the first form (with no GTK_CHECK_VERSION) and that is correct, though the second may happen to also work. But fill me in if there's a reason for two forms here.

Comment on attachment 554540 [details] [diff] [review] Part 7. With a proper implementation of GetClientOffset on GTK2 popups expect the coordinates of their move events to be the top left of the outer window like all other platforms now. > // This is wrong, but noautohide titlebar panels currently depend on it > // (bug 601545#c13). mBounds.TopLeft() should refer to the > // window-manager frame top-left, but WidgetToScreenOffset() gives the > // client window origin. > mBounds.MoveTo(WidgetToScreenOffset()); > > nsGUIEvent event(PR_TRUE, NS_MOVE, this); > >- event.refPoint = mBounds.TopLeft(); >+ // Popup windows expect the move event to send coordinates of the outer top >+ // left of the window. >+ nsIntRect screenBounds; >+ GetScreenBounds(screenBounds); >+ event.refPoint = screenBounds.TopLeft(); With this change in the NS_MOVE event, can mBounds now be (correctly) set to screenBounds?

Comment on attachment 554541 [details] [diff] [review] Part 8. gdk_window_get_origin isn't always in sync with gdk_window_get_root_origin so don't use it when that might cause problems. >+ // We don't use gdk_window_get_origin here because sometimes it is not >+ // in sync with gdk_window_get_root_origin and sometimes we need it to >+ // be. Where do we need this to be in sync?

(In reply to Karl Tomlinson (:karlt) from comment #24) > Part 6. Implement nsIWidget::GetClientOffset on GTK2. > > I think you only need the first form (with no GTK_CHECK_VERSION) and that is > correct, though the second may happen to also work. > But fill me in if there's a reason for two forms here. I just copied other code in the codebase doing the same thing.

(In reply to Karl Tomlinson (:karlt) from comment #25) > With this change in the NS_MOVE event, can mBounds now be (correctly) set to > screenBounds? I think we still fail some tests if we do that, but I can verify that.

(In reply to Timothy Nikkel (:tn) from comment #27) > I just copied other code in the codebase doing the same thing. Ah, nsScreenGtk. Yes, don't copy that and just use gdk_x11_xatom_to_atom, please. (In reply to Timothy Nikkel (:tn) from comment #28) > (In reply to Karl Tomlinson (:karlt) from comment #25) > > With this change in the NS_MOVE event, can mBounds now be (correctly) set to > > screenBounds? > > I think we still fail some tests if we do that, but I can verify that. Yes, please try that. My understanding was that the mBounds code there was only incorrect because the NS_MOVE event had the wrong coords, and I'm keen to ensure that an NS_MOVE event does not trigger another move request. (Bug 601545 comment 13)

(In reply to Karl Tomlinson (:karlt) from comment #29) > (In reply to Timothy Nikkel (:tn) from comment #28) > > (In reply to Karl Tomlinson (:karlt) from comment #25) > > > With this change in the NS_MOVE event, can mBounds now be (correctly) set to > > > screenBounds? > > > > I think we still fail some tests if we do that, but I can verify that. > > Yes, please try that. My understanding was that the mBounds code there was > only incorrect because the NS_MOVE event had the wrong coords, and I'm keen > to ensure that an NS_MOVE event does not trigger another move request. > (Bug 601545 comment 13) So I tried just making GetScreenBounds return the correct (outer) bounds and this causes at least half a dozen or so tests to fail (https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1c4a0cf111ad). (I imagine making mBounds correct would also entail making GetScreenBounds correct.)

(In reply to Karl Tomlinson (:karlt) from comment #25) > > // This is wrong, but noautohide titlebar panels currently depend on it > > // (bug 601545#c13). mBounds.TopLeft() should refer to the > > // window-manager frame top-left, but WidgetToScreenOffset() gives the > > // client window origin. > > mBounds.MoveTo(WidgetToScreenOffset()); > > > > nsGUIEvent event(PR_TRUE, NS_MOVE, this); > > > >- event.refPoint = mBounds.TopLeft(); > >+ // Popup windows expect the move event to send coordinates of the outer top > >+ // left of the window. > >+ nsIntRect screenBounds; > >+ GetScreenBounds(screenBounds); > >+ event.refPoint = screenBounds.TopLeft(); > > With this change in the NS_MOVE event, can mBounds now be (correctly) set to > screenBounds? Sorry I wasn't clear here. What I meant was: can the mBounds.MoveTo(WidgetToScreenOffset()) code here be changed to mBounds.MoveTo(screenBounds.TopLeft())?

Comment on attachment 554540 [details] [diff] [review] Part 7. With a proper implementation of GetClientOffset on GTK2 popups expect the coordinates of their move events to be the top left of the outer window like all other platforms now. r- because AIUI mBounds.x/y need to be consistent with the NS_MOVE coords.

Comment on attachment 554541 [details] [diff] [review] Part 8. gdk_window_get_origin isn't always in sync with gdk_window_get_root_origin so don't use it when that might cause problems. Removing review request pending answer to comment 26.

Sorry that I haven't been responsive. I'll try to come up with answers.

(In reply to Karl Tomlinson (:karlt) from comment #32) > Comment on attachment 554540 [details] [diff] [review] [diff] [details] [review] > Part 7. With a proper implementation of GetClientOffset on GTK2 popups > expect the coordinates of their move events to be the top left of the outer > window like all other platforms now. > > r- because AIUI mBounds.x/y need to be consistent with the NS_MOVE coords. The only place that uses the coordinates of NS_MOVE events is nsXULPopupManager::PopupMoved and this patch queue changes that path to expect it this way.

Comment on attachment 554541 [details] [diff] [review] Part 8. gdk_window_get_origin isn't always in sync with gdk_window_get_root_origin so don't use it when that might cause problems. We don't need this anymore. Not sure why.

This makes the topleft of the bounds match the refpoint of the event we dispatch and also guards the check_for_rollup call.

Now that mBounds doesn't hold the client bounds we need to actually implement GetClientBounds so it returns the right value.

Comment on attachment 578140 [details] [diff] [review] Part 7 v4. With a proper implementation of GetClientOffset on GTK2 popups expect the coordinates of their move events to be the top left of the outer window like all other platforms now. Thank you for tidying all this up!

Pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/452f27a6ecd5 https://hg.mozilla.org/integration/mozilla-inbound/rev/42ded0d67419 https://hg.mozilla.org/integration/mozilla-inbound/rev/70bc06b28e60 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e487e82702 https://hg.mozilla.org/integration/mozilla-inbound/rev/977209386a0c https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7d48de0799 https://hg.mozilla.org/integration/mozilla-inbound/rev/35d64d297f08 https://hg.mozilla.org/integration/mozilla-inbound/rev/30ad10cf30e0

https://hg.mozilla.org/mozilla-central/rev/452f27a6ecd5 https://hg.mozilla.org/mozilla-central/rev/42ded0d67419 https://hg.mozilla.org/mozilla-central/rev/70bc06b28e60 https://hg.mozilla.org/mozilla-central/rev/a5e487e82702 https://hg.mozilla.org/mozilla-central/rev/977209386a0c https://hg.mozilla.org/mozilla-central/rev/fe7d48de0799 https://hg.mozilla.org/mozilla-central/rev/35d64d297f08 https://hg.mozilla.org/mozilla-central/rev/30ad10cf30e0