Bug 47426 - [Qt] Editing commands should not be executed on non-editable content.
Summary: [Qt] Editing commands should not be executed on non-editable content.
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: Qt
Depends on:
Blocks: 46905
  Show dependency treegraph
Reported: 2010-10-08 11:18 PDT by Yael
Modified: 2010-10-15 07:04 PDT (History)
5 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 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.
Comment 1 Yael 2010-10-08 12:11:14 PDT
Created attachment 70274 [details]

Remove the calls to editor()->command() when not in editable mode.
Comment 2 Yael 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.
Comment 3 Yael 2010-10-08 12:16:48 PDT
Not a Spatial Navigation issue, but it impacts the testing of Spatial Navigation.
Comment 4 Yael 2010-10-08 13:20:09 PDT
Example page that shows the problem:
<input type="radio" id="myradiobutton">
<span tabindex=1 id="r">My span</span>

Click the span, and after that, it is not possible to scroll.
Comment 5 Yael 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>
Comment 6 Yael 2010-10-08 14:33:41 PDT
Created attachment 70289 [details]

Added test.
In an e-mail exchange with the originator of this code, he was ok with removing it.
Comment 7 Yael 2010-10-08 15:16:24 PDT
Created attachment 70298 [details]

Modified the test to check that any scrolling happened instead of exactly 40 pixels. Different platforms may have different settings.
Comment 8 Antonio Gomes 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.
Comment 9 Antonio Gomes 2010-10-12 07:15:01 PDT
Comment on attachment 70298 [details]

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.
Comment 10 Yael 2010-10-12 07:32:57 PDT
Created attachment 70530 [details]
Comment 11 Yael 2010-10-12 07:33:52 PDT
Comment on attachment 70530 [details]

Removed the line about not having tests, now that we do have a test.
Comment 12 WebKit Commit Bot 2010-10-12 07:47:38 PDT
Comment on attachment 70530 [details]

Clearing flags on attachment: 70530

Committed r69582: <http://trac.webkit.org/changeset/69582>
Comment 13 Ademar Reis 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>