Bug 139443 – [GTK] Add API to set the web view editable into WebKit2 GTK+ API

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

[Tomas Popela]

Add the webkit_web_view_set_editable() and webkit_web_view_get_editable() to set the view editable (user can change its content).

[Tomas Popela]

Created attachment 242912 [details]
Patch

[WebKit Commit Bot]

Attachment 242912 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:890:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:892:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:893:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:894:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:895:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 242912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242912&action=review

You should also add the new symbols to the documentation

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:885
> +     * information see #webkit_web_view_set_editable.

#webkit_web_view_set_editable -> webkit_web_view_set_editable()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:895
> +    g_object_class_install_property(gObjectClass,
> +                                    PROP_EDITABLE,
> +                                    g_param_spec_boolean("editable",
> +                                                         _("Editable"),
> +                                                         _("Whether the content can be modified by the user."),
> +                                                         FALSE,
> +                                                         WEBKIT_PARAM_READWRITE));

This should be indented following wk coding style.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3367
> + * it doesn't. You can change @web_view's document programmatically regardless of
> + * this setting.

You can change @web_view's document programmatically regardless of this setting. <- I don't understand this sentence

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3369
> + * Return value: a #gboolean indicating the editable state

Use Returns instead of Return value:

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3391
> + * elements. You can change @web_view's document programmatically regardless of
> + * this setting. By default a #WebKitWebView is not editable.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3394
> + * document are editable. This function provides a low-level way to make the

Why low-level?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3404
> +    if (editable == webkit_web_view_get_editable(webView))

You could use etPage(webView)->isEditable() directly here instead of the public method to avoid doing the g_return_if_fail again

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3410
> +    if (editable)
> +        getPage(webView)->applyEditingStyleToBodyElement();

Isn't setEditable() enough? shouldn't setEditable do this automatically?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1344
> +#if PLATFORM (GTK)
> +void WebPageProxy::setEditable(bool editable)

Why is this specific to GTK?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1362
> +    m_process->send(Messages::WebPage::ApplyEditingStyleToBodyElement(), m_pageID);

Can we get rid of this message and call this in the web process when setEditable is called with true? What happens if setEditable is called with false afterwards?

> Source/WebKit2/UIProcess/WebPageProxy.h:398
> +    void setEditable(bool);
> +    bool isEditable() const { return m_isEditable; };

I think should be either setIsEditable() and isEditable() or setEditable() and editable() I think

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:121
> +    test->setEditable(TRUE);

TRUE -> true

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:174
> +    test->setEditable(TRUE);

Ditto.

[Tomas Popela]

Created attachment 242930 [details]
Patch

[Tomas Popela]

