Summary: | WebKit misplaces cursor on option/control-pageup/pagedown | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
Component: | Forms | Assignee: | Tony Chang <tony> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | adele, darin, enrica, jon | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-04-30 17:49:30 PDT
The windows version of this bug is: https://bugs.webkit.org/show_bug.cgi?id=18820 *** Bug 21955 has been marked as a duplicate of this bug. *** 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 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 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?
The original bug says this is broken in <textarea>, but the bug fix has no effect on <textarea>. What's up? (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. Created attachment 49762 [details]
Patch
(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 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.
Committed r55566: <http://trac.webkit.org/changeset/55566> *** Bug 18820 has been marked as a duplicate of this bug. *** |