Bug 62743

Summary: Support logical line movement in vertical writing mode
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, dglazkov, enrica, hyatt, mitz, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 55481    
Bug Blocks: 49408, 62833    
Attachments:
Description Flags
work in progress
none
work in progress (with tests)
none
fixes the bug
darin: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 none

Description Ryosuke Niwa 2011-06-15 12:03:26 PDT
document.execCommand('move', 'forward'/'backward', 'line') should work in vertical writing writing mode.
Comment 1 Ryosuke Niwa 2011-06-15 12:35:40 PDT
Created attachment 97343 [details]
work in progress

It turned out that when moving across p's (-2.html tests), we can't use lineTop for x coordinate for the reasons explained in my patch for the bug 55481.
Comment 2 Ryosuke Niwa 2011-06-15 12:37:45 PDT
Created attachment 97344 [details]
work in progress (with tests)

Oops, forgot to add tests.
Comment 3 Ryosuke Niwa 2011-06-16 16:49:09 PDT
Created attachment 97520 [details]
fixes the bug
Comment 4 Ryosuke Niwa 2011-06-16 16:52:51 PDT
This bug is about fixing modify('move', 'forward'/'backward', 'line'), not about arrow keys navigations.  That's tracked by the bug 62833 and blocked by this bug.

The code change itself is pretty trivial; the patch is big because I've added many tests and renamed some functions.
Comment 5 Darin Adler 2011-06-16 17:16:17 PDT
Comment on attachment 97520 [details]
fixes the bug

It would make this patch easier to read if the renaming was done in a separate preparation step. The part of this code I am not sure about is absoluteLineDirectionPointToLocalPointInBlock.
Comment 6 WebKit Review Bot 2011-06-16 17:19:39 PDT
Comment on attachment 97520 [details]
fixes the bug

Attachment 97520 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8879373

New failing tests:
editing/selection/vertical-rl-ltr-extend-line-backward-p.html
editing/selection/vertical-rl-rtl-extend-line-forward-p.html
editing/selection/vertical-rl-ltr-extend-line-forward-br.html
editing/selection/vertical-rl-rtl-extend-line-backward-br.html
editing/selection/vertical-rl-rtl-extend-line-forward-br.html
editing/selection/vertical-rl-ltr-extend-line-forward-wrap.html
editing/selection/vertical-rl-rtl-extend-line-backward-p.html
editing/selection/vertical-rl-ltr-extend-line-forward-p.html
editing/selection/vertical-lr-ltr-extend-line-backward-br.html
editing/selection/vertical-lr-ltr-extend-line-forward-br.html
editing/selection/vertical-rl-ltr-extend-line-backward-br.html
editing/selection/vertical-rl-ltr-extend-line-backward-wrap.html
Comment 7 WebKit Review Bot 2011-06-16 17:19:45 PDT
Created attachment 97525 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Ryosuke Niwa 2011-06-16 17:21:36 PDT
Thanks for a review!

(In reply to comment #5)
> (From update of attachment 97520 [details])
> It would make this patch easier to read if the renaming was done in a separate preparation step. The part of this code I am not sure about is absoluteLineDirectionPointToLocalPointInBlock.

Yeah, I don't like that function either.  I rewrote it a couple of times to get it feel right but it's still ugly :(  I'm more than happy to do some refactoring in follow up patches.
Comment 9 Ryosuke Niwa 2011-06-16 17:23:22 PDT
Of course, these tests require platform-specific results so I'll stick around on #webkit and rebaseline as needed.
Comment 10 Ryosuke Niwa 2011-06-16 17:25:48 PDT
Committed r89091: <http://trac.webkit.org/changeset/89091>