Skip to content

Commit

Permalink
Refactor elementHandle.click parsing
Browse files Browse the repository at this point in the history
Move the parsing of the options for elementHandle.click outside of the
promise goroutine and back onto the main goja thread in the mapping
layer. This will help mitigate the risk of a panic if more than one
goroutine is accessing the goja runtime off the main goja thread.

This doesn't solve the problem completely though since this API calls
to other areas of the codebase which does still interact with the goja
runtime. See #1170 for
further details.
  • Loading branch information
ankur22 committed Jan 23, 2024
1 parent 10399c8 commit 7e4b794
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 33 deletions.
13 changes: 10 additions & 3 deletions browser/mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,18 @@ func mapElementHandle(vu moduleVU, eh *common.ElementHandle) mapping {
maps := mapping{
"boundingBox": eh.BoundingBox,
"check": eh.Check,
"click": func(opts goja.Value) *goja.Promise {
"click": func(opts goja.Value) (*goja.Promise, error) {
ctx := vu.Context()

popts := common.NewElementHandleClickOptions(eh.Timeout())
if err := popts.Parse(ctx, opts); err != nil {
return nil, fmt.Errorf("parsing element click options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
err := eh.Click(opts)
err := eh.Click(popts)
return nil, err //nolint:wrapcheck
})
}), nil
},
"contentFrame": func() (mapping, error) {
f, err := eh.ContentFrame()
Expand Down
12 changes: 4 additions & 8 deletions common/element_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -733,18 +733,14 @@ func (h *ElementHandle) BoundingBox() *Rect {
// Click scrolls element into view and clicks in the center of the element
// TODO: look into making more robust using retries
// (see: https://github.com/microsoft/playwright/blob/master/src/server/dom.ts#L298)
func (h *ElementHandle) Click(opts goja.Value) error {
actionOpts := NewElementHandleClickOptions(h.defaultTimeout())
if err := actionOpts.Parse(h.ctx, opts); err != nil {
k6ext.Panic(h.ctx, "parsing element click options: %v", err)
}
func (h *ElementHandle) Click(opts *ElementHandleClickOptions) error {
click := h.newPointerAction(
func(apiCtx context.Context, handle *ElementHandle, p *Position) (any, error) {
return nil, handle.click(p, actionOpts.ToMouseClickOptions())
return nil, handle.click(p, opts.ToMouseClickOptions())
},
&actionOpts.ElementHandleBasePointerOptions,
&opts.ElementHandleBasePointerOptions,
)
if _, err := call(h.ctx, click, actionOpts.Timeout); err != nil {
if _, err := call(h.ctx, click, opts.Timeout); err != nil {
return fmt.Errorf("clicking on element: %w", err)
}
applySlowMo(h.ctx)
Expand Down
36 changes: 15 additions & 21 deletions tests/element_handle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,11 @@ func TestElementHandleClick(t *testing.T) {
button, err := p.Query("button")
require.NoError(t, err)

err = button.Click(tb.toGojaValue(struct {
NoWaitAfter bool `js:"noWaitAfter"`
}{
// FIX: this is just a workaround because navigation is never triggered
// and we'd be waiting for it to happen otherwise!
NoWaitAfter: true,
}))
opts := common.NewElementHandleClickOptions(button.Timeout())
// FIX: this is just a workaround because navigation is never triggered
// and we'd be waiting for it to happen otherwise!
opts.NoWaitAfter = true
err = button.Click(opts)
require.NoError(t, err)

res := tb.asGojaValue(p.Evaluate(tb.toGojaValue("() => window['result']")))
Expand All @@ -130,13 +128,11 @@ func TestElementHandleClickWithNodeRemoved(t *testing.T) {
button, err := p.Query("button")
require.NoError(t, err)

err = button.Click(tb.toGojaValue(struct {
NoWaitAfter bool `js:"noWaitAfter"`
}{
// FIX: this is just a workaround because navigation is never triggered
// and we'd be waiting for it to happen otherwise!
NoWaitAfter: true,
}))
opts := common.NewElementHandleClickOptions(button.Timeout())
// FIX: this is just a workaround because navigation is never triggered
// and we'd be waiting for it to happen otherwise!
opts.NoWaitAfter = true
err = button.Click(opts)
require.NoError(t, err)

res := tb.asGojaValue(p.Evaluate(tb.toGojaValue("() => window['result']")))
Expand All @@ -156,13 +152,11 @@ func TestElementHandleClickWithDetachedNode(t *testing.T) {
// Detach node to panic when clicked
p.Evaluate(tb.toGojaValue("button => button.remove()"), tb.toGojaValue(button))

err = button.Click(tb.toGojaValue(struct {
NoWaitAfter bool `js:"noWaitAfter"`
}{
// FIX: this is just a workaround because navigation is never triggered and we'd be waiting for
// it to happen otherwise!
NoWaitAfter: true,
}))
opts := common.NewElementHandleClickOptions(button.Timeout())
// FIX: this is just a workaround because navigation is never triggered
// and we'd be waiting for it to happen otherwise!
opts.NoWaitAfter = true
err = button.Click(opts)
assert.ErrorContains(
t, err,
"element is not attached to the DOM",
Expand Down
2 changes: 1 addition & 1 deletion tests/page_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestNestedFrames(t *testing.T) {
assert.Nil(t, err)
assert.NotNil(t, button1Handle)

err = button1Handle.Click(nil)
err = button1Handle.Click(common.NewElementHandleClickOptions(button1Handle.Timeout()))
assert.Nil(t, err)

v := frame2.Evaluate(tb.toGojaValue(`() => window.buttonClicked`))
Expand Down

0 comments on commit 7e4b794

Please sign in to comment.