Bug 19456 - [GTK] Editing capabilities
Summary: [GTK] Editing capabilities
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Christian Dywan
URL:
Keywords: Gtk
Depends on: 15616
Blocks: 35351
  Show dependency treegraph
 
Reported: 2008-06-09 19:56 PDT by Alp Toker
Modified: 2013-10-03 14:42 PDT (History)
8 users (show)

See Also:


Attachments
Implement webkit_web_view_execute_command and command-executed signal (5.14 KB, patch)
2009-12-18 03:09 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Implement webkit_web_view_execute_command, command-executed, catch internal commands (7.47 KB, patch)
2009-12-18 04:24 PST, Christian Dywan
gns: review-
Details | Formatted Diff | Diff
Implement webkit_web_view_execute_editor_command and editor-command-executed (11.84 KB, patch)
2011-02-16 06:26 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Implement webkit_web_view_execute_editor_command and editor-command-executed (13.89 KB, patch)
2011-02-16 06:31 PST, Christian Dywan
mrobinson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2008-06-09 19:56:57 PDT
We provide web_view_get/set_editable() but HTML composers need more convenience API which we need to support.
Comment 1 Alp Toker 2008-06-09 20:11:34 PDT
The Evolution email client's composer is a good place to look for inspiration.

The important things we can learn from it:

gtkhtml_editor_run_command()

^ For running editor commands. We have this in WebKit but the command strings may be different.

gtkhtml_editor_get_action()
gtkhtml_editor_get_ui_manager()

^ For hooking up UI actions directly to the composer.

gtkhtml_editor_get_html()
gtkhtml_editor_set_html()

^ For accessing the full content.

gtkhtml_editor_get_paragraph_data()
gtkhtml_editor_set_paragraph_data()

^ For accessing individual paragraph contents(?)

It will also be important to hook up Undo/Redo for editing, which is available in WebKit but not yet hooked up in the GTK+ port. Note the ability to group editing actions and label them in undo history.

