Bug 47426

Summary:

[Qt] Editing commands should not be executed on non-editable content.

Product:

WebKit

Reporter:

Yael <yael>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ademar, commit-queue, ossy, staikos, tonikitoo

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

46905

Attachments:

Description	Flags
Patch	none
Pat	none
Patch	tonikitoo: review+
Patch	none

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	Details Formatted Diff Diff
Pat (5.04 KB, patch) 2010-10-08 14:33 PDT, Yael	no flags	Details Formatted Diff Diff
Patch (5.35 KB, patch) 2010-10-08 15:16 PDT, Yael	tonikitoo: review+	Details Formatted Diff Diff
Patch (5.31 KB, patch) 2010-10-12 07:32 PDT, Yael	no flags	Details Formatted Diff Diff
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"> My span </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.