"eaten" right-clicks: list of issues

star-buck commented 8 years ago

right-click on menu button to open window, then hover over Panel Options to have the sidepopup show. then move the mouse one up and click on "Alternatives" will not do anything while the popup is about to close. This also happens elsehwere, but the curious thing is that if you move the mouse too fast to the next entry you can execute the click normally, so it looks like its just happening when the dialog is in the mode to close but still visible.

star-buck commented 7 years ago

dolphin: right-click on folder, right-click again without moving mouse to close popup, right-click again without moving mouse sometimes doesnt make popup appear again.

notmart commented 7 years ago

on the issue that happens for the alternatives menu item, i found that it happens sometimes when the "panel options" submenu is open (not every time, seems to happen only when the submenu appeared since a very short time) in QMenu::mousePressevent, the proper mouse press event always arrives, and the proper action is always set in the call to QMenuPrivate::setCurrentAction() in the event handler, but in those rare cases, the action does not get triggered

notmart commented 7 years ago

seems the problem is in mouseReleaseEvent, sometimes d->mouseDown is nullptr, that makes the event handler bail out and the action not triggered

notmart commented 7 years ago

i think i have a Qt patch: https://paste.kde.org/ptp5z0eua d->mouseDown is static, so when you click on alternatives, if the panel options submenu is open, if the events gets delivered in just the right order (that's why doesn't always happen) the submenu gets hidden, resets d->mouseDown and then the mouse release event bails out. i think it may be correct to reset it only when mouseDown is the menu that is being hidden. David, what do you think about it? would that be correct? i can try to upstream it

davidedmundson commented 7 years ago

Does the submenu shares the same d pointer? I didn't think it did. If it doesn't, then I don't understand this patch.

Also if your logic is right, is the first d->mouseDown = 0 in the early return in mouseReleaseEvent wrong too?

Interestingly, (or rather frustratingly) it doesn't seem this dolphin bug that I'm looking into is the same thing.

davidedmundson commented 7 years ago

Update: Now I understand what you said: different d, shared d->mouseDown

Based on that we have a reliable way to reproduce what that bug is fixing:

click on panel options
move over alternatives
release

It's not entirely the same as starbuck's report, but it's effectively the same thing, click down - submenu hide event - click release, just easier to force that getting the clicks exactly in between the SH_Menu_SubMenuSloppyCloseTimeout.

As for my mouseReleaseEvent question, can you also check QMenu::event case Show: that'll be the same bug but in reverse, i.e if you had another submenu between panel options and alternatives.

davidedmundson commented 7 years ago

Writing my notes on the dolphin bug:

The times it's failing are when we get QGraphicsScenePrivate::mousePressEventHandler with the new right click before the QGraphicsScenePrivate::ungrabMouse

Right now I can't give a good writeup of when or why that happens.

davidedmundson commented 7 years ago

I can see exactly what's happening.

QtGraphicsView works as follows, when you click an item grabs the mouse (probably a folder icon or something).

If you release whilst that window still has focus, it releases it. However, for context menus (like all context menus) it has that awful issue that the window gets the mouse down, then never gets the mouse released because the app creates a new window that then "eats" the release event.

QtGraphicView has a bodge that works 99% of the time:

