Tk Source Code

I ran into this bug when I tried to scroll the listbox control on my laptop with windows 8.
Using the two finger scroll with notebook touchpad on the [listbox] widget only works in one direction, down.
The use of the mouse wheel works correctly.
The [text] widget works correctly with two finger scroll with notebook touchpad..

Merged to core-8-6-branch and trunk.

Thanks to all for your contributions!

I have reviewed all this once more and have set my mind to the following conclusions:

1. What we currently have as a bugfix is correct. Accumulation on Windows OS only, numerically correct, with timeout, and a thread safe solution.

2. Trying to test this accumulation feature is pointless. There is no way to generate a WM_MOUSEWHEEL event (this can only be generated by the Windows OS itself, not by Tk). Event generating <MouseWheel> events does not generate a WM_MOUSEWHEEL event, but a <MouseWheel> event.

3. At the scripting level, it is okay to run event generate <MouseWheel> -delta 6 and it is fully correct to get the small delta in the bound script. It is exactly what the programmer specified: generate a <MouseWheel> event with a small delta of 6.

4. The documented behavior for event generate is "Generates a window event and arranges for it to be processed just as if it had come from the window system." This does not contradict the above: it is just that Tk will never see such a <MouseWheel> event with a delta of less than 120. The bugfix does exactly this, so that high resolution wheels are now supported. But IF the event generated by the OS WOULD BE propagated to Tk with a delta of 6 then this event would be seen by Tk as the same event as the one generated by event generate <MouseWheel> -delta 6

Shortly, there is nothing else to do in the bugfix branch than to remove the test that was added when we were trying to test accumulation. It is just not possible to check this. This removal is made in [b06d31bb].

Thank you, Francois, for the detailed view.

The possibility to have scroll steps smaller than "one page" might make sense for widgets which may scroll very detailed. So, both make sense:

• <MouseWheel> with +/-120 meaning "one page scroll". This is Button 7/8 on unix.
• <MouseWheel> with less than +/-120 meaning "smaller unit like one pixel". This does not exist on Unix.

Isn't is an idea to support both by firering two events:

• Fire Button7/8 on multiple of 120
• In addition fire <MouseWheel> with an eventuall smaller unit

As this "compatibility" to Unix may cause a double-scrolling, we may give the new event another name.

This might be elegant to require only one binding for Windows and Unix. On Windows, a fine scrolling application may choose only to bind to MouseWheel and not to Button7/8.

Just an idea.

Thanks, Harald

The problem if we added the accumulation code in HandleEventGenerate() is that:

- the user will no longer be able to generate <MouseWheel> events with less than WHEEL_DELTA (== 120) deltas: only when the accumulated deltas will reach WHEEL_DELTA the event will actually be generated. This is counterintuitive.

- HandleEventGenerate() is a generic code place, and we have a problem for Windows only (please correct me if I'm wrong here)

Actually I believe what is currently in the bugfix branch is correct. Only WM_MOUSEWHEEL processing needs accumulation. The user may want to event generate <MouseWheel> -delta 6, and this value of 6 for the delta should just be fed to the binding without being touched.

I think the test (listbox-32) does not really check what it intends to test. It checks there is accumulation of <MouseWheel> events with low deltas and that after a threshold of 120 the event really gets propagated to the widget. But it does so with a sequence of twenty event generate <MouseWheel> -delta 6, which are processed one by one without accumulation.

So the test would be incorrectly written.

What disturbs me however with this conclusion is twofold:

- I don't see how the accumulation feature can be tested without event generating <MouseWheel>

- From "event generate" man page:

Generates a window event and arranges for it to be processed just as if it had come from the window system.

The problem with the test is that

    event generate .l <MouseWheel> -delta 6

I guess the accumulation code needs to be added somewhere else, perhaps in Tk_HandleEvent().

Yes we can, thanks gfor the patch!

(Still same failure with the test case, I will look at this now).

Can we please have a thread safe solution somewhat like the attached patch?

Actually no. The test reveals an actual problem.

Thanks to both for your respective contributions.

I agree that fixing the Windows (WM_MOUSEWHEEL) event is way better than dealing with each binding of each impacted widget one by one. That's a good fix, thanks. Timeouts and numerical accuracy look good to me as well.

I have now committed this new implementation in the branch bugfix branch.

It occurred to me that the new test listbox-22 now fails (whereas it passed with the previous implementation in the listbox bindings) as follows:

==== listbox-32 <MouseWheel> event win32 FAILED
==== Contents of test case:

    set log {}
    .l insert 0 a b c d e f g h i j k l m n o p q r s t
    .l yview 8
    update
    record yv {*}[.l yview]
        for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta 6 }
        update
    record yv {*}[.l yview]
        for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta 6 }
        update
    record yv {*}[.l yview]
        for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta -6 }
        update
    record yv {*}[.l yview]
        for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta -6 }
        update
    record yv {*}[.l yview]
    set log

