The default bug view has changed. See this FAQ.

mouse events for :hover in XUL popup firing incorrectly

RESOLVED FIXED in mozilla11

Status

()

Core
XUL
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Kyle Simpson, Assigned: tnikkel)

Tracking

({dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 5 obsolete attachments)

8.73 KB, text/plain
Details
1010 bytes, text/plain
Details
7.26 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
5.79 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
4.51 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.06 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.35 KB, patch
Neil Deakin
: review+
roc
: review+
Details | Diff | Splinter Review
3.15 KB, patch
karlt
: review+
Details | Diff | Splinter Review
39.04 KB, patch
Details | Diff | Splinter Review
2.31 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.89 KB, patch
karlt
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 543065 [details]
testcase

So this testcase should reproduce the problem, correct?

Comment 2

6 years ago
No, the panel needs a titlebar:

<panel noautohide="true" titlebar="normal"/>
(Reporter)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
(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

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #543065 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 543806 [details]
testcase

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

Updated

6 years ago
Blocks: 659710
(Assignee)

Comment 7

6 years ago
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.
Assignee: nobody → tnikkel
(Reporter)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 8

6 years ago
Created attachment 554533 [details] [diff] [review]
Part 1. DeCOMtaminate nsMenuPopupFrame::GetWidget.
Attachment #554533 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #554533 - Flags: review? → review?(enndeakin)
(Assignee)

Comment 9

6 years ago
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.
Attachment #554535 - Flags: review?(enndeakin)
(Assignee)

Comment 10

6 years ago
Created attachment 554536 [details] [diff] [review]
Part 3. Make the client bounds of a widget be relative to its parent in all cases.
Attachment #554536 - Flags: review?(jmathies)
(Assignee)

Comment 11

6 years ago
Created attachment 554537 [details] [diff] [review]
Part 4. Add an API to widgets for resizing/moving the client area.
Attachment #554537 - Flags: review?(jmathies)
(Assignee)

Comment 12

6 years ago
Created attachment 554538 [details] [diff] [review]
Part 5. Make the view bounds of a popup coincide with the client area of the widget.
Attachment #554538 - Flags: review?(enndeakin)
(Assignee)

Comment 13

6 years ago
Created attachment 554539 [details] [diff] [review]
Part 6. Implement nsIWidget::GetClientOffset on GTK2.
Attachment #554539 - Flags: review?(karlt)
(Assignee)

Comment 14

6 years ago
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.
Attachment #554540 - Flags: review?(karlt)
(Assignee)

Comment 15

6 years ago
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.
Attachment #554541 - Flags: review?(karlt)

Comment 16

6 years ago
tn, would you mind posting a roll up patch merged to tip?
(Assignee)

Comment 17

6 years ago
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.

Updated

6 years ago
Attachment #554533 - Flags: review?(enndeakin) → review+

Updated

6 years ago
Attachment #554536 - Flags: review?(jmathies) → review+

Comment 18

6 years ago
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.
Attachment #554537 - Flags: superreview?(roc)
Attachment #554537 - Flags: review?(jmathies)
Attachment #554537 - Flags: review+
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.
Attachment #554537 - Flags: superreview?(roc) → superreview+

Comment 20

6 years ago
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
Attachment #554535 - Flags: review?(enndeakin) → review+

Comment 21

6 years ago
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.
Attachment #554538 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

6 years ago
Attachment #554538 - Flags: review?(roc)
Attachment #554538 - Flags: review?(roc) → review+
(Assignee)

Comment 22

6 years ago
(In reply to Neil Deakin from comment #20)
> outselves -> ourselves

Fixed. Thanks.
(Assignee)

Comment 23

6 years ago
(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.
Attachment #554539 - Flags: review?(karlt) → review+
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?
(Assignee)

Comment 27

6 years ago
(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.
(Assignee)

Comment 28

6 years ago
(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)
(Assignee)

Comment 30

6 years ago
(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.
Attachment #554540 - Flags: review?(karlt) → review-
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.
Attachment #554541 - Flags: review?(karlt)
(Assignee)

Comment 34

6 years ago
Sorry that I haven't been responsive. I'll try to come up with answers.
(Assignee)

Comment 35

6 years ago
(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.
(Assignee)

Comment 36

5 years ago
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.
Attachment #554541 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
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.
Attachment #554540 - Attachment is obsolete: true
Attachment #578134 - Flags: review?(karlt)
(Assignee)

Comment 38

5 years ago
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.
Attachment #578135 - Flags: review?(karlt)
Attachment #578135 - Flags: review?(karlt) → review+
(Assignee)

Comment 39

5 years ago
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.
Attachment #578134 - Attachment is obsolete: true
Attachment #578138 - Flags: review?(karlt)
Attachment #578134 - Flags: review?(karlt)
(Assignee)

Comment 40

5 years ago
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.
Attachment #578138 - Attachment is obsolete: true
Attachment #578140 - Flags: review?(karlt)
Attachment #578138 - Flags: review?(karlt)
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!
Attachment #578140 - Flags: review?(karlt) → review+
(Assignee)

Comment 42

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

5 years ago
Depends on: 707814
Keywords: dev-doc-needed
Depends on: 1077652
You need to log in before you can comment on or make changes to this bug.