Bug 73821 – keyboard event doesn't fire while moving mouse with button pressed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Rakesh]

1. Register an eventListener for keydown. Add an alert in the callback function
2. Click and hold the left mouse button. Move the mouse around (as if you want to select something)
3. While moving the mouse press any key.

Key Down event listener is not called.

This happens only on Windows. Works fine on mac and linux.

Chromium issue: http://code.google.com/p/chromium/issues/detail?id=83518

[Rakesh]

This issue looks like hapening due to PAN scrolling.
After debugging I found it is returning from this code in EventHandler::keyEvent:

#if ENABLE(PAN_SCROLLING)
    if (Page* page = m_frame->page()) {
        if (page->mainFrame()->eventHandler()->m_panScrollInProgress || m_autoscrollInProgress) {
            // If a key is pressed while the autoscroll/panScroll is in progress then we want to stop
            if (initialKeyEvent.type() == PlatformKeyboardEvent::KeyDown || initialKeyEvent.type() == PlatformKeyboardEvent::RawKeyDown)
                stopAutoscrollTimer();

            // If we were in autoscroll/panscroll mode, we swallow the key event
            return true;
        }
    }
#endif

This check should only be for pan scrolling?

This I think some inputs can be taken from https://bugs.webkit.org/show_bug.cgi?id=28005

[Rakesh]

Created attachment 118007 [details]
Proposed patch

Removed the check the autoscroll in keyEvent so that autoscroll does not stop on key press. This behaviour is consistent with IE and Firefox.

[Alexey Proskuryakov]

Comment on attachment 118007 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118007&action=review

> Source/WebCore/page/EventHandler.cpp:2526
> -        if (page->mainFrame()->eventHandler()->m_panScrollInProgress || m_autoscrollInProgress) {
> +        if (page->mainFrame()->eventHandler()->m_panScrollInProgress) {
>              // If a key is pressed while the autoscroll/panScroll is in progress then we want to stop

Please fix the comment. It is no longer correct after the change.

When was this code introduced, and why?

> LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:11
> +        var y = document.body.scrollHeight + 100;

Please give "y" a meaningful name.

> LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:19
> +        debug("Test manually that scrolling deos not stop if we press a key while autoscroll is happening.");

Typo: does.

> LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:24
> +function testAutoScroll() {
> +    shouldBe('document.body.scrollTop + window.innerHeight', 'document.body.scrollHeight');

What guarantees that scrolling will be done by the time this function is called?

[Rakesh]

(In reply to comment #3)
> >              // If a key is pressed while the autoscroll/panScroll is in progress then we want to stop
> 
> Please fix the comment. It is no longer correct after the change.

Yes, the comment is no longer valid.
 
> When was this code introduced, and why?
> 

This change was introduced by http://trac.webkit.org/changeset/35758(Brian pointed me to this). This in my view was meant for pan scrolling only.

> > LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:11
> > +        var y = document.body.scrollHeight + 100;
> 
> Please give "y" a meaningful name.
> 
> > LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:19
> > +        debug("Test manually that scrolling deos not stop if we press a key while autoscroll is happening.");
> 
> Typo: does.
> 

Will do the suggested changes.

> > LayoutTests/fast/events/autoscroll-should-not-stop-on-keypress.html:24
> > +function testAutoScroll() {
> > +    shouldBe('document.body.scrollTop + window.innerHeight', 'document.body.scrollHeight');
> 
> What guarantees that scrolling will be done by the time this function is called?

Its an approx. time by which it should happen. Please suggest if any better way to know for sure.

[Rakesh]

Created attachment 118175 [details]
Updated patch

Modified comments and fixed typo and variable in test case.

[Alexey Proskuryakov]

Comment on attachment 118175 [details]
Updated patch

Talked on IRC, and I had some confusion about this patch. Posting the explanation here for posterity.

The fix does more than the original description asked for - in addition to making the event fire (which could be achieved by avoiding an early return), it fixes a user visible issue where autoscrolling stopped if a key was pressed.

Both new behaviors seem to match IE9 in my testing.

[WebKit Review Bot]

Comment on attachment 118175 [details]
Updated patch

Clearing flags on attachment: 118175

Committed r102408: <http://trac.webkit.org/changeset/102408>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on SL ever since the test was added:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r102526%20(35366)/results.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r102526%20(35366)/fast/events/autoscroll-should-not-stop-on-keypress-pretty-diff.html

[Alexey Proskuryakov]

Looks like this way of starting autoscroll doesn't work on Mac. But we have other autoscroll tests in fast/events, which work fine. Looks like the trick is to use an iframe instead of moving mouse pointer out of the window.

Rakesh, would you be willing to adjust the test to be more like existing autoscroll ones, so that it would work on Mac? In the meantime, Ryosuke is going to skip it.

[Rakesh]

(In reply to comment #10)
> Looks like this way of starting autoscroll doesn't work on Mac. But we have other autoscroll tests in fast/events, which work fine. Looks like the trick is to use an iframe instead of moving mouse pointer out of the window.
> 
> Rakesh, would you be willing to adjust the test to be more like existing autoscroll ones, so that it would work on Mac? In the meantime, Ryosuke is going to skip it.

Yes, will make that change. Sorry, I saw the failing tests but could not get hold of a mac so could not check the failures. 

https://bugs.webkit.org/show_bug.cgi?id=74538 is logged for this by rniwa.