[webkit-reviews] review granted: [Bug 20303] [Qt] Key events are not working in frames. : [Attachment 31378] new patch to include recommended changed in the comments.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 18 09:44:25 PDT 2009
Adam Treat <treat at kde.org> has granted Yongjun Zhang
<yongjun.zhang at nokia.com>'s request for review:
Bug 20303: [Qt] Key events are not working in frames.
https://bugs.webkit.org/show_bug.cgi?id=20303
Attachment 31378: new patch to include recommended changed in the comments.
https://bugs.webkit.org/attachment.cgi?id=31378&action=review
------- Additional Comments from Adam Treat <treat at kde.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 44737)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,18 @@
> +2009-06-16 Yongjun Zhang <yongjun.zhang at nokia.com>
> +
> + 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 <jianli at chromium.org>
>
> 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 @@
> +2009-06-16 Yongjun Zhang <yongjun.zhang at nokia.com>
> +
> + 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 <dboddie at trolltech.com>
>
> 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 @@
> +2009-06-16 Yongjun Zhang <yongjun.zhang at nokia.com>
> +
> + 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 <bfulgham at gmail.com>
>
> 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 @@
> +2009-06-16 Yongjun Zhang <yongjun.zhang at nokia.com>
> +
> + 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 <xlopez at igalia.com>
>
> 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>
More information about the webkit-reviews
mailing list