Bug 20303 – [Qt] Key events are not working in frames.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

Example: 

http://www.koffice.org/developer/apidocs/

[Tor Arne Vestbø]

Works using the 4.4.0 demobrowser, but no with arora against 4.0.0. or main

[Tor Arne Vestbø]

Using the arrow keys to navigate the individual frames of the test case site (KOffice API docs) does not work. This works fine when there's only one main frame. Tab key works though.

[Yongjun Zhang]

Created attachment 30558 [details]
sending scroll key events to subframe.

For scrolling keys (Key_Up, Key_Down, Key_Left, Key_Right), send the key event to currently focused subframe, instead of the main frame.

[Yongjun Zhang]

Created attachment 30560 [details]
Oops! The previous patch was wrong.

Send scrolling key events to subframe, instead of the main frame.

[Holger Freyther]

Comment on attachment 30560 [details]
Oops! The previous patch was wrong.


> -    if (!mainFrame->d->frame->eventHandler()->scrollOverflow(direction, granularity))
> -        mainFrame->d->frame->view()->scroll(direction, granularity);
> +    bool handled = true;
> +    if (!frame->eventHandler()->scrollOverflow(direction, granularity)) {
> +        handled = frame->view()->scroll(direction, granularity);
> +        Frame* parent = frame->tree()->parent();
> +        while(!handled && parent) {
> +            handled = parent->view()->scroll(direction, granularity);
> +            parent = parent->tree()->parent();
> +        }
> +    }

Are you sure you want to descent here? We are using focusedOrMainFrame() in the qwebpage.cpp. I don't think it is appropriate to descent here...

I think you want the following code..

    if (!frame->eventHandler()->scrollOverflow(direction, granularity))
        return frame->view()->scroll(direction, granularity);
    return false;

you return true... could you elaborate?

and it would be great if you could create a LayoutTest for this bug. We have an EventSender in Qt which allows you to send key events from HTML and then you can check if the keys arrive at the right frame. Could you attempt doing that?

[Holger Freyther]

As of the comment in Bug #26070 we will need this eventSender patch.

[Yongjun Zhang]

Created attachment 30870 [details]
add a layout test to test the fix.

thanks for the comments.

The idea is to scroll the currently focused subframe first, if it can't scroll to the desired direction, we scroll the parent frame. 

I added a layout test to make sure the key event goes to the right sub-frame.

[Holger Freyther]

Comment on attachment 30870 [details]
add a layout test to test the fix.

Great. You could make this with one or two lines less of code with a do {} while construct. Would you be interested in moving this function into the EventHandler class and make the windows version call that "shared" implementation. It would be greatly appreciated?

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 30870 [details] [review])
> Great. You could make this with one or two lines less of code with a do {}
> while construct. Would you be interested in moving this function into the
> EventHandler class and make the windows version call that "shared"
> implementation. It would be greatly appreciated?

It sounds like this still needs work?  It's r+'d but it sounds like I can't land it as is.  Please advise.

[Yongjun Zhang]

Created attachment 31374 [details]
changes according to the comments.

Changes as per zecke's comment:

1. move scroll handling code to EventHandler so that other ports can share the same functionality.
2. use do..while to shorten the code a bit.

[Yongjun Zhang]

Created attachment 31378 [details]
new patch to include recommended changed in the comments.

Change maded as per zecke's suggestions:

1. move scrolling handling code to EventHandler so that other ports can use it.
2. use do...while to shorten the code a bit.

[Adam Treat]