---- Result was:
{yv 0.4 0.65} {yv 0.4 0.65} {yv 0.4 0.65} {yv 0.75 1} {yv 0.75 1}
---- Result should have been (exact matching):
{yv 0.4 0.65} {yv 0.2 0.45} {yv 0 0.25} {yv 0.2 0.45} {yv 0.4 0.65}
==== listbox-32 FAILED

I guess the expected results need to be adjusted since we have now numerical accuracy, but I didn't dig into that failure yet.

So I looked to https://msdn.microsoft.com/en-us/library/windows/desktop/ms645617(v=vs.85).aspx to understand.

You basically implement in C what was proposed in TCL:

- have an accumulation timeout - only return multiple of 120 - store the division rest of 120 for the next accumulation

That looks smart.

So, on the TCL side, a simple bind is possible:

bind Listbox <MouseWheel> {
        %W yview scroll [expr {- (%D / 120) * 4}] units

And any other widget systems like BWIdget etc are autmatically fixed.

That looks great.

Thank you, Harald

I tested the touch panel of some other laptops:

Asus with Windows 7
Asus with Windows 8 (my)
Asus with windows 10
HP with windows 8
MSI with Windows 10

Unfortunately I do not have laptops with other systems. On two Asus with windows 8 and 10 the touch panel works at high resolution by sending WM_MOUSEWHEEL messages with the value of delta = + - 2..10. On the others, the touch panel works like a mouse wheel, sending WM_MOUSEWHEEL with delta = + - 120. In the corrected listbox, scrolling with the touch panel works fine.

Only it occurred to me that such correction is wrong. Because the <MouseWheel> event in Tk and the WM_MOUSEWHEEL message are not the same. WM_MOUSEWHEEL is described in MSDN. <MouseWheel> is described in "doc/event" and "doc/bind" (slightly differently). In general, processing of <MouseWheel> event in the entire Tk library is done uniformly.

After fixing the "listbox", the "scrollbar" widget remains and widgets that use the "ttk::bindMouseWheel" function. Need correct them?

The bug with processing exists on some Windows computers because of incomplete processing of WM_WOUSEWHEEL.

Therefore I suggest to do full processing WM_MOUSEWHEEL in "win/tkWinX.c" as it is described in MSDN, return the listbox as it was and do not touch other parts of Tk.

Consider this hotfix for:

...win/tkWinX.c line 1134 ...
	case WM_MOUSEWHEEL: {
	    /* 
	     * Support for high resolution wheels
	     */
	    static DWORD dwTickCountPrev = 0;
	    static short wheel_acc = 0;
	    DWORD dwTickCount;

	    dwTickCount = GetTickCount();
	    if (dwTickCount - dwTickCountPrev < 1500) {
		wheel_acc += (short) HIWORD(wParam);
	    } else {
		wheel_acc = (short) HIWORD(wParam);
	    }
	    dwTickCountPrev = dwTickCount;

	    if (abs(wheel_acc) < WHEEL_DELTA) return;

	    /*
	     * We have invented a new X event type to handle this event. It
	     * still uses the KeyPress struct. However, the keycode field has
	     * been overloaded to hold the zDelta of the wheel. Set nbytes to
	     * 0 to prevent conversion of the keycode to a keysym in
	     * TkpGetString. [Bug 1118340].
	     */

	    event.type = MouseWheelEvent;
	    event.xany.send_event = -1;
	    event.xkey.nbytes = 0;
	    event.xkey.keycode = wheel_acc / WHEEL_DELTA * WHEEL_DELTA;
	    wheel_acc = wheel_acc % WHEEL_DELTA;
	    break;
	}

Just getting the best code, like as suggested by Harald with substraction the consumed delta and timeout

Dear dnikolajevich,

thank you for test, great that it resolves the issue.

About your comment:

> oehhar, I agree, you can rewrite the event processing procedure correctly, but I think that the code size is more important here.

Well, code size (or memory) comparison:

• 2 global variables against one per Widget
• Two compilable procs compared to longer bound scripts.

It might happen, that the alternate version wins in code size...

And functionality might be better, which is the important point.

Neverhteless, great that it works. This is absolutely ok.

Thank you, Harald

Thank you for dealing with the problem.

The current patch really solves the problem with the MouseWheel event in listbox on my touchpad.

oehhar, I agree, you can rewrite the event processing procedure correctly, but I think that the code size is more important here.

For testing, it's good to add tests to "tests/lisbox.test". Since there were no tests for check the event <MouseWheel> I suggest to add them. For example:

... tests/listbox.test (from line ~3200) ...
test listbox-32.2 {<MouseWheel> event win32} -constraints {
	win 
} -setup {
    destroy .l
    listbox .l -width 10 -height 5
    pack .l
    update
} -body {
    set log {}
    .l insert 0 a b c d e f g h i j k l m n o p q r s t
    .l yview 8
    update
    record yv {*}[.l yview]
	for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta 6 }
	update
    record yv {*}[.l yview]
	for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta 6 }
	update
    record yv {*}[.l yview]
	for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta -6 }
	update
    record yv {*}[.l yview]
	for {set i 0} {$i < 20} {incr i} { event generate .l <MouseWheel> -delta -6 }
	update
    record yv {*}[.l yview]
    set log
} -cleanup {
    destroy .l
} -result {{yv 0.4 0.65} {yv 0.2 0.45} {yv 0 0.25} {yv 0.2 0.45} {yv 0.4 0.65}}

