Bug 92681 - XI2 events' subpixel precision may come slightly out of window positions
Summary: XI2 events' subpixel precision may come slightly out of window positions
Status: RESOLVED MOVED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/Input/Core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Peter Hutterer
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-26 17:29 UTC by Carlos Garnacho Parro
Modified: 2018-12-17 17:27 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
app with button in the top left corner (1.28 KB, text/x-csrc)
2018-01-24 17:25 UTC, Adam Purkrt
no flags Details
effect of the bug on menu in MATE desktop (158.36 KB, video/x-matroska)
2018-01-26 14:13 UTC, Adam Purkrt
no flags Details
Fix (2.14 KB, patch)
2018-01-27 01:22 UTC, Egmont Koblinger
no flags Details | Splinter Review

Description Carlos Garnacho Parro 2015-10-26 17:29:03 UTC
On gnome-shell, if I run xinput test-xi2, side-maximize it on the left by dragging the window to the left edge, the window will end up aligned to the left screen edge. However, moving the pointer towards that edge will sometimes issue events like:

EVENT type 6 (Motion)
    device: 2 (16)
    detail: 0
    flags: 
    root: -0.86/139.21
    event: -0.86/75.21
    buttons:
    modifiers: locked 0 latched 0 base 0 effective: 0
    group: locked 0 latched 0 base 0 effective: 0
    valuators:
        0: -0.86
    windows: root 0xd6 event 0x4600001 child 0x0

As can be seen in https://bugzilla.gnome.org/show_bug.cgi?id=756463 , this negative coordinate fools GTK+ into thinking it should synthesize crossing events as those happen.
Comment 1 Douglas 2017-03-22 02:56:56 UTC
It has been reported that it affects windows placed to the left and top of the screen. I just want to add that it's affecting panels (maybe it's the same thing. Is it?) The wingpanel of elementary OS doesn't always recognize the pointer passing over and clicking, same with Budgie's top panel and MATE's. Unity works fine, and so does Cinnamon and Gnome.

I've tested on three absolutely different machines. All affected. Does it affect all users? Why isn't this getting the proper attention...

Most importantly right now, is there a workaround? Can I replace this version of Xinput?
Comment 2 Douglas 2017-07-02 18:51:16 UTC
This bug will die with Xorg. No activity. No reports elsewhere. No way to set a bounty on it. Wayland is not affected, it seems.
Comment 3 Adam Purkrt 2018-01-24 17:20:41 UTC
Xfce4-panel 4.13 and higher - the new version relying on gtk3 - is hit by this too. Really annoying.
Comment 4 Adam Purkrt 2018-01-24 17:25:45 UTC
Created attachment 136946 [details]
app with button in the top left corner

A simple application based on gtk3 with a button in the upper left corner. Move the mouse pointer along the screen edge near the top left corner of the screen and watch as the button gets selected and deselected, randomly (changes shade). It should always be selected, when the mouse is over it. This is not always the case.

Something really should done about this bug, it is a major annoyance.
Comment 5 Adam Purkrt 2018-01-26 14:13:15 UTC
Created attachment 136974 [details]
effect of the bug on menu in MATE desktop

