Bug 18819 – WebKit misplaces cursor on option/control-pageup/pagedown

• Component: Forms
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

The windows version of this bug is:
https://bugs.webkit.org/show_bug.cgi?id=18820

[Tony Chang]

*** Bug 21955 has been marked as a duplicate of this bug. ***

[Tony Chang]

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.

[Darin Adler]

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.

[Darin Adler]

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?

[Darin Adler]

The original bug says this is broken in <textarea>, but the bug fix has no effect on <textarea>. What's up?

[Tony Chang]

(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.

[Tony Chang]

Created attachment 49762 [details]
Patch

[Tony Chang]

(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.

[Eric Seidel (no email)]

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.

[Tony Chang]

Committed r55566: <http://trac.webkit.org/changeset/55566>

[Tony Chang]

*** Bug 18820 has been marked as a duplicate of this bug. ***