I put the files "tests/all.tcl", "tests/constraints.tcl", "test/listbox.test" in a separate directory. Added code above in listbox.test and run

tclsh all.tcl

All works.

Francois, for me, the patch looks good.

Two thoughts:

Numerics

Numerically correct would be to substract only the consumed delta. I am not convinced that this is relevant.

bind Listbox <MouseWheel> {
        incr tk::Priv(wheel_acc_v_%W) %D
        if {abs($tk::Priv(wheel_acc_v_%W)) >= 120} {
            set delta [expr {-$tk::Priv(wheel_acc_v_%W) / 120}]
            incr tk::Priv(wheel_acc_v_%W) $Delta
            %W yview scroll [expr {$delta * 4}] units
        }

Only one mousewheel counter with auto-reset

Eventually, the following would be a good idea:

• have only one counter for all Widgets for Mousewheel Delta on Windows
• Reset it to 0 with a timeout, e.g. after 3 seconds.

I think, it should be more a short-time memory. User might be surprised to have a quick jump after even have changed the widget and to return.

I am not good in bindings and not in tk internals. But here would be the idea as a binding sketch:

bind Listbox <MouseWheel> "tk::winmousewheel %D %W"
proc tk::winmousewheel {delta win} {
    if {[info exists tk::Priv(wheel_acc_after_id)]} {
        after cancel $tk::Priv(wheel_acc_after_id)
        unset tk::Priv(wheel_acc_after_id)
    }
    incr tk::Priv(wheel_acc_v) %D
    if {abs($tk::Priv(wheel_acc_v)) >= 120} {
        set delta [expr {-$tk::Priv(wheel_acc_v) / 120}]
        incr tk::Priv(wheel_acc_v) $Delta
        %W yview scroll [expr {$delta * 4}] units
    }
    if {$tk::Priv(wheel_acc_v) != 0} {
        after 3000 tk::winmousewheelreset
    }
}
proc tk::winmousewheelreset {} {
    unset -nocomplain tk::Priv(wheel_acc_after_id)
    unset -nocomplain tk::Priv(wheel_acc_v)
}

I would also be happy to have this procedure as a general binding utility which might even be used by external packages. We have the same issue in BWidget...

Big issue is the testability. My mousepad does not show the issue. Maybe, we should simulate the issue by "event generate" commands.

Thank you, Harald

I have now committed a slightly modified version of the suggested fix in branch bug-75d38f8608.

Could dnikolajevich (the OP who is seing the bug) please check that this fixes the issue for the listbox?

Also, I think we have the same issue with:

- the Tk scrollbar, see this source code

- Ttk scrollable items, see source code here and there

Could you please confirm (please provide your test script) and check whether a similar fix does the job for you?

I agree for the own ticket.

I have no patch ready.

It was an error to write this. I was just frustrated about this fact not working. Perhaps it works, but I make something wrong (-takefocus missing etc.).

Sorry, Harald

I'd say this is a different issue which deserves it's own ticket. Moreover could you suggest a patch?

Francois,

great that you are careing.

What I experience as anoying is that MouseWheel bindings are not assigned by default to ttk::scrollbar. Perhaps this is another ticket. But that, I would appreciate.

Thank you, Harald

Thanks for all the explanations, and for the suggested fix.

From the WM_MOUSEWHEEL documentation on MSDN:

The wheel rotation will be a multiple of WHEEL_DELTA, which is set at 120. This is the threshold for action to be taken, and one such action (for example, scrolling one increment) should occur for each delta.

The delta was set to 120 to allow Microsoft or other vendors to build finer-resolution wheels (a freely-rotating wheel with no notches) to send more messages per rotation, but with a smaller value in each message. To use this feature, you can either add the incoming delta values until WHEEL_DELTA is reached (so for a delta-rotation you get the same response), or scroll partial lines in response to the more frequent messages. You can also choose your scroll granularity and accumulate deltas until it is reached.

Since one of the options (scroll partial lines) is not available with the listbox (scroll units can only be pages or units aka lines), I conclude that you have proposed the right fix by accumulating the deltas.

I'll commit this in a bugfix branch soon. I have figure out whether other <MouseWheel> bindings need to be changed accordingly or not.

I have more details on the problem.
This occurs due to the fact that with two finger scroll with touchpad
the widget receives several messages WM_MOUSEWHEEL with a value of delta
smaller than when using the mouse wheel. This can be seen from the command

bind .lb <MouseWheel> { puts %D }

So when scrolling with the mouse wheel, MouseWheel events occur with
%D values equal to +-120, and when two finger scroll them goes more,
but the value of %D lies within 2..10 (in my system).
With the .text widget, such problems do not occur because the events
for it are set this way:
(/tk8.6.8/library/text.tcl)
    bind Text <MouseWheel> {
        if {%D >= 0} {
            %W yview scroll [expr {-%D/3}] pixels
        } else {
             %W yview scroll [expr {(2-%D)/3}] pixels
        }
    }
This is not entirely correct, but it works. And for the listbox widget,
this is set like this:
(/tk8.6.8/library/listbox.tcl)
    bind Listbox <MouseWheel> {
        %W yview scroll [expr {- (%D / 120) * 4}] units
    }
In my system, I got rid of this problem by changing the code in
"listbox.tcl" file like this:
    bind Listbox <MouseWheel> {
        incr tk::wheel_acc(%W) %D
        if {abs( $tk::wheel_acc(%W) ) >= 120} {
            %W yview scroll [expr {- ($tk::wheel_acc(%W) / 120) * 4}] units
            set tk::wheel_acc(%W) 0
        }
    }
But I'm not sure that this code is correct and beautiful.

Hi folks,

we have the following ways to scroll:

• a scrollbar control
• Mousewheel
• Two-finger gesture on touch-screen
• Two-finger gesture on multi-touchpad

Francois, if you have a laptop, just try it (for example in Firefox): take two fingers on the mouse-pad. Say, we have amouse pad with 10cm x 10 cm size and we put a coordinate system with 0 at the upper left corner. You put the fingers at y=4cm and x= 4 and 6 cm. Then you slide to y=6cm and x= 4 and 6 cm (slide down). This scrolls nicely. I personally like that.

I never knew that it works within tk so I may investigate it. I have no idea how mouse pad gestures are sent to the application. Eventually, it is also driver dependent.

Now I have tried in wish 8.6.8:

% pack [listbox .l]
% .l insert end A B C D E F G H I J K L M

And the result with the scrolling possibilities:

* mousewheel: ok, scrolls up and down * two-finger gesture on mousepad: ok, scrolls up and down * two finger gesture on touch-screen: does not scroll. Instead, the line is selected.

So I don't see the bug bug am happy that the mousepad gesture scroll works.

The is a Dell Latitude E7440 with Win10 64 bit.

I cannot try to reproduce, I don't have any such hardware nor Win 8 available.

I'm not even sure of what is "the two finger scroll" action.

Are you able to start some debugging on your side perhaps?