Bug 20099 - [GTK] SHIFT+PAGE_UP/DOWN doesn't extend selection
Summary: [GTK] SHIFT+PAGE_UP/DOWN doesn't extend selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Adrien Nader
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2008-07-18 13:49 PDT by Adrien Nader
Modified: 2008-08-27 18:42 PDT (History)
0 users

See Also:


Attachments
Trivial patch changing call to frame->selection()->modify instead of frame->editor()->command (1.09 KB, patch)
2008-07-18 13:55 PDT, Adrien Nader
no flags Details | Formatted Diff | Diff
new version with the proper granularity (simpler btw) (988 bytes, patch)
2008-07-22 10:48 PDT, Adrien Nader
sam: review-
Details | Formatted Diff | Diff
Same as before plus the changelog entry (1.41 KB, patch)
2008-08-22 10:17 PDT, Adrien Nader
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien Nader 2008-07-18 13:49:50 PDT
Using SHIFT and UP/DOWN or HOME/END correctly extends selection but not with PAGE_UP/PAGE_DOWN.
There's no bug, just no implementation.

I'm attaching a trivial patch which is basically an adaptation of the code for up/down which is written just before.
Comment 1 Adrien Nader 2008-07-18 13:55:52 PDT
Created attachment 22374 [details]
Trivial patch changing call to frame->selection()->modify instead of frame->editor()->command

A fairly simple patch which is basically a copy of what was written for the handling of up/down.

I'm not sure the proper granularity is ParagraphGranularity though.
Comment 2 Pierre-Luc Beaudoin 2008-07-22 07:51:05 PDT
Based on GEdit's behavior, ParagraphGranularity isn't ok.  But considering that Firefox doesn't implement this, it might be ok.
Comment 3 Adrien Nader 2008-07-22 10:48:59 PDT
Created attachment 22431 [details]
new version with the proper granularity (simpler btw)

The granulariy is now correct. The proper thing was already available but I was not aware of its existence. The patch is also simpler.

Basically this patch changes
  "MovePageUp"
to
  kevent->shiftKey() ? "MovePageUpAndModifySelection" : "MovePageUp"
Comment 4 Sam Weinig 2008-07-26 14:55:18 PDT
Comment on attachment 22431 [details]
new version with the proper granularity (simpler btw)

This looks ok, but it needs a changelog, and depending on the state of DRT for the GTK port a regression test as well.
Comment 5 Christian Dywan 2008-07-26 15:06:14 PDT
The new lines are indentend with tabs, but you should only use spaces in WebCore. When you address Sam's comments, please fix that as well. Otherwise the patch works as expected from a little testing.
Comment 6 Adrien Nader 2008-08-22 10:17:52 PDT
Created attachment 22942 [details]
Same as before plus the changelog entry

Finally updated the patch with a changelog and spaces (damn default configuration !).
Comment 7 Eric Seidel (no email) 2008-08-27 16:14:15 PDT
Comment on attachment 22942 [details]
Same as before plus the changelog entry

I'm assuming that it's impossible to test this with DumpRenderTreeGtk.  Looks sane enough.
Comment 8 Holger Freyther 2008-08-27 18:42:27 PDT
Looked sane and landed in r35958.