if (mouseEvent->button() == 0 && mouseEvent->buttons() == 0 && lastMouseGrabberItemHasImplicitMouseGrab) { // ### This is a temporary fix for until we get proper mouse // grab events. clearMouseGrabber(); return; }

Which reads if we get a mouse movement and the user isn't pressing any buttons, simulate the click being released. That doesn't work if you don't move before re-clicking. This "temporary fix" is from at least 2011, probably older.

Given it's definitely in QGraphicsScene there's absolutely no point trying to fix it in Qt, it's a deprecated module, I'd never get something proper in. I'll see if I can get something in Dolphin.

notmart commented 7 years ago

https://codereview.qt-project.org/#/c/185946/

davidedmundson commented 7 years ago

Dolphin patch: https://git.reviewboard.kde.org/r/129960/

notmart commented 7 years ago

@apachelogger can this patch be added to our qt build waiting it's upstreamed? https://codereview.qt-project.org/#/c/185946/

notmart commented 7 years ago

after months of back and forth with Qt people, finally got the patch merged https://codereview.qt-project.org/#/c/185946/ unfortunately they accepted it to be merged only in "dev" that means it will be only in Qt 5.10 (planned in november, so far) but since the patch is safe to merge in Qt 5.7 5.8 and 5.9, it should probably be included in our stuff for netrunner/maui/neon (@apachelogger, @shadeslayer is this feasible?)

star-buck commented 7 years ago

There are two things: Patching Qt is troublesome Since Qt5.10 is quite far, relying on all kde distros to be fixed needs a workaround.

For frameworks and plasma, its tolerable, but imo anything down the Qt layer warrants workarounds if the bug is identified and publicize too far away.

notmart commented 7 years ago

since this is an issue deep in qt, there are no general workarounds. one thing that could be done is to make it leass easy to trigger in the most visible cases, like putting the "panel options" submenu on top of the menu in the case of the panel being on the bottom, which could make sense in general

hsitter commented 7 years ago

not applying on 5.7 http://build.neon.kde.org/job/xenial_unstable_qt_qtbase_src/14/console

davidedmundson commented 7 years ago

I've found what's wrong with all the ones that affected QtQuick.

I understand the original bug, and why the workaround we kept adding everywhere no longer works.

I can fix the workaround but I'd need to spend another day or three to come up with a good patch for Qt. Otherwise we'll just keep hitting this same issue in new places where it's not worked around and having it break again with every release.

The root cause is:

QtQuick internally keeps track of which item has "grabbed" the mouse
It's weirdly a singleton between windows, but that should be fine
It relies on the fact that when we get an X mouse press event we should always get a mouse release event. X is surprisingly smart and will send this even if you change window focus.
However - context menus are bastards. They do a low level grab (not like the grab mentioned above) and grab all mouse events at the X level. That's why we don't get our release event.
Our original QtQuickItem which was handling the right click event no longer gets our release event
The reason it's only somtimes is we have a "race" between being created and the grab going through X, and the user actually releasing the mouse and that event being sent.

We used to work around this by just manually doing ungrabMouse() after we create a context menu. This no longer works because some code got shuffled. QQuickWindow now sends the event then grabs the mouse.

star-buck commented 7 years ago

What package/version would the fixed workaround be officially shipped in? (as waiting for official patched Qt or ship own repackaged patched Qt is not going to fly too well for most distros.)

star-buck commented 7 years ago

corrected above comment: i see this is only about right-clicks, not left clicks, so I guess Qt5.9 was only meant to not affect those.

davidedmundson commented 7 years ago

It's still sort of about left clicks. Though not necessarily all that you mentioned.

It silently messes up when a popup appears mid-click. This happens typically as the result of a right click. Could happen elsewhere.

That makes the next click (either left or right) go missing.

davidedmundson commented 7 years ago

Updated twist:

I've spent the day trying to recreate a very simple test, ready to fix Qt and was failing. It always worked without stealing the clicks.

My test was failing to fail because Qt has tried to detect this issue above: They've got a bit of code that says "if you lose window focus, ungrab the item."

So why doesn't that work in these plasma cases:

Panelview starts with: setFlags(Qt::FramelessWindowHint|Qt::WindowDoesNotAcceptFocus);

If it never accepted focus, we can't lose it. Obviously.

So: 1) I now have a nice easy to reproduce test which doesn't involve plasma.

2) It's still an issue on 5.9

3) I can possibly come up with a much much better workaround now I know this. Maybe even get a real fix purely on the Plasma side.

davidedmundson commented 7 years ago

I've pushed the updated workaround.

You will need: Plasma 5.9.6 AND plasma-framework 5.33

star-buck commented 7 years ago

Plasma 5.9.6 is due when again?

davidedmundson commented 7 years ago

Edit, turns out it's actually in Plasma 5.9.5. Jonathan must have got delayed tagging.

Release is today or so.

hsitter commented 7 years ago

qt patch landing in neon user edition in an hour or so

davidedmundson commented 7 years ago

which one?

We've got a few different issues all mashed up in here.

hsitter commented 7 years ago

^^

@davidedmundson https://packaging.neon.kde.org/qt/qtbase.git/tree/debian/patches/0001-Reset-d-mouseDown-only-if-it-s-the-menu-being-hidden.patch?h=Neon/release

blue-systems / plasma-issues

"eaten" right-clicks: list of issues #8