WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
139443
[GTK] Add API to set the web view editable into WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=139443
Summary
[GTK] Add API to set the web view editable into WebKit2 GTK+ API
Tomas Popela
Reported
2014-12-09 04:31:17 PST
Add the webkit_web_view_set_editable() and webkit_web_view_get_editable() to set the view editable (user can change its content).
Attachments
Patch
(16.24 KB, patch)
2014-12-09 05:37 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(15.31 KB, patch)
2014-12-09 09:16 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(15.31 KB, patch)
2014-12-09 09:43 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(11.22 KB, patch)
2015-01-16 01:29 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(15.65 KB, patch)
2015-01-19 01:20 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(16.42 KB, patch)
2015-01-20 00:18 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(17.87 KB, patch)
2015-01-20 02:20 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Patch
(17.89 KB, patch)
2015-01-20 02:33 PST
,
Tomas Popela
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Tomas Popela
Comment 1
2014-12-09 05:37:52 PST
Created
attachment 242912
[details]
Patch
WebKit Commit Bot
Comment 2
2014-12-09 05:39:09 PST
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 3
2014-12-09 05:50:49 PST
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
Comment 4
2014-12-09 09:16:08 PST
Created
attachment 242930
[details]
Patch
Tomas Popela
Comment 5
2014-12-09 09:17:27 PST
(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 6
2014-12-09 09:21:45 PST
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
Comment 7
2014-12-09 09:43:58 PST
Created
attachment 242933
[details]
Patch
Tomas Popela
Comment 8
2015-01-15 22:41:05 PST
This needs to be reworked after
http://trac.webkit.org/changeset/178536
Carlos Garcia Campos
Comment 9
2015-01-15 22:48:39 PST
(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
Comment 10
2015-01-16 01:29:05 PST
Created
attachment 244754
[details]
Patch
Carlos Garcia Campos
Comment 11
2015-01-16 01:50:09 PST
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
Comment 12
2015-01-16 02:30:01 PST
(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
Comment 13
2015-01-19 01:20:50 PST
Created
attachment 244882
[details]
Patch
Carlos Garcia Campos
Comment 14
2015-01-19 02:37:42 PST
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
Comment 15
2015-01-19 02:55:58 PST
(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
Comment 16
2015-01-19 02:59:41 PST
(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
Comment 17
2015-01-20 00:18:33 PST
Created
attachment 244972
[details]
Patch
Carlos Garcia Campos
Comment 18
2015-01-20 01:15:12 PST
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
Comment 19
2015-01-20 02:20:53 PST
Created
attachment 244976
[details]
Patch
Tomas Popela
Comment 20
2015-01-20 02:33:56 PST
Created
attachment 244977
[details]
Patch Rebased patch
WebKit Commit Bot
Comment 21
2015-01-20 03:22:01 PST
Comment on
attachment 244977
[details]
Patch Clearing flags on attachment: 244977 Committed
r178703
: <
http://trac.webkit.org/changeset/178703
>
WebKit Commit Bot
Comment 22
2015-01-20 03:22:05 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug