Bug 100937

Summary: [BlackBerry] DRT - eventSender.keyDown() needs to support pageUp, pageDown, home, end key
Product: WebKit Reporter: Xiaobo Wang <xiaobwang>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mifenton, mxie, rwlbuis, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
patch
rwlbuis: review-
patch - updated none

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.