Last Comment Bug 668437 - mouse events for :hover in XUL popup firing incorrectly
: mouse events for :hover in XUL popup firing incorrectly
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Timothy Nikkel (:tnikkel)
:
Mentors:
Depends on: 707814 1077652
Blocks: 659710
  Show dependency treegraph
 
Reported: 2011-06-29 20:58 PDT by Kyle Simpson
Modified: 2014-11-03 09:14 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (against the devtools branch) for illustrating bug with CSS :hover rules in an XUL popup (8.73 KB, text/plain)
2011-06-29 20:58 PDT, Kyle Simpson
no flags Details
testcase (461 bytes, application/vnd.mozilla.xul+xml)
2011-06-29 22:33 PDT, Timothy Nikkel (:tnikkel)
no flags Details
testcase (1010 bytes, text/plain)
2011-07-04 12:05 PDT, Timothy Nikkel (:tnikkel)
no flags Details
Part 1. DeCOMtaminate nsMenuPopupFrame::GetWidget. (7.26 KB, patch)
2011-08-19 13:56 PDT, Timothy Nikkel (:tnikkel)
enndeakin: review+
Details | Diff | Splinter 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. (5.79 KB, patch)
2011-08-19 13:57 PDT, Timothy Nikkel (:tnikkel)
enndeakin: review+
Details | Diff | Splinter Review
Part 3. Make the client bounds of a widget be relative to its parent in all cases. (4.51 KB, patch)
2011-08-19 13:59 PDT, Timothy Nikkel (:tnikkel)
jmathies: review+
Details | Diff | Splinter Review
Part 4. Add an API to widgets for resizing/moving the client area. (14.06 KB, patch)
2011-08-19 13:59 PDT, Timothy Nikkel (:tnikkel)
jmathies: review+
roc: superreview+
Details | Diff | Splinter Review
Part 5. Make the view bounds of a popup coincide with the client area of the widget. (5.35 KB, patch)
2011-08-19 14:00 PDT, Timothy Nikkel (:tnikkel)
enndeakin: review+
roc: review+
Details | Diff | Splinter Review
Part 6. Implement nsIWidget::GetClientOffset on GTK2. (3.15 KB, patch)
2011-08-19 14:01 PDT, Timothy Nikkel (:tnikkel)
karlt: review+
Details | Diff | Splinter 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. (1.28 KB, patch)
2011-08-19 14:02 PDT, Timothy Nikkel (:tnikkel)
karlt: review-
Details | Diff | Splinter 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. (1.66 KB, patch)
2011-08-19 14:02 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
rollup patch (39.04 KB, patch)
2011-08-22 10:16 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 7 v2. 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. (3.13 KB, patch)
2011-11-30 16:39 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 8. Implement nsIWidget::GetClientBounds on GTK2. (2.31 KB, patch)
2011-11-30 16:40 PST, Timothy Nikkel (:tnikkel)
karlt: review+
Details | Diff | Splinter Review
Part 7 v3. 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. (2.96 KB, patch)
2011-11-30 17:04 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter 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. (2.89 KB, patch)
2011-11-30 17:16 PST, Timothy Nikkel (:tnikkel)
karlt: review+
Details | Diff | Splinter Review

Description Kyle Simpson 2011-06-29 20:58:46 PDT
Created attachment 543050 [details]
patch (against the devtools branch) for illustrating bug with CSS :hover rules in an XUL popup

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
Comment 1 Timothy Nikkel (:tnikkel) 2011-06-29 22:33:19 PDT
Created attachment 543065 [details]
testcase

So this testcase should reproduce the problem, correct?
Comment 2 Neil Deakin 2011-06-30 02:04:10 PDT
No, the panel needs a titlebar:

<panel noautohide="true" titlebar="normal"/>
Comment 3 Kyle Simpson 2011-06-30 06:36:24 PDT
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.
Comment 4 Timothy Nikkel (:tnikkel) 2011-06-30 09:41:38 PDT
(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.)
Comment 5 Neil Deakin 2011-06-30 10:08:09 PDT
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.
Comment 6 Timothy Nikkel (:tnikkel) 2011-07-04 12:05:15 PDT
Created attachment 543806 [details]
testcase

