Bug 19465

Created attachment 21605 [details] Test Case To reproduce the bug, place the cursor at the very end of the first line of text (after the '?'). Then press the 'down' key repeatedly to try to move the cursor to the last line in the textarea. The behavior I observe is that the cursor gets to the line with the '3. ' and then pressing down has no effect. The cursor effectively gets stuck at that line. Pressing the right arrow will move it to the next line and then the down arrow will begin to work again.

aha, one potentially important piece of information: In QtWebKit, when you press down repeatedly and then get stuck at the "3. " line, if you then press the right arrow key and then the left arrow key, an ASSERT will be violated: Starting program: /home/jonathon/Projects/webkit.git/WebKitBuild/Debug/bin/QtLauncher [Thread debugging using libthread_db enabled] [New Thread 0xb4fbb720 (LWP 21709)] Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0xb4fbb720 (LWP 21709)] 0xb70770f4 in WebCore::VisiblePosition::previous (this=0xbfbc7084, stayInEditableContent=true) at ../../../WebCore/editing/VisiblePosition.cpp:95 95 ASSERT(inSameLine(temp, prev)); 90 // we should always be able to make the affinity DOWNSTREAM, because going previous from an 91 // UPSTREAM position can never yield another UPSTREAM position (unless line wrap length is 0!). 92 if (prev.isNotNull() && m_affinity == UPSTREAM) { 93 VisiblePosition temp = prev; 94 temp.setAffinity(UPSTREAM); 95 ASSERT(inSameLine(temp, prev)); 96 } 97 #endif 98 99 if (!stayInEditableContent) #0 0xb70770f4 in WebCore::VisiblePosition::previous (this=0xbfbc7084, stayInEditableContent=true) at ../../../WebCore/editing/VisiblePosition.cpp:95 #1 0xb705b663 in WebCore::SelectionController::modifyMovingBackward ( this=0xbfbc7250, granularity=WebCore::CharacterGranularity) at ../../../WebCore/editing/SelectionController.cpp:439 #2 0xb7060aa5 in WebCore::SelectionController::modify (this=0xbfbc7250, alter=WebCore::SelectionController::MOVE, dir=WebCore::SelectionController::BACKWARD, granularity=WebCore::CharacterGranularity, userTriggered=false) at ../../../WebCore/editing/SelectionController.cpp:515 #3 0xb70606ad in WebCore::SelectionController::modify (this=0x8122d58, alter=WebCore::SelectionController::MOVE, dir=WebCore::SelectionController::BACKWARD, granularity=WebCore::CharacterGranularity, userTriggered=true) at ../../../WebCore/editing/SelectionController.cpp:479 #4 0xb7028aab in executeMoveBackward (frame=0x8122a48) at ../../../WebCore/editing/EditorCommand.cpp:530 #5 0xb7026e42 in WebCore::Editor::Command::execute (this=0xbfbc7658, parameter=@0xbfbc7670, triggeringEvent=0x0) at ../../../WebCore/editing/EditorCommand.cpp:1371 #6 0xb7402f12 in QWebPage::triggerAction (this=0x8121a78, action=QWebPage::MoveToPreviousChar, checked=false) at ../../../WebKit/qt/Api/qwebpage.cpp:1373 #7 0xb73fee6c in QWebPagePrivate::keyPressEvent (this=0x8121ca0, ev=0xbfbc7bc4) at ../../../WebKit/qt/Api/qwebpage.cpp:534 #8 0xb73ff9ca in QWebPage::event (this=0x8121a78, ev=0xbfbc7bc4) at ../../../WebKit/qt/Api/qwebpage.cpp:1675 #9 0xb74058e8 in QWebView::keyPressEvent (this=0x8121750, ev=0xbfbc7bc4) at ../../../WebKit/qt/Api/qwebview.cpp:710 #10 0xb5b2d6f4 in QWidget::event () from /usr/lib/libQtGui.so.4 #11 0xb7405cee in QWebView::event (this=0x8121750, e=0xbfbc7bc4) at ../../../WebKit/qt/Api/qwebview.cpp:539 #12 0xb5ad5c0c in QApplicationPrivate::notify_helper () from /usr/lib/libQtGui.so.4 #13 0xb5adaaec in QApplication::notify () from /usr/lib/libQtGui.so.4 #14 0xb58a36a9 in QCoreApplication::notifyInternal () from /usr/lib/libQtCore.so.4 #15 0xb5b3122e in ?? () from /usr/lib/libQtGui.so.4 #16 0xb5b65eeb in ?? () from /usr/lib/libQtGui.so.4 #17 0xb5b67fce in ?? () from /usr/lib/libQtGui.so.4 #18 0xb5b404e5 in QApplication::x11ProcessEvent () from /usr/lib/libQtGui.so.4 #19 0xb5b693ba in ?? () from /usr/lib/libQtGui.so.4 #20 0xb5283bf8 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0 #21 0xb5286e5e in ?? () from /usr/lib/libglib-2.0.so.0 #22 0xb52873ac in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0 #23 0xb58cef98 in QEventDispatcherGlib::processEvents () from /usr/lib/libQtCore.so.4 #24 0xb5b691b5 in ?? () from /usr/lib/libQtGui.so.4 #25 0xb58a292d in QEventLoop::processEvents () from /usr/lib/libQtCore.so.4 #26 0xb58a2abd in QEventLoop::exec () from /usr/lib/libQtCore.so.4 #27 0xb58a4d3d in QCoreApplication::exec () from /usr/lib/libQtCore.so.4 #28 0xb5ad5567 in QApplication::exec () from /usr/lib/libQtGui.so.4 #29 0x0805809b in main (argc=Cannot access memory at address 0x0 ) at /home/jonathon/Projects/webkit.git/WebKit/qt/QtLauncher/main.cpp:314 The program is running. Exit anyway? (y or n)