Video demonstrating the bug in MATE destkop. The item under mouse cursor is not always selected. Also, the blinking is the result of this bug.
Comment 6 Egmont Koblinger 2018-01-27 01:10:10 UTC
(In reply to Douglas from comment #1)

> Why isn't this getting the proper attention...

(In reply to Adam Purkrt from comment #4)

> it is a major annoyance.

I can totally feel your pain, guys. I love Linux and open source and stuff, but there are a couple of things I truly dislike, one of them being usability annoyances getting way lower attention than they'd deserve. I'm not talking about X.Org in particular, it's across all the pieces. If it was me, I'd say such bugs are release critical. Devs usually say these are minor.

(In reply to Douglas from comment #2)

> No way to set a bounty on it.

How many beers would you have paid? :-)
Comment 7 Egmont Koblinger 2018-01-27 01:22:36 UTC
Created attachment 136979 [details] [review]
Fix

After 4 hours of debugging a totally unknown source, here's a fix that works for me. Could you guys please test it?

It's against 1.19.5 because I recompiled my distro's packages and that's the version my distro ships. I hope it still more or less applies against git master.

There are two things to the story, and I think either segment of my patch fixes the issue, but it's probably better to have both parts in place.

One is an ugly use of trunc() instead of floor(). trunc() rounds towards zero, and as such, with slightly negative (temporary??, before clamping??) mouse coordinates, root_x_frac becomes negative (e.g. a value of -0.6 is stored as root_x = 0, root_x_frac = -0.6, as opposed to root_x = -1, root_x_frac = 0.4 which would be the logical, following the pattern of the rest of the code with the FP1616 and FP3232 data types).

Another is that at certain places, root_x is set to a certain value, but root_x_frac remains unchanged, still containing the previous value. This is probably how the previous negative fraction can leak to places it shouldn't.

[Same for root_y and root_y_frac, of course.]

My patch (especially this latter part, resetting root_x_frac whenever root_x is set) may not be complete, and I have a slight feeling that my patch still doesn't address the real root cause. I'm not sure about it.

Meta:

I think I've done my fair share to fix this issue, I won't have any more time to work on it. Douglas, Adam, could you please nag the devs on mailing lists and such if they don't notice this patch here in a reasonable time? It should be a piece of cake for them to polish this up and commit based on my findings.
Comment 8 Adam Purkrt 2018-01-27 15:46:14 UTC
Thank you very much Egmont for your time. Just testing the patch against 1.19.6 (it is also applicable to current git version). So far so good. I will try to post a short memo of this bug at xorg-devel, hopefully it will get some notice.
Comment 9 Adam Purkrt 2018-01-28 18:09:09 UTC
Testing a bit more, I found it is not necessary to substitute trunc with floor; the patch works when only the part setting root_x_frac and root_y_frac to zero is left in.
Comment 10 Jason Gerecke 2018-01-29 17:11:24 UTC
Gave this a quick test in the hopes that it might fix (or at least improve) bug 92186. Unfortunately, it looks like all this does is just result in the loss of fractional coordinate data, causing errors to increase slightly across the board. E.g., the first row of the table in the linked bug would now report a root coordinate of "164.00" making the delta from expected "-3.70" instead of "-3.00".
Comment 11 Egmont Koblinger 2018-01-29 19:49:22 UTC
Jason, I'm sorry to hear that my patch makes another bug a bit worse. My patch consists of two parts, either one is sufficient to fix this bug here. Could you please maybe try which of its two parts cause that regression?

Taking a quick look at that other bug, you seem to be much more familiar with this topic and the intended behavior than I am. Either you or some other X.Org developer should take a closer look, I'm afraid I'm unable to further help, it would require much more time than I can voluntarily devote to this issue.

Both parts of my patch fix a "code smell", where the code was just fishy, didn't fit in with the rest of the picture. As I already mentioned, I had a guts feeling that I did not address the core issue, I mean why does that number become negative in the first place?

There must be levels of architectural complexity that I'm totally unaware of... But just looking at the overall picture: Handling the motion of a value within a certain interval causes problems, really?? Just sounds totally ridiculous.

While my patch makes your bug even somewhat worse, it at least fixes one of the two bugs, so still it's not obvious whether it's more useful or more harmful to apply it on its own.
Comment 12 Daniel Stone 2018-01-31 11:29:52 UTC
A few suggestions.

Firstly, Xorg patches are reviewed on the mailing list (xorg-devel@lists.x.org), with patches sent through git send-email; you might want to try that to get some more attention.

Secondly, the changes to CheckVirtualMotion() seem like they would drop fractions on the floor in too many cases. Presumably we only want to do that when the co-ordinates actually hit the limits, rather than unconditionally drop the fractional component even when it's within bounds.

One way to do that would be an explicit check against the limits, and only set ev->root_* to the sprite co-ordinates if they're out of bounds. Alternatively, you could make the sprite's HotSpot struct actually carry the fractional parts as well. I would have a slight preference for the latter, but am not involved in Xorg or review anymore, so if anyone else suggests otherwise, you're probably better off listening to them. :)

I'm curious about the changing of rounding from trunc() to floor(). Previously, -0.86 would be expressed as (0, -0.86), and this changes it to (-1, 0.14). It's not necessarily a bad change, but it is a user-visible one and I'm curious why it's necessary or better.

You might also want to put some tests for negative co-ordinates in test/xi2/protocol-eventconvert.c or similar.
Comment 13 Rafael Kitover 2018-03-21 20:13:34 UTC
Any news about this?

I just hit this bug here:

https://bugs.launchpad.net/bugs/1757209
Comment 14 GitLab Migration User 2018-12-17 17:27:26 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/577.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.