:active tracking gets out of sync when releasing the mouse over a different document

RESOLVED FIXED

Status

()

Core
Event Handling
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mstange, Assigned: smaug)

Tracking

(Blocks: 1 bug, {regression})

Trunk
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Steps to reproduce:
1. Press the left mouse button on the refresh button in the browser chrome.
2. While holding the button down, move the mouse away from the refresh button, down into the content viewport.
3. Release the mouse button.
4. Move the mouse back over the refresh button.

Notice that the button now has the pressed appearance (:hover:active) instead of the merely hovered appearance.

My guess about what's happening:
nsEventStateManager probably assumes that it will get a mouseup for every mousedown that it receives. Before bug 130078 that usually happened (except when going into drag mode, bug 379272) because the widget was doing mouse capturing. Now there's no automatic mouse capturing at the chrome-content boundary anymore, so the mouseup gets sent to a different nsEventStateManager, so mActiveContent doesn't get cleared properly.
blocking2.0: --- → ?
I'm about to fix something quite close to this in bug 524037,  so I can
take a look at this too.
Assignee: nobody → Olli.Pettay

Updated

7 years ago
blocking2.0: ? → betaN+
Created attachment 473520 [details] [diff] [review]
patch

This should work reasonable well.

I need to write still a mochitest for this.
Attachment #473520 - Flags: review?(roc)
(Reporter)

Comment 3

7 years ago
Instead of adding a global for the active ESM, couldn't you just make mActiveContent global?

Moreover, I've just discovered that there's a very similar problem with hover: It can happen that there's hovered content in one ESM and then a mouseexit event is sent into a different ESM. This event then fails to clear mHoverContent in the previously-hovered ESM, and it also fails to send the right mouseout events. (Missing mouseout events can result in sticky tooltips, for example, which I've seen frequently since bug 130078 landed.)

This condition will be easily reproducible after bug 547787, which will remove the gap between the tabs and the chrome-content edge. Then you can reproduce it like this (on Mac):
1. Open a Firefox window and deactivate it.
2. Move the mouse over a tab and wait for the tooltip to appear.
3. Move the mouse down into the content viewport.
The tab will stay hovered and the tooltip doesn't go away.

These STR result in mousemove events on the chrome ESM followed by a mouseexit event on the content ESM. This mouseexit event is sent because content in inactive windows shouldn't receive mouse events.
This didn't happen before bug 130078 because our widget code would detect that the mouse has moved into a different widget. So it would send a mouse exit event to the previously-hovered widget which would end up in the chrome ESM.
(In reply to comment #3)
> Instead of adding a global for the active ESM, couldn't you just make
> mActiveContent global?
That should work too, but would cause some more code changes.

sActiveESM could be used also to fix :hover.
The good thing using global ESM is that we get mPrescontext easily.

I'll update the patch.
Summary: :active tracking gets out of sync when releasing the mouse over a different document → :active/:hover tracking gets out of sync when releasing the mouse over a different document
Attachment #473520 - Flags: review?(roc)
Blocks: 594791
(Reporter)

Updated

7 years ago
Duplicate of this bug: 594938
This also affects content, fwiw.
Created attachment 474817 [details] [diff] [review]
patch

Markus, does this fix also the hover problem you're seeing?
(I don't have a mac nearby.)
This does fix other similar sounding hover problems.
Attachment #473520 - Attachment is obsolete: true
Attachment #474817 - Flags: review?(mstange)
(Reporter)

Comment 8

7 years ago
It doesn't fix the hover problem I'm seeing. I'll try to find out why.
(Reporter)

Comment 9

7 years ago
Ah, because the problem I'm seeing doesn't really have to do with mHoverContent, but with mLastMouseOverElement.
In nsEventStateManager::NotifyMouseOut, we only send a mouse out event (and unset :hover) if this ESM has a mLastMouseOverElement, otherwise we return early. But the right mLastMouseOverElement is in a different ESM...
Comment on attachment 474817 [details] [diff] [review]
patch

This is actually wrong also in that :hover state should stay active
in the ancestor ESM.
Attachment #474817 - Attachment is obsolete: true
Attachment #474817 - Flags: review?(mstange)
(In reply to comment #6)
> This also affects content, fwiw.

In which way?
So what does  bug 547787 actually do?
Does it add :hover to something which is an ancestor of <browser>?
I think I'll split the :hover handling to a new bug.
Summary: :active/:hover tracking gets out of sync when releasing the mouse over a different document → :active tracking gets out of sync when releasing the mouse over a different document
(Reporter)

Comment 14

7 years ago
(In reply to comment #12)
> So what does  bug 547787 actually do?

I shouldn't have mentioned bug 547787... the problem is reproducible without it.

> Does it add :hover to something which is an ancestor of <browser>?

It uses :hover on something which is directly adjacent to the <browser>.

Before it, the tabs had a little gap under them, so when you moved your mouse over a tab and down into content, you could have a mouse out event on the tab before you entered content.

I'm talking about tabs-on-bottom, of course.
Blocks: 596600
Created attachment 475515 [details] [diff] [review]
patch
Attachment #475515 - Flags: review?(roc)
Comment on attachment 475515 [details] [diff] [review]
patch

Test?
Attachment #475515 - Flags: review?(roc) → review+
Will write a test before pushing this.
Blocks: 379272
Created attachment 475840 [details] [diff] [review]
+tests
http://hg.mozilla.org/mozilla-central/rev/88146d118475
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.