Pasting this js into the error console I'm able to reproduce the problem on Windows (but not Linux).
Comment 7 Timothy Nikkel (:tnikkel) 2011-07-12 12:39:04 PDT
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.
Comment 8 Timothy Nikkel (:tnikkel) 2011-08-19 13:56:04 PDT
Created attachment 554533 [details] [diff] [review]
Part 1. DeCOMtaminate nsMenuPopupFrame::GetWidget.
Comment 9 Timothy Nikkel (:tnikkel) 2011-08-19 13:57:58 PDT
Created 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.
Comment 10 Timothy Nikkel (:tnikkel) 2011-08-19 13:59:09 PDT
Created attachment 554536 [details] [diff] [review]
Part 3. Make the client bounds of a widget be relative to its parent in all cases.
Comment 11 Timothy Nikkel (:tnikkel) 2011-08-19 13:59:45 PDT
Created attachment 554537 [details] [diff] [review]
Part 4. Add an API to widgets for resizing/moving the client area.
Comment 12 Timothy Nikkel (:tnikkel) 2011-08-19 14:00:41 PDT
Created attachment 554538 [details] [diff] [review]
Part 5. Make the view bounds of a popup coincide with the client area of the widget.
Comment 13 Timothy Nikkel (:tnikkel) 2011-08-19 14:01:32 PDT
Created attachment 554539 [details] [diff] [review]
Part 6. Implement nsIWidget::GetClientOffset on GTK2.
Comment 14 Timothy Nikkel (:tnikkel) 2011-08-19 14:02:02 PDT
Created 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.
Comment 15 Timothy Nikkel (:tnikkel) 2011-08-19 14:02:36 PDT
Created 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.
Comment 16 Jim Mathies [:jimm] 2011-08-22 06:45:00 PDT
tn, would you mind posting a roll up patch merged to tip?
Comment 17 Timothy Nikkel (:tnikkel) 2011-08-22 10:16:40 PDT
Created attachment 554896 [details] [diff] [review]
rollup patch

Here is a rollup patch of all the patches. All the patches still applied cleanly so no merging was needed.
Comment 18 Jim Mathies [:jimm] 2011-08-23 09:33:20 PDT
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 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 17:37:34 PDT
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 20 Neil Deakin 2011-08-28 16:58:25 PDT
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 21 Neil Deakin 2011-08-28 17:00:53 PDT
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.
Comment 22 Timothy Nikkel (:tnikkel) 2011-08-30 16:18:16 PDT
(In reply to Neil Deakin from comment #20)
> outselves -> ourselves

Fixed. Thanks.
Comment 23 Timothy Nikkel (:tnikkel) 2011-08-30 16:24:46 PDT
(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 24 Karl Tomlinson (:karlt) 2011-09-05 00:21:32 PDT
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 25 Karl Tomlinson (:karlt) 2011-09-05 00:36:05 PDT
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 26 Karl Tomlinson (:karlt) 2011-09-05 00:40:08 PDT
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?
Comment 27 Timothy Nikkel (:tnikkel) 2011-09-05 07:29:50 PDT
(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.
Comment 28 Timothy Nikkel (:tnikkel) 2011-09-05 07:32:26 PDT
(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.
Comment 29 Karl Tomlinson (:karlt) 2011-09-05 21:38:46 PDT
(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)
Comment 30 Timothy Nikkel (:tnikkel) 2011-09-07 22:19:15 PDT
(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.)
Comment 31 Karl Tomlinson (:karlt) 2011-09-08 00:56:20 PDT
(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 32 Karl Tomlinson (:karlt) 2011-09-28 20:05:18 PDT
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 33 Karl Tomlinson (:karlt) 2011-09-28 20:16:29 PDT
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.
Comment 34 Timothy Nikkel (:tnikkel) 2011-09-28 21:37:07 PDT
Sorry that I haven't been responsive. I'll try to come up with answers.
Comment 35 Timothy Nikkel (:tnikkel) 2011-10-06 13:17:33 PDT
(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 36 Timothy Nikkel (:tnikkel) 2011-11-30 16:37:16 PST
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.
Comment 37 Timothy Nikkel (:tnikkel) 2011-11-30 16:39:08 PST
Created attachment 578134 [details] [diff] [review]
Part 7 v2. 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 makes the topleft of the bounds match the refpoint of the event we dispatch and also guards the check_for_rollup call.
Comment 38 Timothy Nikkel (:tnikkel) 2011-11-30 16:40:22 PST
Created attachment 578135 [details] [diff] [review]
Part 8. Implement nsIWidget::GetClientBounds on GTK2.

Now that mBounds doesn't hold the client bounds we need to actually implement GetClientBounds so it returns the right value.
Comment 39 Timothy Nikkel (:tnikkel) 2011-11-30 17:04:54 PST
Created attachment 578138 [details] [diff] [review]
Part 7 v3. 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.
Comment 40 Timothy Nikkel (:tnikkel) 2011-11-30 17:16:51 PST
Created 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.
Comment 41 Karl Tomlinson (:karlt) 2011-11-30 17:21:20 PST
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!

Note You need to log in before you can comment on or make changes to this bug.