-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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() | |||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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');
There was a problem hiding this comment.
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?"
There was a problem hiding this comment.
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{}
.
There was a problem hiding this comment.
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.
Attempt to fix #37