Comment on attachment 31378 [details]
new patch to include recommended changed in the comments.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44737)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Yongjun Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: platform/qt/fast/events/event-sender-keydown-frame.html
> +
> +        Bug 20303: [Qt] Key events are not working in frames.
> +
> +        Merge scrolling handling code in qt and win port, move it to
> +        EventHandler.
> +
> +        * page/EventHandler.cpp:
> +        (WebCore::EventHandler::scrollRecursively):
> +        * page/EventHandler.h:
> +
>  2009-06-16  Jian Li  <[e-mail]>
>  
>          Reviewed by David Levin.
> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 44736)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -856,6 +856,21 @@ bool EventHandler::scrollOverflow(Scroll
>      return false;
>  }
>  
> +bool EventHandler::scrollRecursively(ScrollDirection direction, ScrollGranularity granularity)
> +{
> +    bool handled = scrollOverflow(direction, granularity);
> +    if (!handled) {
> +        Frame* f = m_frame;
> +        do {
> +            FrameView* view = f->view();
> +            handled = view ? view->scroll(direction, granularity) : false;
> +            f = f->tree()->parent();
> +        } while (!handled && f);
> +     }
> +
> +    return handled;
> +}
> +
>  IntPoint EventHandler::currentMousePosition() const
>  {
>      return m_currentMousePosition;
> Index: WebCore/page/EventHandler.h
> ===================================================================
> --- WebCore/page/EventHandler.h	(revision 44736)
> +++ WebCore/page/EventHandler.h	(working copy)
> @@ -109,6 +109,8 @@ public:
>  
>      bool scrollOverflow(ScrollDirection, ScrollGranularity);
>  
> +    bool scrollRecursively(ScrollDirection, ScrollGranularity);
> +
>      bool shouldDragAutoNode(Node*, const IntPoint&) const; // -webkit-user-drag == auto
>  
>      bool tabsToLinks(KeyboardEvent*) const;
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 44737)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Yongjun Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 20303: [Qt] Key events are not working in frames.
> +        
> +        Send scrolling events to current focused frame, bubble the event
> +        up to parent frame if it is not handled.  Use EventHandler's new
> +        shared scrolling code.
> +
> +        * Api/qwebpage.cpp:
> +        (QWebPagePrivate::keyPressEvent):
> +        (QWebPagePrivate::handleScrolling):
> +        * Api/qwebpage_p.h:
> +
>  2009-06-16  David Boddie  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 44736)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -796,7 +796,7 @@ void QWebPagePrivate::keyPressEvent(QKey
>              defaultFont = view->font();
>          QFontMetrics fm(defaultFont);
>          int fontHeight = fm.height();
> -        if (!handleScrolling(ev)) {
> +        if (!handleScrolling(ev, frame)) {
>              switch (ev->key()) {
>              case Qt::Key_Back:
>                  q->triggerAction(QWebPage::Back);
> @@ -999,7 +999,7 @@ void QWebPagePrivate::shortcutOverrideEv
>      }
>  }
>  
> -bool QWebPagePrivate::handleScrolling(QKeyEvent *ev)
> +bool QWebPagePrivate::handleScrolling(QKeyEvent *ev, Frame* frame)

Inconsistent * decoration here.  Choose one or the other and it looks like it should be Qt coding style judging from the surrounding code.

>  {
>      ScrollDirection direction;
>      ScrollGranularity granularity;
> @@ -1046,10 +1046,7 @@ bool QWebPagePrivate::handleScrolling(QK
>          }
>      }
>  
> -    if (!mainFrame->d->frame->eventHandler()->scrollOverflow(direction, granularity))
> -        mainFrame->d->frame->view()->scroll(direction, granularity);
> -
> -    return true;
> +    return frame->eventHandler()->scrollRecursively(direction, granularity);
>  }
>  
>  /*!
> Index: WebKit/qt/Api/qwebpage_p.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage_p.h	(revision 44736)
> +++ WebKit/qt/Api/qwebpage_p.h	(working copy)
> @@ -45,6 +45,7 @@ namespace WebCore
>      class Element;
>      class Node;
>      class Page;
> +    class Frame;
>  
>  #ifndef QT_NO_CURSOR
>      class SetCursorEvent : public QEvent {
> @@ -113,7 +114,7 @@ public:
>  
>      void shortcutOverrideEvent(QKeyEvent*);
>      void leaveEvent(QEvent *);
> -    bool handleScrolling(QKeyEvent*);
> +    bool handleScrolling(QKeyEvent*, WebCore::Frame*);
>  
>  #ifndef QT_NO_SHORTCUT
>      static QWebPage::WebAction editorActionForKeyEvent(QKeyEvent* event);
> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 44737)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Yongjun Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +	
> +	Bug 20303: [Qt] Key events are not working in frames.
> +        
> +        Move the scroll handling code to EventHandler so that other
> +        ports can share the functionality.
> +
> +	* WebView.cpp:
> +        (WebView::keyDown):
> +        (EnumTextMatches::QueryInterface):
> +
>  2009-06-16  Brent Fulgham  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> Index: WebKit/win/WebView.cpp
> ===================================================================
> --- WebKit/win/WebView.cpp	(revision 44736)
> +++ WebKit/win/WebView.cpp	(working copy)
> @@ -1600,15 +1600,7 @@ bool WebView::keyDown(WPARAM virtualKeyC
>              return false;
>      }
>  
> -    if (!frame->eventHandler()->scrollOverflow(direction, granularity)) {
> -        handled = frame->view()->scroll(direction, granularity);
> -        Frame* parent = frame->tree()->parent();
> -        while(!handled && parent) {
> -            handled = parent->view()->scroll(direction, granularity);
> -            parent = parent->tree()->parent();
> -        }
> -    }
> -    return handled;
> +    return frame->eventHandler()->scrollRecursively(direction, granularity);
>  }
>  
>  bool WebView::keyPress(WPARAM charCode, LPARAM keyData, bool systemKeyDown)
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 44737)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> [phone]  Yongjun Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 20303: [Qt] Key events are not working in frames.
> +
> +        Add a layout test to test the event is sent to the right sub-frame.
> +
> +        * platform/qt/fast/events/event-sender-keydown-frame-expected.txt: Added.
> +        * platform/qt/fast/events/event-sender-keydown-frame.html: Added.
> +
>  2009-06-16  Xan Lopez  <[e-mail]>
>  
>          Disable another new test.
> Index: LayoutTests/platform/qt/fast/events/event-sender-keydown-frame-expected.txt
> ===================================================================
> --- LayoutTests/platform/qt/fast/events/event-sender-keydown-frame-expected.txt	(revision 0)
> +++ LayoutTests/platform/qt/fast/events/event-sender-keydown-frame-expected.txt	(revision 0)
> @@ -0,0 +1,4 @@
> +Test for bug 20303: [Qt] key events are not working in frames.
> +
> +
> +SUCCESS
> Index: LayoutTests/platform/qt/fast/events/event-sender-keydown-frame.html
> ===================================================================
> --- LayoutTests/platform/qt/fast/events/event-sender-keydown-frame.html	(revision 0)
> +++ LayoutTests/platform/qt/fast/events/event-sender-keydown-frame.html	(revision 0)
> @@ -0,0 +1,53 @@
> +<head>
> +</head>
> +<body>
> +    <p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=20303">bug 20303</a>:
> +    [Qt] key events are not working in frames.</p>
> +<script>
> +    function reportIFramePos()
> +    {
> +        var x = document.getElementById("anIFrame").contentDocument.body.scrollLeft;
> +        var y = document.getElementById("anIFrame").contentDocument.body.scrollTop;
> +
> +        // result, the iframe should be scrolled down
> +        if (y > 0)
> +            document.getElementById("console").innerHTML = "SUCCESS";
> +        else
> +            document.getElementById("console").innerHTML = "FAILURE";
> +    }
> +
> +    function testAndReport() {
> +         if (window.layoutTestController)
> +            layoutTestController.dumpAsText();
> +
> +        if (window.eventSender) {
> +            var frame = document.getElementById("anIFrame");
> +
> +            // center the mouse cursor
> +            var x = frame.offsetLeft + frame.offsetWidth/2;
> +            var y = frame.offsetTop + frame.offsetHeight/2;
> +
> +            // send mouse event to focus the iframe
> +            eventSender.mouseMoveTo(x, y);
> +            eventSender.mouseDown();
> +            eventSender.mouseUp();
> +
> +            // send key down event
> +            eventSender.keyDown('\uf701');
> +
> +            // report
> +            reportIFramePos();
> +            layoutTestController.notifyDone();
> +        }
> +    }
> +</script>
> +
> +<iframe style="width:350px;border:dotted green 1px" width="200" height="200"
> + id="anIFrame" src="resources/divs.html" onload="javascript:testAndReport()"></iframe>
> +</iframe>
> +<div id="result">
> +</div>
> +<div id="console">
> +</div>
> +</body>
> +</html>

[Adam Treat]

Oops, to much context...

To be clear the patch looks good, but you need to change the coding style of the line:

> +bool QWebPagePrivate::handleScrolling(QKeyEvent *ev, Frame* frame)

So that the * decoration is at least consistent.

[Yongjun Zhang]

Created attachment 31647 [details]
Correct the coding style pointed out by Adam.

[Holger Freyther]

Great! thank you very much.

[Eric Seidel (no email)]

Comment on attachment 31647 [details]
Correct the coding style pointed out by Adam.

f should be "frame" in:
Frame* f = m_frame;

OTherwise this looks fine.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/platform/qt/fast/events/event-sender-keydown-frame-expected.txt
	A	LayoutTests/platform/qt/fast/events/event-sender-keydown-frame.html
	M	WebCore/ChangeLog
	M	WebCore/page/EventHandler.cpp
	M	WebCore/page/EventHandler.h
	M	WebKit/qt/Api/qwebpage.cpp
	M	WebKit/qt/Api/qwebpage_p.h
	M	WebKit/qt/ChangeLog
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebView.cpp
Committed r45257
http://trac.webkit.org/changeset/45257

[Robert Hogan]

Created attachment 58290 [details]
Patch

[Robert Hogan]

(In reply to comment #18)
> Created an attachment (id=58290) [details]
> Patch

It looks like this test was added without elements required for it work.

[Robert Hogan]

Reopening so that fix to test can be reviewed and landed.

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> Oops, to much context...
> 
> To be clear the patch looks good, but you need to change the coding style of the line:
> 
> > +bool QWebPagePrivate::handleScrolling(QKeyEvent *ev, Frame* frame)
> 
> So that the * decoration is at least consistent.

Adam, we do no longer use Qt coding style in WebKit. WebKit style wins if there is any conflict between the two style guides.

[Eric Seidel (no email)]

Comment on attachment 30870 [details]
add a layout test to test the fix.

Cleared Holger Freyther's review+ from obsolete attachment 30870 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 31378 [details]
new patch to include recommended changed in the comments.

Cleared Adam Treat's review+ from obsolete attachment 31378 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 58290 [details]
Patch

Clearing flags on attachment: 58290

Committed r60970: <http://trac.webkit.org/changeset/60970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.