Bug 19456 – [GTK] Editing capabilities

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alp Toker]

We provide web_view_get/set_editable() but HTML composers need more convenience API which we need to support.

[Alp Toker]

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");

[Matthew Barnes]

(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.

[Christian Dywan]

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

[WebKit Review Bot]

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

[Christian Dywan]

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.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

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.

[Christian Dywan]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 82624 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7917350

[Christian Dywan]

Created attachment 82625 [details]
Implement webkit_web_view_execute_editor_command and editor-command-executed

My apologies, the previous patch was not quite uptodate.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 82624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7926222

[WebKit Review Bot]

Attachment 82625 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7920279

[Early Warning System Bot]

Attachment 82625 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7927169

[Build Bot]

Attachment 82625 [details] did not build on win:
Build output: http://queues.webkit.org/results/7918303

[WebKit Review Bot]

Attachment 82625 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7927190

[WebKit Review Bot]

Attachment 82625 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7927207

[Martin Robinson]

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?

[Christian Dywan]

(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.

[Martin Robinson]

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.