Bug 100937 - [BlackBerry] DRT - eventSender.keyDown() needs to support pageUp, pageDown, home, end key
Summary: [BlackBerry] DRT - eventSender.keyDown() needs to support pageUp, pageDown, h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-01 02:46 PDT by Xiaobo Wang
Modified: 2012-11-09 04:55 PST (History)
5 users (show)

See Also:


Attachments
patch (3.70 KB, patch)
2012-11-01 03:26 PDT, Xiaobo Wang
rwlbuis: review-
Details | Formatted Diff | Diff
patch - updated (4.51 KB, patch)
2012-11-09 00:10 PST, Xiaobo Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaobo Wang 2012-11-01 02:46:36 PDT
These keys are used in existing Layout tests, we should support them.
Comment 1 Xiaobo Wang 2012-11-01 03:26:11 PDT
Created attachment 171805 [details]
patch
Comment 2 Xiaobo Wang 2012-11-01 03:30:36 PDT
The following tests will be unblocked by this patch.
editing/input/scroll-viewport-page-up-down.html
editing/selection/move-begin-end.html
fast/forms/select-popup-pagekeys.html
Comment 3 Rob Buis 2012-11-01 05:06:28 PDT
Comment on attachment 171805 [details]
patch

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

> Tools/ChangeLog:8
> +        1. Translate these keys to their corresponding BlackBerry key code.

Agreed.

> Tools/ChangeLog:9
> +        2. Break down a KeyChar event to a KeyDown/KeyUp pair.

Why is that needed? What does it fix?
Comment 4 Rob Buis 2012-11-07 15:27:46 PST
Comment on attachment 171805 [details]
patch

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

> Tools/ChangeLog:10
> +

Also you should state the tests this fixes.
Comment 5 Xiaobo Wang 2012-11-08 23:42:49 PST
Comment on attachment 171805 [details]
patch

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

>> Tools/ChangeLog:9
>> +        2. Break down a KeyChar event to a KeyDown/KeyUp pair.
> 
> Why is that needed? What does it fix?

It's because we only handle scrolling on a KeyDown event in WebPage::keyEvent(), and internally a KeyChar event is interpreted as a KeyDown event + a KeyUp event, in InputHandler::handleKeyboardInput().

>> Tools/ChangeLog:10
>> +
> 
> Also you should state the tests this fixes.

Yes, will add the test list here.
Comment 6 Xiaobo Wang 2012-11-09 00:10:57 PST
Created attachment 173220 [details]
patch - updated
Comment 7 Rob Buis 2012-11-09 04:35:23 PST
Comment on attachment 173220 [details]
patch - updated

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

LGTM.

> Tools/ChangeLog:12
> +        KeyUp event in InputHandler::handleKeyboardInput().

No PR?

> Tools/DumpRenderTree/blackberry/EventSender.cpp:185
> +    page->keyEvent(BlackBerry::Platform::KeyboardEvent(charCode, BlackBerry::Platform::KeyboardEvent::KeyUp, modifiers));

We should be considering using BlackBerry::Platform::KeyboardEvent; to get rid of the namespaces. You'd need just KeyboardEvent then.
Comment 8 WebKit Review Bot 2012-11-09 04:55:05 PST
Comment on attachment 173220 [details]
patch - updated

Clearing flags on attachment: 173220

Committed r134052: <http://trac.webkit.org/changeset/134052>
Comment 9 WebKit Review Bot 2012-11-09 04:55:08 PST
All reviewed patches have been landed.  Closing bug.