WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38213
Page up / page down moves edited content for the entire height
https://bugs.webkit.org/show_bug.cgi?id=38213
Summary
Page up / page down moves edited content for the entire height
Zelidrag Hornung
Reported
2010-04-27 13:23:00 PDT
Frame page up / page down scroll height calculation seems to be broken in EditorCommand.cpp. Function verticalScrollDistance() assumes that the scrolling distance for these operations is the entire client height instead of the visible portion of it. Repro Steps: 1. Load a Google Doc, or any other page hosting an iframe that has vertical scrollbar. 2. Select the frame content, then press Page Down to page down through the doc, one page at a time. Expect to page one page at a time. Instead, it the page scrolls almost the entire content height instead. Check out
http://crosbug.com/2562
for further details.
Attachments
Fix for scroll height calc for page up/down key handling in Editor
(10.88 KB, patch)
2010-04-27 18:16 PDT
,
Zelidrag Hornung
no flags
Details
Formatted Diff
Diff
Fix for scroll height calc for page up/down key handling in Editor
(10.91 KB, patch)
2010-04-27 18:24 PDT
,
Zelidrag Hornung
levin
: review-
Details
Formatted Diff
Diff
fixed mac link and behavior, other comments addressed
(12.18 KB, patch)
2010-04-30 16:24 PDT
,
Zelidrag Hornung
no flags
Details
Formatted Diff
Diff
fixed mac link and behavior, other comments addressed
(12.18 KB, patch)
2010-04-30 16:33 PDT
,
Zelidrag Hornung
no flags
Details
Formatted Diff
Diff
fixed mac link issues and scroll behavior, other comments addressed
(12.18 KB, patch)
2010-04-30 17:20 PDT
,
Zelidrag Hornung
no flags
Details
Formatted Diff
Diff
added additional test for testing visual viewport of scrolled frame
(
deleted
)
2010-05-03 11:59 PDT
,
Zelidrag Hornung
levin
: review-
levin
: commit-queue-
Details
Formatted Diff
Diff
Addressed the latest set of comments
(16.41 KB, patch)
2010-05-07 16:50 PDT
,
Zelidrag Hornung
levin
: review-
levin
: commit-queue-
Details
Formatted Diff
Diff
Review comment fixed, handling of non-iframe contenteditable cases
(26.41 KB, patch)
2010-05-14 13:06 PDT
,
Zelidrag Hornung
ojan
: review-
ojan
: commit-queue-
Details
Formatted Diff
Diff
latest comments addressed, merged cursor/scroll tests, ahem fonts...
(22.88 KB, patch)
2010-05-21 13:08 PDT
,
Zelidrag Hornung
ojan
: review+
ojan
: commit-queue-
Details
Formatted Diff
Diff
Changes m_frame->settings() check
(22.71 KB, patch)
2010-05-21 15:08 PDT
,
Zelidrag Hornung
no flags
Details
Formatted Diff
Diff
Patch
(122.72 KB, patch)
2010-05-23 22:32 PDT
,
Tony Chang
no flags
Details
Formatted Diff
Diff
Patch
(88.25 KB, patch)
2010-05-26 02:24 PDT
,
Tony Chang
ojan
: review+
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Ojan Vafai
Comment 1
2010-04-27 13:47:26 PDT
I don't see this on Chromium 5.0.391.0 (Developer Build 45722) on Mac. I don't see it in Google Docs and I don't see it on a test page that just has an iframe. Can you include more detailed steps? What browsers have you tested in? What versions?
Tony Chang
Comment 2
2010-04-27 17:18:14 PDT
A test case attached to the bug would be awesome.
Zelidrag Hornung
Comment 3
2010-04-27 17:31:32 PDT
I've seen this in 5.0.375.17 dev on Windows and it is also detected in the current tip of the trunk which we pulled into ChromeOS. Please take a look at the example in WK layout test at LayoutTests/editing/input/option-page-up-down.html Currently, this layout test seems to be rigged to pass around this bug.
Tony Chang
Comment 4
2010-04-27 17:53:22 PDT
You're right, it looks like we're moving the cursor to the wrong place (although we seem to be scrolling the right amount). I'll investigate.
Zelidrag Hornung
Comment 5
2010-04-27 17:56:45 PDT
Actually, we don't even scroll properly by page there (if the edit box is focused). I have a fix for this being prepared for the upload.
Zelidrag Hornung
Comment 6
2010-04-27 18:16:47 PDT
Created
attachment 54490
[details]
Fix for scroll height calc for page up/down key handling in Editor Fix for scroll height calc for page up/down key handling in Editor.
WebKit Review Bot
Comment 7
2010-04-27 18:21:23 PDT
Attachment 54490
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zelidrag Hornung
Comment 8
2010-04-27 18:24:50 PDT
Created
attachment 54491
[details]
Fix for scroll height calc for page up/down key handling in Editor removed tabs from ChangeLogs
Tony Chang
Comment 9
2010-04-27 18:57:26 PDT
Thanks for the patch! A few comments: - This doesn't link on Mac because WebCore::SelectionController::setSelection is in the list of exported symbols. I think you need to update WebCore.base.exp. - The change to setSelection is so you can scroll to iframe so the cursor is the first selected line. In TextEdit on mac, I don't see that behavior. In TextEdit, the frame scrolls so that the cursor is in the middle. This is probably a platform specific behavior. - The existing layout test doesn't really tell you how the view scrolled. We probably need to add a layout test to verify that we scrolled the right amount.
David Levin
Comment 10
2010-04-29 18:37:34 PDT
Comment on
attachment 54491
[details]
Fix for scroll height calc for page up/down key handling in Editor This looks like it is heading on a good path. I have some general comments and when they are done either I or someone else should verify the patch in greater detail. Ideally, the ChangeLog should have per function comments to explain why the change was done in that function. For example, I don't know why the code in verticalScrollDistance changed and the ChangeLog should explain this. When you find yourself adding a bool and then passing in true/false for it, it is a bad sign. It is especially bad if it is with a bunch of other bools. Please change the bool to an enum that describes the desired behavior (something like alignToTop or alignToEdgeIfNeeded?). Also, it looks like Tony had a bunch of good comments which should be addressed.
Zelidrag Hornung
Comment 11
2010-04-30 16:24:07 PDT
Created
attachment 54833
[details]
fixed mac link and behavior, other comments addressed - fixes WebCore::SelectionController::setSelection definition in WebCore.base.exp - made cursor align consistent with other apps on Mac - replaced cursor align control param with enum (instead of bool) in SelectionController::moveTo/setSelection and related calls - added better descriptions to ChangeLogs
Zelidrag Hornung
Comment 12
2010-04-30 16:33:41 PDT
Created
attachment 54834
[details]
fixed mac link and behavior, other comments addressed - fixes WebCore::SelectionController::setSelection definition in WebCore.base.exp - made cursor align consistent with other apps on Mac - replaced cursor align control param with enum (instead of bool) in SelectionController::moveTo/setSelection and related calls - added better descriptions to ChangeLogs - compared to previous patch, it has the correct scroll #if logic for Mac vs. the rest
Zelidrag Hornung
Comment 13
2010-04-30 17:20:08 PDT
Created
attachment 54838
[details]
fixed mac link issues and scroll behavior, other comments addressed - fixes WebCore::SelectionController::setSelection definition in WebCore.base.exp - made cursor align consistent with how other apps work on Mac - replaced cursor align control param with enum (instead of bool) in SelectionController::moveTo/setSelection and related calls - added better descriptions to ChangeLogs - compared to previous patches, this cl used #if OS(DARWIN) properly
Tony Chang
Comment 14
2010-05-01 07:51:06 PDT
(In reply to
comment #13
)
> Created an attachment (id=54838) [details]
I think the code changes look good. It would be great if you could add another test to verify the scroll behavior (center align vs top align). Alternately, maybe you could make the existing test check this. One way would be to use JS to read the scroll offset (maybe give it a range of pixels to be tolerant of font differences).
Zelidrag Hornung
Comment 15
2010-05-03 11:59:35 PDT
Created
attachment 54949
[details]
added additional test for testing visual viewport of scrolled frame
Tony Chang
Comment 16
2010-05-05 19:29:36 PDT
This LGTM, but I am not a reviewer.
David Levin
Comment 17
2010-05-06 10:23:38 PDT
Comment on
attachment 54949
[details]
added additional test for testing visual viewport of scrolled frame Unfortunately, this comment from last time was not addressed: Ideally, the ChangeLog should have per function comments to explain why the change was done in that function. For example, I don't know why the code in verticalScrollDistance changed and the ChangeLog should explain this. (Actually, I think I get it now....but the ChangeLog should still explain it.) Also if there were per function comments, it would hopefully tell me why a particular place became "align always" when before it was "align if needed". Note that if you were to say the same thing more than once in a row then "Ditto." suffices the second time.
> Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 58689) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,32 @@ > +2010-05-03 Zelidrag Hornung <
zelidrag@chromium.org
> > + > + Reviewed by NOBODY (OOPS!). > + > + Fixed frame page up/down scrolling calculation, made sure that the
It should be a . instead of a , You're doing three separate things in this patch. A well focused patch that only does one thing would be smaller, quicker to review, get committed faster, and most importantly reviewed better because of the focused nature. It also would help folks in the future to not have to understand what each part of this patch is doing (but once again per function comments in the ChangeLog woudl also help with this). It may take you slightly longer, but there are far more committers than reviewers, so it is nice to optimize for reviewer time. You don't need to break this one up at this point but it is something to keep in mind. (I'd guess that we would have had two of these committed at this point had that course been taken.)
> + cursor moves with page up/down as well. Please note that now OS(DARWIN) > + implementation will now center the center the cursor on page up/down
Is the cursor behavior tested anywhere? (And where is the mac vs other platforms behavior of the cursor tested?) Is there a test to verify that the cursor isn't moved (on OS(DARWIN)) when alt isn't pressed?
> + while other platforms will align the cursor with the top of displayed > + frame. > +
https://bugs.webkit.org/show_bug.cgi?id=38213
> + * editing/SelectionController.h: > + (WebCore::SelectionController::):
This should be a function name. prepare-ChangeLog messed this up but you should fix it ideally.
> Index: WebCore/editing/EditorCommand.cpp
Now that you have removed the one call to renderbox in this file, will it build with the "#include "RenderBox.h"" be removed?
> Index: WebCore/editing/SelectionController.h > =================================================================== > --- WebCore/editing/SelectionController.h (revision 58684) > +++ WebCore/editing/SelectionController.h (working copy) > @@ -45,6 +45,7 @@ class SelectionController : public Nonco > public: > enum EAlteration { MOVE, EXTEND }; > enum EDirection { FORWARD, BACKWARD, RIGHT, LEFT }; > + enum EScrollCursorAlignment { ALIGN_IF_NEEDED, ALIGN_ALWAYS };
The enum is oddly named with the "E" prefix. The enums themselves (while consistent with the other enums here) are not ConsistentWithTheStyleGuide. The enum names do not stand on their own: ALIGN_IF_NEEDED -- align what if needed? When I see it in the code, it looks like this SelectionController::ALIGN_IF_NEEDED. This makes one think that the selection or selection controller is being aligned with something.
> > SelectionController(Frame* = 0, bool isDragCaretController = false); > > @@ -54,14 +55,14 @@ public: > Node* shadowTreeRootNode() const { return m_selection.shadowTreeRootNode(); } > > void moveTo(const Range*, EAffinity, bool userTriggered = false); > - void moveTo(const VisiblePosition&, bool userTriggered = false); > + void moveTo(const VisiblePosition&, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED);
There is no need for the param to be named here ("cursorAlignment") because it adds no new information.
> + void setSelection(const VisibleSelection&, bool closeTyping = true, bool clearTypingStyle = true, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED, TextGranularity = CharacterGranularity);
Ditto.
> + bool modify(EAlteration, int verticalDistance, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED);
Ditto.
> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html
> +function runTest() { > + var tolerance = 10; > + var frame = frames[0]; > + var doc = frame.document; > + var body = doc.body; > + for (var i = 0; i < 10; ++i) > + body.innerHTML += "<p>line " + i + "</p>\n"; > + frame.focus(); > + frame.getSelection().setPosition(body.firstChild, 0); > + > + var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1; > + var modifiers = onMacPlatform ? ["altKey"] : []; > + > + if (onMacPlatform) > + offsets = [ 51, 159 ]; > + else > + offsets = [ 116, 218 ]; > + > + if (!window.eventSender) > + return; > + > + eventSender.keyDown("pageDown", modifiers); > + if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) { > + throw "Frame viewport should be around " + offsets[0] + > + "px , not at " + frame.pageYOffset; > + } > + > + eventSender.keyDown("pageDown", modifiers); > + var line = frame.getSelection().baseNode.nodeValue; > + if (Math.abs(frame.pageYOffset - offsets[1]) > tolerance) { > + throw "Frame viewport should be around " + offsets[1] + > + "px , not " + frame.pageYOffset; > + } > + > + eventSender.keyDown("pageUp", modifiers); > + var line = frame.getSelection().baseNode.nodeValue; > + if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) { > + throw "Frame viewport should be around " + offsets[0] + > + "px , not at " + frame.pageYOffset; > + } > + > + document.getElementById("results").innerText = "PASS"; > +} > +</script> > + > +<div>On Mac, option+pagedown should move the frame mouse cursor in the middle of the frame viewport in text areas. > +On other platforms, pagedown should move the mouse cursor at the top of the viewport while scroll in text areas.
This talks about the cursor but the test doesn't test the cursor at all from what I can see.
> +This test requires DRT to pass.</div>
Instead of saying that it requires the DRT to pass, ideally it would tell someone how to manually repro the test if possible.
Ojan Vafai
Comment 18
2010-05-06 14:25:45 PDT
Comment on
attachment 54949
[details]
added additional test for testing visual viewport of scrolled frame WebCore/editing/SelectionController.cpp:165 + #if OS(DARWIN) This should use settings->editingBehavior() == EditingMacBehavior. That way it's a runtime setting and once
https://bugs.webkit.org/show_bug.cgi?id=38603
is completed, we can change the setting on the fly in layout tests. eventSender.keyDown("pageDown", modifiers); if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) { throw "Frame viewport should be around " + offsets[0] + "px , not at " + frame.pageYOffset; } You don't have to, but if you want to make this test a bit more complete, you could also add verification that the selection is in the right place (e.g. it should be centered on mac except at the bounds of the scroll region).You can get the selection bounding box with window.getSelection().getBoundingClientRect().
David Levin
Comment 19
2010-05-06 14:53:01 PDT
(In reply to
comment #18
)
> (From update of
attachment 54949
[details]
) > You don't have to, but if you want to make this test a bit more complete, you > could also add verification that the selection is in the right place (e.g. it > should be centered on mac except at the bounds of the scroll region).
Actually, this appears to be part of the changes being done, so it should be tested as I noted above (and it should be trivial to test).
Zelidrag Hornung
Comment 20
2010-05-07 16:50:06 PDT
Created
attachment 55433
[details]
Addressed the latest set of comments Please note that test scroll-viewport-page-up-down.html covers platform differences in scroll view between mac and other platforms while option-page-up-down.html actually ensures that the cursor it on the same line on page up/down event regardless of the platform. * (re)added somehow dropped per-function comments into ChangeLog * renames SelectionController::CursorAlignOnScroll enum and its values to give more details about what they do * removed #include "RenderBox.h" from WebCore/editing/EditorCommand.cpp * renames cursorAlignment param * changed #if OS(DARWIN) to settings->editingBehavior() == EditingMacBehavior in SelectionController.cpp
Ojan Vafai
Comment 21
2010-05-07 17:16:47 PDT
Comment on
attachment 55433
[details]
Addressed the latest set of comments WebCore/editing/SelectionController.cpp:168 + if (m_frame->settings()->editingBehavior() == EditingMacBehavior) You should use the settings pointer you got in line 165. Also, you need to nullcheck settings here. WebCore/editing/EditorCommand.cpp:262 + int height = frame->view()->visibleHeight(); This doesn't look right to me. We're scrolling a renderer here that has overflow (i.e. the scrollbars are on that box, not the frame). Can you maybe add a case to your test (or another test) where you test scrolling in a div with overflow:auto? WebCore/editing/EditorCommand.cpp:260 + if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable())) This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here.
David Levin
Comment 22
2010-05-07 19:30:25 PDT
Comment on
attachment 55433
[details]
Addressed the latest set of comments I think you did some changes that didn't make it in this patch. Sorry if I sound like I'm repeating myself in these comments. I may have missed something so let me know if somehow I thought you should do something and you already did. Thanks!
> Index: WebCore/ChangeLog > +2010-05-07 Zelidrag Hornung <
zelidrag@chromium.org
> > + > + Reviewed by NOBODY (OOPS!). > + > + Fixed frame page up/down scrolling calculation. Made sure that the > + cursor moves with page up/down as well. Please note that now OS(DARWIN)
OS(DARWIN) doesn't match the code anymore.
> + implementation will now center the center the cursor on page up/down > + while other platforms will align the cursor with the top of displayed > + frame. > +
https://bugs.webkit.org/show_bug.cgi?id=38213
> + > + Test: editing/input/option-page-up-down.html (fixed) > + Test: editing/input/scroll-viewport-page-up-down.html (added) > + > + * WebCore.base.exp: > + * editing/EditorCommand.cpp: > + (WebCore::verticalScrollDistance): Fixed page scroll calculation. Now scroll height is calculated only from the visible portion not the entire frame height. > + (WebCore::executeMovePageDown): Now it can tell SelectionController to move the cursor with the page scroll up/down events.
> Index: WebCore/editing/SelectionController.cpp
> + if (userTriggered) { > + Settings* settings = m_frame ? m_frame->settings() : 0;
settings is unused.
> + ScrollAlignment alignment; > + if (m_frame->settings()->editingBehavior() == EditingMacBehavior) > + alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ? ScrollAlignment::alignCenterAlways : ScrollAlignment::alignCenterIfNeeded; > + else > + alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ? ScrollAlignment::alignTopAlways : ScrollAlignment::alignToEdgeIfNeeded; > + > + m_frame->revealSelection(alignment, true);
If m_frame may be 0, then this will crash. (You check for m_frame being 0 above.) I don't see how m_frame can be 0, since this is checked for on line 121 of "SelectionController::setSelection" and the code returns there. Also this code always has used m_frame without doing a 0 check before.
> Index: WebCore/editing/SelectionController.h > + enum CursorAlignOnScroll { ALIGN_CURSOR_ON_SCROLL_IF_NEEDED, > + ALIGN_CURSOR_ON_SCROLL_ALWAYS };
(I tried to mention this before but I must not have been clear... sorry.) Enum members should user InterCaps with an initial capital letter. In other words, it should look like this: enum CursorAlignOnScroll { AlignCursorOnScrollAlwaysIfNeeded, AlignCursorOnScrollAlways }; (You may wrap the line if you wish but you don't have to.)
> + void moveTo(const VisiblePosition&, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);
(I mentioned this before but I must not have made it clear what I meant.) Please don't include the param name "align" since it add no information. In other words, the line should look like this: void moveTo(const VisiblePosition&, bool userTriggered = false, CursorAlignOnScroll = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);
> + void setSelection(const VisibleSelection&, bool closeTyping = true, bool clearTypingStyle = true, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED, TextGranularity = CharacterGranularity);
Ditto.
> + bool modify(EAlteration, int verticalDistance, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);
Ditto.
> Index: LayoutTests/ChangeLog > +2010-05-07 Zelidrag Hornung <
zelidrag@chromium.org
> > + > + Reviewed by NOBODY (OOPS!). > + > + Fixed page up/down scrolling expectations for bug: > +
https://bugs.webkit.org/show_bug.cgi?id=38213
> + Added a new test that verifies how visible portion of frame changes > + while scrolling the frame. The test will check for different value on > + OS(DARWIN) than other platforms since edit frame scroll there will
OS(DARWIN) doesn't match the code anymore.
> + center the cursor on page up/down while it will align it with the top > + on other platforms. > + > + * editing/input/option-page-up-down.html: Fixed rigged test that was masking scrolling and cursor move errors. Added tests to make sure the cursor does not move on Mac if [option] is not pressed.
Thx! But I don't see that change in this patch?
> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html > +function runTest() { > + var tolerance = 10; > + var frame = frames[0]; > + var doc = frame.document; > + var body = doc.body; > + for (var i = 0; i < 10; ++i) > + body.innerHTML += "<p>line " + i + "</p>\n"; > + frame.focus(); > + frame.getSelection().setPosition(body.firstChild, 0); > + > + var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1; > + var modifiers = onMacPlatform ? ["altKey"] : []; > + > + if (onMacPlatform) > + offsets = [ 51, 159 ]; > + else > + offsets = [ 116, 218 ]; > + > + if (!window.eventSender) > + return; > + > + eventSender.keyDown("pageDown", modifiers); > + if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) { > + throw "Frame viewport should be around " + offsets[0] + > + "px , not at " + frame.pageYOffset; > + } > + > + eventSender.keyDown("pageDown", modifiers); > + var line = frame.getSelection().baseNode.nodeValue; > + if (Math.abs(frame.pageYOffset - offsets[1]) > tolerance) { > + throw "Frame viewport should be around " + offsets[1] + > + "px , not " + frame.pageYOffset; > + } > + > + eventSender.keyDown("pageUp", modifiers); > + var line = frame.getSelection().baseNode.nodeValue; > + if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) { > + throw "Frame viewport should be around " + offsets[0] + > + "px , not at " + frame.pageYOffset; > + } > + > + document.getElementById("results").innerText = "PASS"; > +} > +</script>
I don't see how this "ensures that the cursor [is] on the same line on page up/down event regardless of the platform. " >
> +<div>On Mac, option+pagedown should move the frame mouse cursor in the middle of the frame viewport in text areas. > +On other platforms, pagedown should move the mouse cursor at the top of the viewport while scroll in text areas.
As noted previously, I don't understand why this talks about the cursor moving when it doesn't seem to verify that.
> +This test requires DRT to pass.</div>
(As noted previously) Instead of saying that it requires the DRT to pass, ideally it would tell someone how to manually repro the test if possible.
Tony Chang
Comment 23
2010-05-09 17:50:56 PDT
(In reply to
comment #21
)
> (From update of
attachment 55433
[details]
) > WebCore/editing/EditorCommand.cpp:260 > + if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable())) > This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here.
This is correct. I added it to fix the bug where we weren't moving the cursor in contentEditable. The code reads: if the node is not contentEditable, then don't move the cursor (i.e., move the cursor 0 lines).
Tony Chang
Comment 24
2010-05-09 17:54:09 PDT
(In reply to
comment #23
)
> (In reply to
comment #21
) > > (From update of
attachment 55433
[details]
[details]) > > WebCore/editing/EditorCommand.cpp:260 > > + if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable())) > > This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here. > > This is correct. I added it to fix the bug where we weren't moving the cursor in contentEditable. The code reads: if the node is not contentEditable, then don't move the cursor (i.e., move the cursor 0 lines).
I'm sorry, my last comment was confusing. You're asking about contentEditable+overflow:visible. In that case, we should not return early because we do want to move the cursor (I think).
Zelidrag Hornung
Comment 25
2010-05-14 13:06:44 PDT
Created
attachment 56100
[details]
Review comment fixed, handling of non-iframe contenteditable cases - more tests added to cover non-iframe contenteditable scroll/cursor handling - addressed other review comments
David Levin
Comment 26
2010-05-18 15:52:46 PDT
Comment on
attachment 56100
[details]
Review comment fixed, handling of non-iframe contenteditable cases Thanks for your efforts and patiently addressing all of the feedback!
Ojan Vafai
Comment 27
2010-05-18 16:42:15 PDT
Comment on
attachment 56100
[details]
Review comment fixed, handling of non-iframe contenteditable cases I have a few things I'd like to see fixed. Putting my thoughts together now. I'll r+/r- appropriately with my comments.
David Levin
Comment 28
2010-05-18 16:46:21 PDT
Comment on
attachment 56100
[details]
Review comment fixed, handling of non-iframe contenteditable cases Switching back to r? for Ojan comments.
Ojan Vafai
Comment 29
2010-05-18 17:09:43 PDT
Comment on
attachment 56100
[details]
Review comment fixed, handling of non-iframe contenteditable cases Thanks for adding all the extra testing. Just a few more things before committing this. Thanks for being patient.
> Index: WebCore/editing/SelectionController.cpp > @@ -161,8 +161,15 @@ void SelectionController::setSelection(c > + if (userTriggered) { > + ScrollAlignment alignment; > + if (m_frame->settings()->editingBehavior() == EditingMacBehavior)
You're fine not null-checking m_frame, but I think settings() still needs a null-check. Specifically, if m_frame's m_page is nulled out, then settings() returns null.
> Index: LayoutTests/editing/input/option-page-up-down-inpage.html > Index: LayoutTests/editing/input/option-page-up-down.html
Can you combine these two into one test? See my example below for a similar case.
> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html > Index: LayoutTests/editing/input/scroll-viewport-page-up-down-div.html
I think you could combine these two into one test without much work. More importantly, I don't see what scroll-viewport-page-up-down-div.html tests that scroll-viewport-page-up-down.html doesn't. I see that there are extra contentEditable divs there, but they're not actually the ones getting scrolled. Also, these tests shouldn't need a fudge factor. If the issue is text rendering, you should be able to get around it by using the "ahem" font-family. That font is specifically for testing. Every character has the same width/height. How about something like this? I haven't actually ran this, so I'm sure I got some bits wrong. <div>Page up/down (option+page up/down on Mac) should move the move cursor and scroll the content in contenteditable elements. This sample covers scroll position test of iframe element containing contenteditable body. Even though the cursor will move exactly to the same location on all platforms (covered by test option-page-up-down.html), please note that Mac will scroll the visible area by placing the cursor position in the middle. All other platforms will scroll by keeping the cursor aligned with the top edge of the visible area. </div> <div id="editable" contenteditable="true" style="height:150px;overflow:auto;"></div> <iframe src="../resources/contenteditable-iframe-src.html"></iframe> <div id="results">FAIL</div> <script> if (window.layoutTestController) layoutTestController.dumpAsText(); function runTest(scrollable) { var tolerance = 10; for (var i = 0; i < 10; ++i) scrollable.innerHTML += "<p>line " + i + "</p>\n"; frame.focus(); frame.getSelection().setPosition(scrollable.firstChild, 0); var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1; var modifiers = onMacPlatform ? ["altKey"] : []; if (onMacPlatform) offsets = [ 51, 159 ]; else offsets = [ 116, 218 ]; if (!window.eventSender) return; eventSender.keyDown("pageDown", modifiers); if (Math.abs(scrollable.scrollTop - offsets[0]) > tolerance) { throw "Frame viewport should be around " + offsets[0] + "px , not at " + scrollable.scrollTop; } eventSender.keyDown("pageDown", modifiers); var line = frame.getSelection().baseNode.nodeValue; if (Math.abs(scrollable.scrollTop - offsets[1]) > tolerance) { throw "Frame viewport should be around " + offsets[1] + "px , not " + scrollable.scrollTop; } eventSender.keyDown("pageUp", modifiers); var line = frame.getSelection().baseNode.nodeValue; if (Math.abs(scrollable.scrollTop - offsets[0]) > tolerance) { throw "Frame viewport should be around " + offsets[0] + "px , not at " + scrollable.scrollTop; } document.getElementById("results").innerText += "PASS<br>"; } runTest(document.getElementById('editable')); runTest(frames[0].document.body); </script>
> +<body onload="runTest()"> > +<iframe src="../resources/contenteditable-iframe-src.html" onload="runTest()"></iframe>
This isn't relevant if you change the test as I suggested above, but for future reference, there is a race condition here. runTest() can get called twice before the test finishes.
Zelidrag Hornung
Comment 30
2010-05-21 13:08:40 PDT
Created
attachment 56740
[details]
latest comments addressed, merged cursor/scroll tests, ahem fonts... * added check for m_frame->settings() * merged iframe and div tests into the same files * ahem-ized fonts used across these tests
WebKit Commit Bot
Comment 31
2010-05-21 13:10:41 PDT
Comment on
attachment 56740
[details]
latest comments addressed, merged cursor/scroll tests, ahem fonts... Rejecting patch 56740 from review queue.
zelidrag@chromium.org
does not have reviewer permissions according to
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py
. - If you do not have reviewer rights please read
http://webkit.org/coding/contributing.html
for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). Due to
bug 30084
the commit-queue will require a restart after your change. Please contact
eseidel@chromium.org
to request a commit-queue restart. After restart the commit-queue will correctly respect your reviewer rights.
Zelidrag Hornung
Comment 32
2010-05-21 13:12:36 PDT
Comment on
attachment 56740
[details]
latest comments addressed, merged cursor/scroll tests, ahem fonts... wrong flags
Ojan Vafai
Comment 33
2010-05-21 13:31:13 PDT
Comment on
attachment 56740
[details]
latest comments addressed, merged cursor/scroll tests, ahem fonts... r=me with the change below. Thanks for being patient with all this. WebCore/editing/SelectionController.cpp:174 + if (macStyleScroll) If settings is null, that means we're in a state where it doesn't matter what editing behavior we use (e.g. a page transition). The null check is just to avoid crashing. So you can get rid of the macStyleScroll variable and just do something like this: if (m_frame->settings() && m_frame->settings()->editingBehavior() == EditingMacBehavior)
Zelidrag Hornung
Comment 34
2010-05-21 15:08:10 PDT
Created
attachment 56750
[details]
Changes m_frame->settings() check changed m_frame->settings() check per latest comment.
WebKit Commit Bot
Comment 35
2010-05-23 04:45:26 PDT
Comment on
attachment 56750
[details]
Changes m_frame->settings() check Rejecting patch 56750 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: tree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 18294 test cases. editing/input/option-page-up-down.html -> failed Exiting early after 1 failures. 3778 tests run. 53.45s total testing time 3777 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output:
http://webkit-commit-queue.appspot.com/results/2362077
Tony Chang
Comment 36
2010-05-23 22:22:14 PDT
I ran the tests on my mac. The two new tests both failed because they need an extra blank line in the end of the results. However, platform/mac/editing/selection/25228.html also fails, but as mentioned in the test, this is probably expected. The test expects us to scroll such that the cursor is barely exposed, but this change causes the cursor to be centered. This seems to match TextEdit behavior.
Tony Chang
Comment 37
2010-05-23 22:32:45 PDT
Created
attachment 56847
[details]
Patch
Tony Chang
Comment 38
2010-05-24 00:13:29 PDT
I've uploaded a diff with the updated test expectations. While the new results for platform/mac/editing/selection/25228.html are correct, but it makes the test no longer test what it's supposed to test. I tried to come up with a new test that tests
r42583
, but I'm having a hard time.
Ojan Vafai
Comment 39
2010-05-25 17:34:08 PDT
Comment on
attachment 56847
[details]
Patch I find the horizontal centering to be a bit weird, but TextEdit does it and it's better than the old behavior of aligning to the left. I would r+ except for platform/mac/editing/selection/25228.html. I feel like we should either fix it so it's actually testing what it claims to or delete the test. The only thing I can think of for testing this would be to do the following: 1. Use the "ahem" font-family so that character widths are fixed. I think they're all 15px. 2. Make the div wide (e.g. 1000px) 3. Put the cursor in the middle of the first line. 4. Move the cursor backward one character. 5. Assert that there is a specific scrollLeft value on the body. If
http://trac.webkit.org/changeset/42583
ever regressed, the scrollLeft would change. It's harder than the old test to verify correctness visually, but at least should keep testing that code path and we'll know if it ever changes.
Tony Chang
Comment 40
2010-05-26 02:24:57 PDT
Created
attachment 57081
[details]
Patch
Tony Chang
Comment 41
2010-05-26 02:26:12 PDT
I've updated the test such that rolling out
r42583
will cause platform/mac/editing/selection/25228.html to fail.
Ojan Vafai
Comment 42
2010-05-26 06:42:25 PDT
Comment on
attachment 57081
[details]
Patch Thanks for following up on this.
Tony Chang
Comment 43
2010-05-26 21:18:25 PDT
Committed
r60268
: <
http://trac.webkit.org/changeset/60268
>
WebKit Review Bot
Comment 44
2010-05-26 21:55:08 PDT
http://trac.webkit.org/changeset/60268
might have broken GTK Linux 32-bit Release
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug