RESOLVED FIXED 47426
[Qt] Editing commands should not be executed on non-editable content. 
https://bugs.webkit.org/show_bug.cgi?id=47426
Summary [Qt] Editing commands should not be executed on non-editable content.
Yael
Reported 2010-10-08 11:18:53 PDT
EditorClientQt is trying to execute edit commands even if it is not in editable content. It is not clear to me why this is necessary. e.g. clicking somewhere (not editable content) in the page can generate a selection start. Based on existence of selection start we call editing commands and consume the key events, but then we can't scroll. This scenario is not very predictable so it would be difficult to provide a good layout test. I ran all editing layout tests with and without this change, and did not observe any difference in the results.
Attachments
Patch (2.45 KB, patch)
2010-10-08 12:11 PDT, Yael 		no flags
DetailsFormatted DiffDiff
Pat (5.04 KB, patch)
2010-10-08 14:33 PDT, Yael 		no flags
DetailsFormatted DiffDiff
Patch (5.35 KB, patch)
2010-10-08 15:16 PDT, Yael 		tonikitoo: review+
DetailsFormatted DiffDiff
Patch (5.31 KB, patch)
2010-10-12 07:32 PDT, Yael 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yael
Comment 1 2010-10-08 12:11:14 PDT
Created attachment 70274 [details] Patch Remove the calls to editor()->command() when not in editable mode.
Yael
Comment 2 2010-10-08 12:15:08 PDT
SVN blame seems to indicate that this code was originally added in r23545. Would be great to know if this is still needed.
Yael
Comment 3 2010-10-08 12:16:48 PDT
Not a Spatial Navigation issue, but it impacts the testing of Spatial Navigation.
Yael
Comment 4 2010-10-08 13:20:09 PDT
Example page that shows the problem: <html> <style> </style> <body> <input type="radio" id="myradiobutton"> <br><br><br><br><br><br><br><br><br><br> <span tabindex=1 id="r">My span</span> <br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br> </body> </html> Click the span, and after that, it is not possible to scroll.
Yael
Comment 5 2010-10-08 13:25:30 PDT
Callstack was asked for this issue: 0 WebCore::EditorClientQt::handleKeyboardEvent EditorClientQt.cpp 460 0x01e26134 1 WebCore::Editor::handleKeyboardEvent Editor.cpp 125 0x018a454d 2 WebCore::EventHandler::defaultKeyboardEventHandler EventHandler.cpp 2369 0x01b3f838 3 WebCore::Node::defaultEventHandler Node.cpp 2964 0x0182572b 4 WebCore::Node::dispatchGenericEvent Node.cpp 2693 0x018239de 5 WebCore::Node::dispatchEvent Node.cpp 2577 0x018231c8 6 WebCore::EventTarget::dispatchEvent EventTarget.cpp 278 0x01808c63 7 WebCore::EventHandler::keyEvent EventHandler.cpp 2310 0x01b3f06e 8 QWebPagePrivate::keyPressEvent qwebpage.cpp 945 0x01e59396 9 QWebPage::event qwebpage.cpp 2856 0x01e61ca2 10 QWebView::keyPressEvent qwebview.cpp 1033 0x01e673fe 11 QWidget::event qwidget.cpp 8222 0x03723a03 12 QWebView::event qwebview.cpp 834 0x01e66eba 13 QApplicationPrivate::notify_helper qapplication.cpp 4396 0x036b67b6 14 QApplication::notify qapplication.cpp 3857 0x036b42f9 15 QCoreApplication::notifyInternal qcoreapplication.cpp 732 0x0439e62f 16 QCoreApplication::sendSpontaneousEvent qcoreapplication.h 218 0x036b9023 17 qt_sendSpontaneousEvent qapplication.cpp 5388 0x036b6b27 18 QKeyMapper::sendKeyEvent qkeymapper_x11.cpp 1867 0x0378f030 19 QKeyMapperPrivate::translateKeyEvent qkeymapper_x11.cpp 1837 0x0378ed2c 20 QApplication::x11ProcessEvent qapplication_x11.cpp 3429 0x03757d6b ... <More>
Yael
Comment 6 2010-10-08 14:33:41 PDT
Created attachment 70289 [details] Pat Added test. In an e-mail exchange with the originator of this code, he was ok with removing it.
Yael
Comment 7 2010-10-08 15:16:24 PDT
Created attachment 70298 [details] Patch Modified the test to check that any scrolling happened instead of exactly 40 pixels. Different platforms may have different settings.
Antonio Gomes
Comment 8 2010-10-12 06:54:15 PDT
(In reply to comment #6) > Created an attachment (id=70289) [details] > Pat > > Added test. > In an e-mail exchange with the originator of this code, he was ok with removing it. The code clean up proposed generally looks good. I am just double checking with Yael if there is not any case not checked by the LayoutTests in Qt's DRT that we might be missing, for example Document::inDesignMode cases.
Antonio Gomes
Comment 9 2010-10-12 07:15:01 PDT
Comment on attachment 70298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70298&action=review > WebKit/qt/ChangeLog:10 > + A layout test would be very frigile, as the bad behavior is not consistent. Please lets remove this line since we now have a layout test.
Yael
Comment 10 2010-10-12 07:32:57 PDT
Created attachment 70530 [details] Patch
Yael
Comment 11 2010-10-12 07:33:52 PDT
Comment on attachment 70530 [details] Patch Removed the line about not having tests, now that we do have a test.
WebKit Commit Bot
Comment 12 2010-10-12 07:47:38 PDT
Comment on attachment 70530 [details] Patch Clearing flags on attachment: 70530 Committed r69582: <http://trac.webkit.org/changeset/69582>
Ademar Reis
Comment 13 2010-10-15 07:04:18 PDT
Revision r69582 cherry-picked into qtwebkit-2.1 with commit a5266b9 <http://gitorious.org/webkit/qtwebkit/commit/a5266b9>
Note You need to log in before you can comment on or make changes to this bug.