Bug 80981 – [GTK] [WK2] Add javascript clipboard functionality settings to WebKit2 GTK+ API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Antaryami Pandia (apandia)]

Add dom paste setting to WebKit2 GTK+ API

[Antaryami Pandia (apandia)]

Created attachment 131591 [details]
Patch.

Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 131591 [details]
Patch.

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:854
> +     * Whether to enable DOM paste. If set to %TRUE, document.execCommand("Paste")
> +     * will correctly execute and paste content of the clipboard.

What's missing here is some context as to why this is disabled by default and why one would want to enable it.

[Carlos Garcia Campos]

Comment on attachment 131591 [details]
Patch.

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

Patch looks good to me in general, but I'm a bit confusing about what dom paste is reading the gtk-doc comments.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:851
> +     * WebKitSettings:enable-dom-paste

Trailing : missing here.

>> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:854
>> +     * Whether to enable DOM paste. If set to %TRUE, document.execCommand("Paste")
>> +     * will correctly execute and paste content of the clipboard.
> 
> What's missing here is some context as to why this is disabled by default and why one would want to enable it.

What is dom paste? Does this mean if this setting is disabled "Paste" editing command doesn't work? What does "correctly" means in this case? why this setting guarantees the command is executed correctly.

[Antaryami Pandia (apandia)]

Thanks for the feedback.

> What's missing here is some context as to why this is disabled by default and why one would want to enable it.

The "m_isDOMPasteAllowed" settings (webcore settings which is set by this websttings -enable-dom-paste) is initialized false. Also in webkit1 this is disabled by default.

> Patch looks good to me in general, but I'm a bit confusing about what dom paste is reading the gtk-doc comments.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:851
> > +     * WebKitSettings:enable-dom-paste
> 
> Trailing : missing here.

Will do.
 
> What is dom paste? Does this mean if this setting is disabled "Paste" editing command doesn't work? 

the enable-dom-paste is used to set the "m_isDOMPasteAllowed". Webcore use this flag to see if dome paste is enabled or not while executing document.execCommand("Paste"). Method supportedPaste() in EditorCommand.cpp. 

> What does "correctly" means in this case? why this setting guarantees the command is executed correctly.

I have used the comment used in webkit1 webkitwebsettings. The main purpose of the flag is to enable dom paste. Please let me know if I should change the comment.

[Antaryami Pandia (apandia)]

Created attachment 131798 [details]
Updated patch

1. Added the missing trailing :
2. Modify the comment to reflect the purpose of the websettings.

[Carlos Garcia Campos]

Comment on attachment 131798 [details]
Updated patch

So this setting only affects to Paste command when invoked from JavaScript or DOM bindings API? That could be clarified in the documentation. I think it would be interesting to test not only the settings API but the effect of enabling/disabling this setting, adding a test case to TestEditor.cpp. There are not DOM bindings in WebKit2, so the name dom paste could be confusing.

[Antaryami Pandia (apandia)]

