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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ type Document interface {
CreateTextNode(s string) *Text
ElementFromPoint(x, y int) Element
EnableStyleSheetsForSet(name string)
ExecCommand(name string, showDefaultUI bool, valueArgument string) bool
GetElementsByClassName(name string) []Element
GetElementsByTagName(name string) []Element
GetElementsByTagNameNS(ns, name string) []Element
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.


func (d document) Implementation() DOMImplementation {
// FIXME implement
panic("not implemented")
Expand Down