Other things to note are the spell checking support and smiley conversion (we should make it easy for this kind of cute feature to be implemented directly in the app since it doesn't really belong in the widget).

Another interesting design decision is that the editor is a subclass of the HTML widget. It might be worth doing this kind of separation over WebView.

See also:
http://www.go-evolution.org/New_Composer

Grep of gtkhtml use in the composer:

gnome/evolution/composer$ grep gtkhtml_ *
e-composer-actions.c:	response = gtkhtml_editor_file_chooser_dialog_run (
e-composer-actions.c:	if (!gtkhtml_editor_get_changed (editor) &&
e-composer-actions.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-composer-actions.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-composer-actions.c:	filename = gtkhtml_editor_get_filename (editor);
e-composer-actions.c:	if (!gtkhtml_editor_save (editor, filename, TRUE, &error)) {
e-composer-actions.c:	gtkhtml_editor_run_command (GTKHTML_EDITOR (composer), "saved");
e-composer-actions.c:	response = gtkhtml_editor_file_chooser_dialog_run (
e-composer-actions.c:	gtkhtml_editor_set_filename (GTKHTML_EDITOR (composer), filename);
e-composer-actions.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-composer-actions.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-composer-actions.c:	manager = gtkhtml_editor_get_ui_manager (GTKHTML_EDITOR (composer));
e-composer-actions.h:	(gtkhtml_editor_get_action (GTKHTML_EDITOR (composer), (name)))
e-composer-autosave.c:	if (!gtkhtml_editor_get_changed (editor))
e-composer-autosave.c:	gtkhtml_editor_set_changed (editor, FALSE);
e-composer-private.c:	manager = gtkhtml_editor_get_ui_manager (GTKHTML_EDITOR (composer));
e-composer-private.c:	manager = gtkhtml_editor_get_ui_manager (GTKHTML_EDITOR (composer));
e-composer-private.c:	manager = gtkhtml_editor_get_ui_manager (editor);
e-msg-composer.c:		text = gtkhtml_editor_get_text_plain (editor, &length);
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "save-data-on");
e-msg-composer.c:		text = gtkhtml_editor_get_text_html (editor, &length);
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "save-data-off");
e-msg-composer.c:	gtkhtml_editor_set_text_html (GTKHTML_EDITOR (composer), body, -1);
e-msg-composer.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-msg-composer.c:		gtkhtml_editor_set_inline_spelling (editor, enable);
e-msg-composer.c:		gtkhtml_editor_set_magic_links (editor, enable);
e-msg-composer.c:		gtkhtml_editor_set_magic_smileys (editor, enable);
e-msg-composer.c:	gtkhtml_editor_set_changed (editor, TRUE);
e-msg-composer.c:	gtkhtml_editor_set_html_mode (GTKHTML_EDITOR (composer), active);
e-msg-composer.c:		language = gtkhtml_spell_language_lookup (language_code);
e-msg-composer.c:	gtkhtml_editor_set_spell_languages (
e-msg-composer.c:	gtkhtml_editor_run_command (GTKHTML_EDITOR (widget), "grab-focus");
e-msg-composer.c:		gtkhtml_editor_run_command (
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "orig");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "signature");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "orig");
e-msg-composer.c:	gtkhtml_editor_set_paragraph_data (editor, "orig", "0");
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "signature");
e-msg-composer.c:	if (gtkhtml_editor_is_paragraph_empty (editor))
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "signature" ,"0");
e-msg-composer.c:	else if (gtkhtml_editor_is_previous_paragraph_empty (editor) &&
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "cursor-backward")) {
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "signature", "0");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "cursor-forward");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:	if (!gtkhtml_editor_is_paragraph_empty (editor))
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "orig");
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "orig", "0");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "indent-zero");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "style-normal");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "insert-paragraph");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "delete-back");
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "signature");
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "signature", "0");
e-msg-composer.c:	html = gtkhtml_editor_get_html (editor);
e-msg-composer.c:		gtkhtml_editor_get_html (editor), stream,
e-msg-composer.c:	html = gtkhtml_editor_get_html (editor);
e-msg-composer.c:	manager = gtkhtml_editor_get_ui_manager (editor);
e-msg-composer.c:	gtkhtml_editor_set_changed (editor, FALSE);
e-msg-composer.c:	if (!gtkhtml_editor_search_by_data (editor, 1, "ClueFlow", "signature", "1"))
e-msg-composer.c:	data = gtkhtml_editor_get_paragraph_data (editor, "signature_name");
e-msg-composer.c:				gtkhtml_editor_set_html_mode (
e-msg-composer.c:				gtkhtml_editor_set_html_mode (
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "editable-off");
e-msg-composer.c:	gtkhtml_editor_set_changed (editor, FALSE);
e-msg-composer.c:	html_content = gtkhtml_editor_get_html_mode (editor);
e-msg-composer.c:	gtkhtml_editor_set_html_mode (GTKHTML_EDITOR (composer), FALSE);
e-msg-composer.c:	html_content = gtkhtml_editor_get_html_mode (editor);
e-msg-composer.c:	html_content = gtkhtml_editor_get_html_mode (editor);
e-msg-composer.c:	html_content = gtkhtml_editor_get_html_mode (editor);
e-msg-composer.c:	html = gtkhtml_editor_get_html (editor);
e-msg-composer.c:	gtkhtml_editor_freeze (editor);
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "cursor-position-save");
e-msg-composer.c:	gtkhtml_editor_undo_begin (editor, "Set signature", "Reset signature");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "block-selection");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "cursor-bod");
e-msg-composer.c:	if (gtkhtml_editor_search_by_data (editor, 1, "ClueFlow", "signature", "1")) {
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "select-paragraph");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "delete");
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "signature", "0");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "delete-back");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "unblock-selection");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "insert-paragraph");
e-msg-composer.c:		if (!gtkhtml_editor_run_command (editor, "cursor-backward"))
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "insert-paragraph");
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "cursor-forward");
e-msg-composer.c:		gtkhtml_editor_set_paragraph_data (editor, "orig", "0");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "indent-zero");
e-msg-composer.c:		gtkhtml_editor_run_command (editor, "style-normal");
e-msg-composer.c:		gtkhtml_editor_insert_html (editor, html_text);
e-msg-composer.c:	gtkhtml_editor_undo_end (editor);
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "cursor-position-restore");
e-msg-composer.c:	gtkhtml_editor_thaw (editor);
e-msg-composer.c:	text = gtkhtml_editor_get_text_plain (editor, &length);
e-msg-composer.c:	gtkhtml_editor_set_html_mode (editor, !alt);
e-msg-composer.c:	if (!gtkhtml_editor_is_paragraph_empty (editor)) {
e-msg-composer.c:		if (gtkhtml_editor_is_previous_paragraph_empty (editor))
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "cursor-backward");
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "italic-off");
e-msg-composer.c:			gtkhtml_editor_run_command (editor, "insert-paragraph");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "style-normal");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "indent-zero");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "text-default-color");
e-msg-composer.c:	gtkhtml_editor_run_command (editor, "italic-off");
Comment 2 Matthew Barnes 2008-06-27 19:29:15 PDT
(In reply to comment #1)
> gtkhtml_editor_get_paragraph_data()
> gtkhtml_editor_set_paragraph_data()
> 
> ^ For accessing individual paragraph contents(?)

I think this is for tagging paragraphs with metadata, such as whether it's a signature paragraph or not.  Evolution treats signature paragraphs special.


> Other things to note are the spell checking support and smiley conversion (we
> should make it easy for this kind of cute feature to be implemented directly 
> in the app since it doesn't really belong in the widget).

If WebKit could not try to implement spell checking itself but just provide a suitable virtual API to subclass and the ability to mark misspellings, that would be preferable.


> Another interesting design decision is that the editor is a subclass of the
> HTML widget. It might be worth doing this kind of separation over WebView.

To clarify, the HTML editor is just a GtkWindow and contains a GtkHTML instance.  Evolution's message composer is a subclass of the HTML editor window.  I anticipate swapping the GtkHTML instance for a WebView instance without changing the fundamental design.


Also, does WebKit have the capacity to convert HTML content to plain text and vice versa?  GtkHtml has such a converter but it may need some updating.
Comment 3 Christian Dywan 2009-12-18 03:09:13 PST
Created attachment 45136 [details]
Implement webkit_web_view_execute_command and command-executed signal

This adds a function to execute commands, such as to make text bold, move text and other things. I'm not sure what the best way to document it is. I guess the documentation should list commands... if you agree with the functionality I intend to look into documenting it.

Use cases:
Implement custom context menu
Keep a history of executed commands
Execute commands with custom key bindings
Prevent commands to restrict text editing
Comment 4 WebKit Review Bot 2009-12-18 03:13:59 PST
Attachment 45136 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:483:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.cpp:2117:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.cpp:3941:  webkit_web_view_execute_command is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:3948:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:360:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5
Comment 5 Christian Dywan 2009-12-18 04:24:54 PST
Created attachment 45143 [details]
Implement webkit_web_view_execute_command, command-executed, catch internal commands

I updated the patch since I noticed that commands coming from WebCore, ie. through Javascript were not handled at all. So I added a shouldExecuteCommand to EditorClient.

One remaining issue: for some reason internal commands have no command names.
Comment 6 WebKit Review Bot 2009-12-18 04:26:37 PST
Attachment 45143 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:109:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.cpp:2118:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.cpp:3946:  webkit_web_view_execute_command is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.h:360:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4
Comment 7 Gustavo Noronha (kov) 2009-12-29 09:48:33 PST
Comment on attachment 45143 [details]
Implement webkit_web_view_execute_command, command-executed, catch internal commands

> +#if PLATFORM(GTK)
> +     if (!m_frame->editor()->client()->shouldExecuteCommand(m_command->value (m_frame.get(), triggeringEvent)))
> +        return false;
> +#endif

Indentation seems off. I think we should not make this gtk-specific. Just add a call to shouldExecuteCommand, and make the default implementation for all ports except ours be return true;

> +
> +#if PLATFORM(GTK)
> +    virtual bool shouldExecuteCommand(const String&) = 0;
> +#endif

Same here. Just add the function, and make sure to add it to all the ports.

> +    /**
> +     * WebKitWebView::command-executed:

Perhaps we should name this editor-command, and get away with the 'executed' part? The command has not really been executed yet - it will be.

> +     * Since: 1.1.14

1.1.19, I guess =)

> +    webkit_web_view_signals[RESOURCE_REQUEST_STARTING] = g_signal_new("command-executed",

wrong index

> +/**
> + * webkit_web_view_execute_command:

I don't like 'execute_command'. It is too generic, and it's not really easy to figure out what we are talking about. I would prefer webkit_web_view_execute_editor_command, or editing_command.

> + * @web_view: a #WebKitWebView
> + * @command: a command string
> + *
> + * Executes the specified command, if possible.

What makes it not possible? We should document that the command will be executed in the focused frame.
Comment 8 Christian Dywan 2011-02-16 06:26:08 PST
Created attachment 82624 [details]
Implement webkit_web_view_execute_editor_command and editor-command-executed

(In reply to comment #7)
> Indentation seems off. I think we should not make this gtk-specific. Just add a call to shouldExecuteCommand, and make the default implementation for all ports except ours be return true;

I removed the platform guards now.

> Perhaps we should name this editor-command, and get away with the 'executed' part? The command has not really been executed yet - it will be.

I renamed the signal "editor-command" and documented that it is called *before* execution.

> > +    webkit_web_view_signals[RESOURCE_REQUEST_STARTING] = g_signal_new("command-executed",

Fixed.

> I don't like 'execute_command'. It is too generic, and it's not really easy to figure out what we are talking about. I would prefer webkit_web_view_execute_editor_command, or editing_command.

Renamed the function to execute_editor_command() taking two string arguments. The second string argument depends on the command, for example an URL for insertImage, createLink.

> > + * @web_view: a #WebKitWebView
> > + * @command: a command string
> > + *
> > + * Executes the specified command, if possible.
> 
> What makes it not possible? We should document that the command will be executed in the focused frame.

I gave insertImage and ToggleBold as examples of failure. It fails if there is no selection, because that is what the change applies to.

I listed useful commands there, the list is not complete and mostly based on most likely useful commands.
Comment 9 WebKit Review Bot 2011-02-16 06:27:49 PST
Attachment 82624 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:387:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:387:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:387:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:275:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Review Bot 2011-02-16 06:30:19 PST
Attachment 82624 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7917350
Comment 11 Christian Dywan 2011-02-16 06:31:35 PST
Created attachment 82625 [details]
Implement webkit_web_view_execute_editor_command and editor-command-executed

My apologies, the previous patch was not quite uptodate.
Comment 12 WebKit Review Bot 2011-02-16 06:32:35 PST
Attachment 82625 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Tools/GtkLauncher/main.c:238:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:242:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:387:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:387:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:387:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:275:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Early Warning System Bot 2011-02-16 06:36:06 PST
Attachment 82624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7926222
Comment 14 WebKit Review Bot 2011-02-16 06:41:16 PST
Attachment 82625 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7920279
Comment 15 Early Warning System Bot 2011-02-16 06:45:14 PST
Attachment 82625 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7927169
Comment 16 Build Bot 2011-02-16 07:07:39 PST
Attachment 82625 [details] did not build on win:
Build output: http://queues.webkit.org/results/7918303
Comment 17 WebKit Review Bot 2011-02-16 07:55:59 PST
Attachment 82625 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7927190
Comment 18 WebKit Review Bot 2011-02-16 09:13:21 PST
Attachment 82625 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7927207
Comment 19 Martin Robinson 2011-02-17 15:26:02 PST
Comment on attachment 82625 [details]
Implement webkit_web_view_execute_editor_command and editor-command-executed

r- for the build breaks and style issues. Do you mind checking if the editing delegate signals added not so long ago fulfill your requirements instead of adding a new signal?
Comment 20 Christian Dywan 2011-02-18 04:31:33 PST
(In reply to comment #19)
> (From update of attachment 82625 [details])
> r- for the build breaks and style issues. Do you mind checking if the editing delegate signals added not so long ago fulfill your requirements instead of adding a new signal?

"should-begin-editing"
"should-end-editing"
"should-insert-node"
"should-insert-text"
"should-delete-range"
"should-change-selected-range"
"should-apply-style"
"user-changed-contents"
"editing-ended"
"selection-changed"

All of them are key binding/ action signals, first wins. That means I can't intercept changes individually. And as far as I see, I can't see what changed, it only tells me that something changed. Please correct me if I'm wrong here.

Let's take an example: I want to prevent users from making text bold or italic. Control+B and Control+I allow that even if I don't provide any buttons. So I need a signal to catch the style change and prevent it. I still want to allow normal editing commands of course, so I can't use a first_wins signal.

I also don't want users to affect text I previously styled as a quotation, so I need to prevent colour changes. To be exact, cursor information would be useful for that which my proposed signal doesn't solve yet.

I would appreciate thoughts on solving these use cases.
Comment 21 Martin Robinson 2013-10-03 14:42:21 PDT
We have the ability to execute commands now (WebKit2, at least) and we have DOM bindings for complicated things. Perhaps we can reopen this when we have a better understanding of a usecase we don't support.