(In reply to comment #7)
> (From update of attachment 131798 [details])
> So this setting only affects to Paste command when invoked from JavaScript or DOM bindings API? That could be clarified in the documentation.

yes, it executes document.execCommand("Paste"). Will modify the comments accordingly. Also I think "successfully execute" should be more appropriate then "correctly execute" in comments in earlier patch (patch1).

> I think it would be interesting to test not only the settings API but the effect of enabling/disabling this setting, adding a test case to TestEditor.cpp

Sorry, but I didn't find "TestEditor.cpp" in my workspace.

>  There are not DOM bindings in WebKit2, so the name dom paste could be confusing.

webcore also uses the term dom paste and the same is also used in wk1. can it be only "Paste", "-enable-paste-from-java-script". Please suggest if you have any better name for it.

[Carlos Garcia Campos]

(In reply to comment #8)

> Sorry, but I didn't find "TestEditor.cpp" in my workspace.

Sorry, I meant Source/WebKit2/UIProcess/API/gtk/tests/TestWebViewEditor.cpp

[Antaryami Pandia (apandia)]

Aww. Turned out I goofed up. Actually "Dom paste" is dependent on another settings "javascript-can-access-clipboard" which is required for cut and copy functionality. (actually i have another workspace where it was hardcoded to true and I created patch in another workspace.) Next time I will be more careful and my sincere apologies for the half-backed change.

In webkit1 there were 2 different settings for cut/copy(javascript-can-access-clipboard) and paste(enable-dom-paste).

So do we required both the settings or we can have a single settings, say "enable-javascript-cutcopypaste" or any better name? If yes should I add the code change for "javascript-can-access-clipboard", in the same patch for this issue or log a new bug.

Also I will add tests to cove this as suggested by carlos.

[Antaryami Pandia (apandia)]

Just add a little more info, the Clipboard functionality "paste" is covered by -enable-dom-paste and the "cut/copy" is covered by "javascript-can-access-clipboard". And I think I should have cover the "cut/copy" funtionality first.

[Antaryami Pandia (apandia)]

Created attachment 131823 [details]
WIP

WIP Patch for early feedback.
Working on to add the tests in TestWebViewEditor.cpp

[Antaryami Pandia (apandia)]

Created attachment 131837 [details]
test page

test page to test java script clipboard functionality.

If I enable the web settings it passes the test works as expected and fails if the websettings is disabled.

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 131798 [details])
> So this setting only affects to Paste command when invoked from JavaScript or DOM bindings API? That could be clarified in the documentation. I think it would be interesting to test not only the settings API but the effect of enabling/disabling this setting, adding a test case to TestEditor.cpp. There are not DOM bindings in WebKit2, so the name dom paste could be confusing.

The DOM you access from JavaScript is actually a type of DOM binding. I think "DOM" or something like it is important here to disassociate this from other pastes, which this does not affect.

What I'm really missing from this patch is why anyone would want to enable or disable the DOM paste setting. Is it for security reasons? Is it nonstandard? I think someone without WebKit experience isn't really going to know what this is for. In fact, I only have a vague intuition that it's for security reasons. The documentaiton should really explain it.

[Martin Robinson]

(In reply to comment #11)
> Just add a little more info, the Clipboard functionality "paste" is covered by -enable-dom-paste and the "cut/copy" is covered by "javascript-can-access-clipboard". And I think I should have cover the "cut/copy" funtionality first.

Yeah, we should add all of the clipboard settings in one patch, I think.

(In reply to comment #13)
> If I enable the web settings it passes the test works as expected and fails if the websettings is disabled.

Take a look at the other tests that are in Source/WebKit2/UIProcess/API/gtk/tests to see how to add a new test. There are some basic sanity tests we do for simple settings like this. I think just the basic sanity test is fine, since all this does is flip a switch in WebCore. This is especially true if there are already layout tests that test the WebCore setting.

[Antaryami Pandia (apandia)]

(In reply to comment #14)
> The DOM you access from JavaScript is actually a type of DOM binding. I think "DOM" or something like it is important here to disassociate this from other pastes, which this does not affect.
> 
> What I'm really missing from this patch is why anyone would want to enable or disable the DOM paste setting. Is it for security reasons? 

That seems to be the reason.

>  I think someone without WebKit experience isn't really going to know what this is for.

You are right. In fact I didn't find any documentation for webkit settings. Is it possible to add a page in wiki for the same? If yes I can compile a list and publish in the wiki page.

> Take a look at the other tests that are in Source/WebKit2/UIProcess/API/gtk/tests to see how to add a new test.

I have added tests for the settings in "TestWebKitSettings.cpp".Please refer to WIP patch. Is this what you intended?

[Martin Robinson]

(In reply to comment #16)

> You are right. In fact I didn't find any documentation for webkit settings. Is it possible to add a page in wiki for the same? If yes I can compile a list and publish in the wiki page.

I'm not opposed to having these settings documented on a wiki page, but I think it's doubly important to have good documentation in the gtkdoc. I notice that Qt WebKit1 only has a single setting, QWebSettings::JavascriptCanAccessClipboard that controls both options. The settings don't seem to be documented for Cocoa.


> I have added tests for the settings in "TestWebKitSettings.cpp".Please refer to WIP patch. Is this what you intended?

Yep, thanks.

[Antaryami Pandia (apandia)]

(In reply to comment #17)
> I'm not opposed to having these settings documented on a wiki page, but I think it's doubly important to have good documentation in the gtkdoc. I notice that Qt WebKit1 only has a single setting, QWebSettings::JavascriptCanAccessClipboard that controls both options. The settings don't seem to be documented for Cocoa.

javascript-can-access-clipboard:- 

"Whether JavaScript can access Clipboard. The default value is FALSE. If set to %TRUE, then it allows cut/copy command to be executed when invoked from java script."

Is this ok?

[Antaryami Pandia (apandia)]

Created attachment 132005 [details]
Patch.

Patch.

[Martin Robinson]

Comment on attachment 132005 [details]
Patch.

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

Did you see my comment? Perhaps we should just expose WebKitSettings:javascript-can-access-clipboard and make it control both WKPreferencesSetJavaScriptCanAccessClipboard and WKPreferencesSetDOMPasteAllowed. I'm curious to here what others think.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:861
> +     * set to %TRUE, then it allows cut/copy command to be executed when

Nit: allows cut and copy commands

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:862
> +     * invoked from java script by using document.execCommand().

java script -> JavaScript

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:867
> +                                                         _("JavaScript can access Clipboard"),

Clipboard -> clipboard

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:876
> +     * is %FALSE. If set to %TRUE, then it allows paste command to be

allows paste command -> allows paste commands

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:877
> +     * executed when invoked from java script by using document.execCommand().

JavaScipt

[Antaryami Pandia (apandia)]

Thanks for the review.

(In reply to comment #20)
> (From update of attachment 132005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132005&action=review
> 
> Did you see my comment? Perhaps we should just expose WebKitSettings:javascript-can-access-clipboard and make it control both WKPreferencesSetJavaScriptCanAccessClipboard and WKPreferencesSetDOMPasteAllowed. I'm curious to here what others think.

I saw your comments, but I thought you are just referring to the implementation of qt port.  I agree with you. Even I have also asked for the feedback about the same in my comment #10 - "So do we required both the settings or we can have a single settings, say "enable-javascript-cutcopypaste" or any better name?".

will do and put it under one settings "javascript-can-access-clipboard".

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:861
> > +     * set to %TRUE, then it allows cut/copy command to be executed when
> 
> Nit: allows cut and copy commands

will do.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:862
> > +     * invoked from java script by using document.execCommand().
> 
> java script -> JavaScript

will do.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:867
> > +                                                         _("JavaScript can access Clipboard"),
> 
> Clipboard -> clipboard

will do.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:876
> > +     * is %FALSE. If set to %TRUE, then it allows paste command to be
> 
> allows paste command -> allows paste commands

will do.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:877
> > +     * executed when invoked from java script by using document.execCommand().
> 
> JavaScipt

will do.

[Antaryami Pandia (apandia)]

Created attachment 132213 [details]
Updated patch

Patch with review comments.

[Antaryami Pandia (apandia)]

Hi martin,
Please review the changes.

[Martin Robinson]

Comment on attachment 132213 [details]
Updated patch

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

Do you have commit access? If so can you fix the following small nits and land this?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:853
> +     * Whether JavaScript can access Clipboard. The default value is %FALSE. If

can access Clipboard -> can access the clipboard

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:854
> +     * set to %TRUE, then it allows cut, copy and pastes command to be executed when

pastes command -> paste commands

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2164
> + * Returns: %TRUE If javascript-can-access-clipboard support is enabled or %FALSE otherwise.

support is enabled -> is enabled

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:207
> +    // By default, JavaScript can access clipboard is disabled.

By default, JavaScript cannot access the clipboard.

[Antaryami Pandia (apandia)]

Thanks for the review.

(In reply to comment #24)
> (From update of attachment 132213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132213&action=review
> 
> Do you have commit access? If so can you fix the following small nits and land this?

I don't have commit access.

> can access Clipboard -> can access the clipboard
> pastes command -> paste commands
> support is enabled -> is enabled
> By default, JavaScript cannot access the clipboard.

will do.

[Antaryami Pandia (apandia)]

Created attachment 132762 [details]
Patch for landing.

Patch with review comments.

[Martin Robinson]

Comment on attachment 132762 [details]
Patch for landing.

Going to tweak the language a bit.

[Martin Robinson]

Committed r111399: <http://trac.webkit.org/changeset/111399>