Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add document.ExecCommand(). #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add document.ExecCommand(). #39

wants to merge 1 commit into from

Conversation

udhos
Copy link

@udhos udhos commented Jun 30, 2017

Attempt to fix #37

@@ -750,6 +751,10 @@ func (d document) DocumentURI() string {
return d.Get("documentURI").String()
}

func (d document) ExecCommand(name string, showDefaultUI bool, valueArgument string) bool {
return d.Call("execCommand", name, showDefaultUI, valueArgument).Bool()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valueArgument is documented at https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand as:

aValueArgument
For commands which require an input argument (such as insertImage, for which this is the URL of the image to insert), this is a DOMString providing that information. Specify null if no argument is needed.

So I don't think we can use string type for it, because it wouldn't be possible to specify null.

We can use either *string, or interface{} and document that it needs to be a string or nil.

@dominikh, do you have thoughts on this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I can tell is this use case worked for me:

dom.GetWindow().Document().ExecCommand("copy", false, "") // document.execCommand('copy');

Copy link
Collaborator

@dmitshur dmitshur Jun 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not surprised. Most JavaScript APIs will "work" when given input other than their documentation says they expect.

But I still think we should follow the documentation precisely here. Otherwise, people who look at this Go API will have questions raised in their minds, like "does this actually work despite not matching what the JS API expects?" and "will there be unintended side-effects if I don't follow the JS API documentation precisely?"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will "" ever be a valid argument? In a lot of other places, it is not, so we special-case "" in the code to act as null. See for example window.GetComputedStyle. – I'd rather not see us use *string or interface{}.

Copy link
Collaborator

@dmitshur dmitshur Jul 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect blank string to be a valid argument, but I haven't formally verified.

If there's already precedent for using "" when it's not a valid value to indicate something else (null), then that should be okay.

However, after briefly looking, it appears blank string may be a valid argument for insertText. It's not necessarily a no-op either, because it deletes selection. Also insertHTML.

Is the reason to push back against *string because it's a hard to use API?

In that case, how about *ExecCommandArgument, where ExecCommandArgument is a struct{ Value string }.

Or we can add logic to treat that parameter differently based on command name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing document.execCommand()?
3 participants