See also: bug 13736. Note that I couldn't reproduce this bug on the Mac with the attached test case, but it may have the same cause as bug 13736 anyway.

Created attachment 21625 [details] potential fix for the stuck cursor Here's a potential fix. It solves the keyboard navigation issue on both Qt and Gtk ports. It basically duplicates the logic of the 'if (x < box->m_x + box->m_width)' block by only using UPSTREAM affinity if the offset is greater than 0. If this fix is deemed correct, it might be worth testing to see if it has an impact on bug 13736 as well.

Comment on attachment 21625 [details] potential fix for the stuck cursor The fix might be right -- it's hard for me to tell. But we need a test case. We require test cases for all bug fixes.

working on the test case. Any opinions on the right place to put it?

Created attachment 21642 [details] Updated patch for stuck cursor with test case Adds a layout test. Note that this is the first test i've ever created for webkit, so I may have missed something or put it in the wrong spot. Feel free to correct me if that's the case.

Comment on attachment 21642 [details] Updated patch for stuck cursor with test case Thanks! This looks good. Normally, we require that the patch includes the output of the test was well as the input. So there should be files with "expected" in their names in the patch. You generate those by running run-webkit-tests. Also, we try to design the tests so they are self explanatory. Someone should be able to tell what successful results look like. Further, we often make the tests "text only" so they can be used cross-platform. That's done by making sure the actual text of the result reflects success (so a dump of the render tree is not necessary) and adding a call to layoutTestController.dumpAsText. This tells the test engine to dump only the text rather than the render tree. There are many examples to show how this is done. One is editing/selection/anchor-focus1.html -- just to pick out a specific example. By the way, I'm not absolutely sure you need the runEditingTest() machinery. I think that sets up some things you really don't need if you're testing input in a textarea. You'd have to find a different way to do moveSelectionForwardBySentenceCommand() and moveSelectionForwardByLineCommand() if you didn't use editing.js -- I think you could just make keyboard events for arrow keys for those. It's not good to use the "by sentence" in the test, since that's very platform dependent. It would be better to move to the end of the line than to move forward by a sentence. Maybe the easiest thing would be to move forward by a line and then backward by a character? review- because this doesn't include expected test results, but please consider my other comments too.

Created attachment 21659 [details] Patch for stuck cursor with updated test case Here's another attempt. Thanks for you guidance on this. I modified the test to use layoutTestController.dumpAsText() and I removed the call to move forward by sentence. Instead I just set the initial selection partway through the first line of text (it's only important that it be past the end of the numbered lines that follow. As I'm not particularly familiar with the test infrastructure, I wasn't exactly sure what to expect for output of the dumpAsText() command. The output before the patch is 6 lines of 'EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification', and after the patch it's 8 lines (since the cursor position no longer gets suck at the end of '3. '). So it seems that the output is adequate to detect the broken behavior. I did not remove the runEditingTest() infrastructure from the test as I did not know a way to send down-arrow key events without it (I'm afraid I'm not particularly familiar with javascript). If you can easily replace it with something simpler, feel free, but I thought I'd leave it how it is since it seems to work.

Comment on attachment 21659 [details] Patch for stuck cursor with updated test case I think the test would be better if it got the value of textarea.selectionStart at the end of the test and wrote it out. Depending on the editing delegate values is too subtle. In fact, I'd prefer not to log the editing delegate messages -- but I can't remember what turns that logging on! Maybe it's something about being inside the editing directory? + layoutTestController.dumpAsText(); The normal idiom for this is to check for the existence of layoutTestController first, before using it, with if (window.layoutTestController). Your version will fail with an exception when it hits that line of code when run in the browser. Also, the test needs to be more self documenting. It should indicate what successful output looks like. I'm going to say review+, but feel free to make another pass at this with a better test.

Created attachment 21680 [details] Further improvements to the test case for stuck cursor bug Hopefully this is the last revision :) The test case should hopefully be a bit more self-explanatory now, and I've made it output 'success' if the test passed and if it failed, it prints out the current cursor position and what it should have been. I couldn't figure out how to disable the editing delegat messages. If you can figure it out, feel free to modify the patch. Thanks for your help

Comment on attachment 21680 [details] Further improvements to the test case for stuck cursor bug r=me

Landed in r34735.

Sadly, this did not fix Bug 13736, which seems to be a different issue.