(In reply to comment #3)
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3367
> > + * it doesn't. You can change @web_view's document programmatically regardless of
> > + * this setting.
> 
> You can change @web_view's document programmatically regardless of this
> setting. <- I don't understand this sentence

Changed to: "You can change @web_view's document through #WebKitWebInspector or #WebKitWebExtension regardless this setting."

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1362
> > +    m_process->send(Messages::WebPage::ApplyEditingStyleToBodyElement(), m_pageID);
> 

Removed this call and code about it as we don't want to set these defaults (like -webkit-nbsp-mode..).

[Carlos Garcia Campos]

Comment on attachment 242930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242930&action=review

API looks good to me, we need a WebKit2 owner to approve the cross-platform changes. Thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:632
> +

Remove this extra line.

[Tomas Popela]

Created attachment 242933 [details]
Patch

[Tomas Popela]

This needs to be reworked after http://trac.webkit.org/changeset/178536

[Carlos Garcia Campos]

(In reply to comment #8)
> This needs to be reworked after http://trac.webkit.org/changeset/178536

Yes, this is a lot easier now that we don't need to change the cross-platform code :-)

[Tomas Popela]

Created attachment 244754 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 244754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244754&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3533
> + * webkit_web_view_get_editable:

hmm, after reading the api again, I wonder if it would be better to use webkit_web_view_is_editable()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3542
> + * Returns whether the user is allowed to edit the document.
> + *
> + * Returns %TRUE if @web_view allows the user to edit the HTML document, %FALSE if
> + * it doesn't. You can change @web_view's document through #WebKitWebInspector or
> + * #WebKitWebExtension regardless this setting. By default a #WebKitWebView is not editable.
> + *
> + * Returns: a #gboolean indicating the editable state

There are 3 returns here :-) I would reorganiza this a bit. The last one is redundant so I would just remove it. I don't understand the part "You can change @web_view's document through #WebKitWebInspector or #WebKitWebExtension regardless this setting". I guess you mean that you can change the html so make the document editable. I think it's confusing, I wouldn't mention the inspector nor web extensions here. We could explain how this affect what the document contains, for example, what happens if the body tag contains contentEditable=True? does this method return true or false? (We could probably add a test case for that indeed).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3563
> + * If @editable is %TRUE, @web_view allows the user to edit the document. If
> + * @editable is %FALSE, an element in @web_view's document can only be edited if the
> + * CONTENTEDITABLE attribute has been set on the element or one of its parent
> + * elements. You can change @web_view's document through #WebKitWebInspector or

This looks better. We should add something similar to the getter, explaining that the document might still be editable if the conentEditable attr is set, even if the web view is not editable.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3564
> + * #WebKitWebExtension regardless this setting. By default a #WebKitWebView is not editable.

I wouldn't mention inspector and extensions here either.

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:265
> +    webkit_web_view_set_editable(m_webView, editable);

So, we are only testing the setter, but not the getter. We should check that by default is_editable returns FALSE as the doc says, and that after calling set_editable with TRUE, is_editable also return TRUE. It would be nice to check also that the web view can override the document, so that if body has contentEditable=false, making the view editable allows to edit the web view. So maybe it's better to add specific test cases for this, instead of reusing the selection ones.

[Tomas Popela]

(In reply to comment #11)
> hmm, after reading the api again, I wonder if it would be better to use
> webkit_web_view_is_editable()

That makes sense.

> There are 3 returns here :-) I would reorganiza this a bit. The last one is
> redundant so I would just remove it. I don't understand the part "You can
> change @web_view's document through #WebKitWebInspector or
> #WebKitWebExtension regardless this setting". I guess you mean that you can
> change the html so make the document editable.

That was meant to highlight that the content of the web view is not "readonly", so you can change it. But it makes sense to not have it there.

> We could explain how
> this affect what the document contains, for example, what happens if the
> body tag contains contentEditable=True? does this method return true or
> false? (We could probably add a test case for that indeed).

In the case that the editable property is set to FALSE and we will load the page with contenteditable set on body it will return FALSE. It could return TRUE only in case that the contenteditable is set on body. But no-one is doing that (other ports).

> So, we are only testing the setter, but not the getter. We should check that
> by default is_editable returns FALSE as the doc says, and that after calling
> set_editable with TRUE, is_editable also return TRUE. It would be nice to
> check also that the web view can override the document, so that if body has
> contentEditable=false, making the view editable allows to edit the web view.
> So maybe it's better to add specific test cases for this, instead of reusing
> the selection ones.

Ok, makes sense.

[Tomas Popela]

Created attachment 244882 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 244882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244882&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:219
> +    test->loadHtml(selectedSpanHTML, 0);

Use nullptr instad of 0

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:237
> +    g_assert(test->isEditable());
> +    test->setEditable(false);
> +    g_assert(!test->isEditable());

The WebView is destroyed when the test finishes so we don't need to reset the state.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:251
> +    test->loadHtml(selectedSpanHTML, 0);

nullptr.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:272
> +}

There's too much code duplicated here, I think we could try to move the common code to a helper function. We could use a format string for the selectedSpanHTML since the only difference is the true/false. We could also add EditorTest::cutSelection() that returns the selected text. That method would include the asserts to check the CUT and PASTE editing commands can be executed, the call to custClipboard and then it would return gtk_clipboard_wait_for_text().

