Bug 18819 - WebKit misplaces cursor on option/control-pageup/pagedown
Summary: WebKit misplaces cursor on option/control-pageup/pagedown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
: 18820 21955 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-30 17:49 PDT by Eric Seidel (no email)
Modified: 2010-03-04 20:45 PST (History)
4 users (show)

See Also:


Attachments
v1 (4.55 KB, patch)
2010-03-01 00:16 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (4.52 KB, patch)
2010-03-01 16:14 PST, Tony Chang
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-04-30 17:49:30 PDT
WebKit misplaces cursor on option/control-pageup/pagedown

1.  Type a bunch of text into a text area.
2.  use control (or option) + page down or page up to scroll the text area

Expected:
This should behave identical to TextEdit: move the caret, as well as placing the caret in the center of the scrolled region.  Instead, WebKit only scrolls the text area enough so that the caret is visible.  it does not scroll to center the caret.

This is the Mac version of this bug.  Our behavior is also wrong on Windows (but different).  I'll file that in a minute.
Comment 1 Eric Seidel (no email) 2008-04-30 17:53:26 PDT
The windows version of this bug is:
https://bugs.webkit.org/show_bug.cgi?id=18820
Comment 2 Tony Chang 2010-02-28 21:44:06 PST
*** Bug 21955 has been marked as a duplicate of this bug. ***
Comment 3 Tony Chang 2010-03-01 00:16:35 PST
Created attachment 49710 [details]
v1

Specifically, we were ignoring the scroll unless there was an overflow style set or if it was a text area.  This fixes option+pageup/down for contenteditable.
Comment 4 Darin Adler 2010-03-01 14:00:36 PST
Comment on attachment 49710 [details]
v1

> -    if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || renderer->isTextArea()))
> +    if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || renderer->isTextArea() || focusedNode->isContentEditable()))

There is no comment here explaining why this code change is correct.

And, further, I think this code change is wrong. If we want the editing meaning of page-down or option-page-down to win out over the scrolling meaning of the same equivalent, that may be fine, but we should *not* do it by returning 0 from verticalScrollDistance when there is a editable node focused. That's too low a level for this sort of policy decision. I don't immediately know which level should make this tradeoff, but this is slightly too low.
Comment 5 Darin Adler 2010-03-01 14:02:13 PST
Comment on attachment 49710 [details]
v1

Hmm, maybe I was too quick to state it's wrong. I don't understand, though, why this would be correct in all editable content. For one thing, the isTextArea test is redundant once we have the editable check. For another, I'm not sure we want this in  <input type=text>.

Did you test this key equivalent in plain old <input type=text> to make sure it still works as expected?
Comment 6 Darin Adler 2010-03-01 14:02:52 PST
The original bug says this is broken in <textarea>, but the bug fix has no effect on <textarea>. What's up?
Comment 7 Tony Chang 2010-03-01 16:11:36 PST
(In reply to comment #6)
> The original bug says this is broken in <textarea>, but the bug fix has no
> effect on <textarea>. What's up?

The bug description is wrong, it option+pageup/down only doesn't work for some contentedtiable cases.  Here's a test case with a input type=text, a textarea, a contenteditable div, and an iframe with a contenteditable body.  option+pagedown works in the first three, but not the last case.

http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%3E%0A%3Cinput%20type%3D%22text%22%20value%3D%22foo%20bar%20betz%20foo%20bar%20betz%22%20%2F%3E%0A%3C%2Fdiv%3E%0A%0A%3Cdiv%3E%20%20%0A%3Ctextarea%3E%0Aline1%0Aline2%20%0Aline3%20%0Aline4%0Aline5%0Aline6%0Aline7%0A%3C%2Ftextarea%3E%0A%0A%3Cdiv%20contentEditable%3D%22true%22%20style%3D%22overflow%3A%20auto%3B%20height%3A50px%3B%20width%3A%20150px%22%3E%0Aline1%3Cbr%2F%3E%0Aline2%3Cbr%2F%3E%0Aline3%3Cbr%2F%3E%20%0Aline4%3Cbr%2F%3E%0Aline5%3Cbr%2F%3E%0Aline6%3Cbr%2F%3E%0Aline7%3Cbr%2F%3E%0A%3C%2Fdiv%3E%0A%0A%3Ciframe%20%20style%3D'height%3A50px'%20src%3D%22data%3Atext%2Fhtml%2C%3Cbody%20contenteditable%3Dtrue%3Eline1%3Cbr%3Eline2%3Cbr%3Eline3%3Cbr%3Eline4%3Cbr%3Eline5%3Cbr%3Eline6%3C%2Fdiv%3E%22%3E%3C%2Fiframe%3E%0A&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

I verified that this doesn't change the behavior in <input type="text"> because apparently, they are not considered contenteditable.

You're right that we no longer need the check for isTextArea, isContentEditable covers that case.  I will upload a new patch.
Comment 8 Tony Chang 2010-03-01 16:14:14 PST
Created attachment 49762 [details]
Patch
Comment 9 Tony Chang 2010-03-01 16:20:26 PST
(In reply to comment #4)
> (From update of attachment 49710 [details])
> > -    if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || renderer->isTextArea()))
> > +    if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || renderer->isTextArea() || focusedNode->isContentEditable()))
> 
> There is no comment here explaining why this code change is correct.
> 
> And, further, I think this code change is wrong. If we want the editing meaning
> of page-down or option-page-down to win out over the scrolling meaning of the
> same equivalent, that may be fine, but we should *not* do it by returning 0
> from verticalScrollDistance when there is a editable node focused. That's too
> low a level for this sort of policy decision. I don't immediately know which
> level should make this tradeoff, but this is slightly too low.

Oh, I think maybe you're reading the code wrong. If a contenteditable node is focused, we were previously returning 0, and with the patch, we now return the amount that we already scrolled.  The return value is used to update the selection, which wasn't happening before.  Scrolling happens regardless of what this function returns.
Comment 10 Eric Seidel (no email) 2010-03-04 13:14:32 PST
Comment on attachment 49762 [details]
Patch

OK.  I think historically others have split out platform-specific bits into separate tests and check them into platform/mac or whatever.

But this is OK.
Comment 11 Tony Chang 2010-03-04 20:34:15 PST
Committed r55566: <http://trac.webkit.org/changeset/55566>
Comment 12 Tony Chang 2010-03-04 20:45:59 PST
*** Bug 18820 has been marked as a duplicate of this bug. ***