[Tomas Popela]

(In reply to comment #14)
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:237
> > +    g_assert(test->isEditable());
> > +    test->setEditable(false);
> > +    g_assert(!test->isEditable());
> 
> The WebView is destroyed when the test finishes so we don't need to reset
> the state.

I'm resetting it on purpose to test whether setting the view as non-editable works.

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #14)
> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:237
> > > +    g_assert(test->isEditable());
> > > +    test->setEditable(false);
> > > +    g_assert(!test->isEditable());
> > 
> > The WebView is destroyed when the test finishes so we don't need to reset
> > the state.
> 
> I'm resetting it on purpose to test whether setting the view as non-editable
> works.

I see, maybe we should add a different test case for that, checking also that cut and paste is not allowed either.

[Tomas Popela]

Created attachment 244972 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 244972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244972&action=review

The implementation and the API looks good to me, we just need to improve a bit the tests.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:107
> -static void testWebViewEditorCutCopyPasteNonEditable(EditorTest* test, gconstpointer)
> +static void loadTestHtml(EditorTest* test, bool contentEditable)
>  {
> -    static const char* selectedSpanHTML = "<html><body contentEditable=\"false\">"
> +    GUniquePtr<char> selectedSpanHTML(g_strdup_printf(
> +        "<html><body contentEditable=\"%s\">"
>          "<span id=\"mainspan\">All work and no play <span id=\"subspan\">make Jack a dull</span> boy.</span>"
>          "<script>document.getSelection().collapse();\n"
>          "document.getSelection().selectAllChildren(document.getElementById('subspan'));\n"
> -        "</script></body></html>";
> +        "</script></body></html>", contentEditable ? "true" : "false"));
>  
> +    test->loadHtml(selectedSpanHTML.get(), nullptr);
> +    test->waitUntilLoadFinished();
> +}

This is too generic, what I meant is that we could make a global

static const char* selectedSpanHTMLFormat = "...."

And then in every test we use the format string to get the actual HTML depedning on whether we want the content editable or not

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:145
>  
> -    test->loadHtml(selectedSpanHTML, 0);
> +    g_assert(!test->isEditable());
> +    test->setEditable(true);
> +    g_assert(test->isEditable());
> +    test->loadHtml(selectedSpanHTML, nullptr);

Can't we use the common span HTML in this case?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:205
> +static void runEditorEditableCutTests(EditorTest* test, bool contentEditable)

What about something more descriptive like loadContentsAndTryToCutSelection?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:229
> +static void testWebViewEditorEditableOnNonEditable(EditorTest* test, gconstpointer)
> +{
> +    runEditorEditableCutTests(test, false);
> +}
> +
> +static void testWebViewEditorEditableOnContentEditable(EditorTest* test, gconstpointer)
> +{
> +    runEditorEditableCutTests(test, true);
> +}

These could be the same test.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:250
> +    EditorTest::add("WebKitWebView", "editable/non-editable", testWebViewEditorNonEditable);
> +    EditorTest::add("WebKitWebView", "editable/editable-on-non-editable", testWebViewEditorEditableOnNonEditable);
> +    EditorTest::add("WebKitWebView", "editable/editable-on-content-editable", testWebViewEditorEditableOnContentEditable);

I find the test names a bit confusing. What's the difference between non-editable and editable-on-non-editable? I guess the second one should be editable-on-non-editable-content?  Maybe we can even merge the 3 tests into a single one editable/editable and do all the checks there.

[Tomas Popela]

Created attachment 244976 [details]
Patch

[Tomas Popela]

Created attachment 244977 [details]
Patch

Rebased patch

[WebKit Commit Bot]

Comment on attachment 244977 [details]
Patch

Clearing flags on attachment: 244977

Committed r178703: <http://trac.webkit.org/changeset/178703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.