Add combo key press

[ankur22]

This fixes #285

Shift+X when passed to press() and used in conjunction with down() and up() now will change the casing of the character, as long as the character is in the format KeyX. A key press that is x or X will ignore the shift modifier key. This fix matches the playwright behaviour.

E.g.

Shift+KeyA -> A
KeyA -> a
A -> A
a -> a
Shift+A -> A
Shift+a -> a

The Shift modifier will only change the case of a character if KeyA - KeyZ is used.

[imiric]

Nicely done, I just think we should move it to press() instead.

And re: the Shift+b behavior, I don't think Playwright makes sense in that case. Shift+b or Shift+B should produce B in both cases. A lowercase b can be typed without Shift. It's fine if we diverge with them to improve things. :)

[inancgumus]

And re: the Shift+b behavior, I don't think Playwright makes sense in that case. Shift+b or Shift+B should produce B in both cases. A lowercase b can be typed without Shift. It's fine if we diverge with them to improve things. :)

I'm mostly OK with what you propose @imiric, but here are my two cents.

Is diverging PW, in this case, worth it? If I were a tester, I'd at least expect similar behavior. Shift+KeyA+B+C means holding down the shift key while pressing the other keys. For example: Shift+KeyA+KeyB+B+b is ABBb, which makes sense to me.

Puppeteers also work with KeyA, KeyX, etc. style.

So instead of Shift+B, it should be Shift+KeyB, not Shift+B. KeyX is also in tandem with JS key codes: https://javascript.info/keyboard-events, https://www.toptal.com/developers/keycode/for/a, etc.

As I said, I'm also OK with what you proposed. But, I'm not sure whether we're improving things or making people's life harder 😅

[imiric]

I wasn't familiar with the KeyA/A difference, but would still find Shift+a producing a surprising. That said, it's best if we stick with Playwright in this case then.

Shift+KeyA+KeyB+B+b is ABBb, which makes sense to me

That's still confusing to me. Does the Shift apply to just the first KeyA, or to all keys? In all cases I've seen this notation used (outside of JS/browsers), it only makes sense for modifier keys that apply to a single non-modifier key, e.g. Shift+Control+Alt+A. After all, if A+B+C equals pressing one after another, there's no need to use + or a modifier key.

But still, let's confirm what Playwright's behavior is for this as well, and follow that to avoid adding more surprises for users.

[inancgumus]

As @imiric also noticed, splitting a given key should be done in press. down and up are only responsible for a single key, and that key can be a modifier as well. For more info: https://github.com/grafana/xk6-browser/pull/326/files#r885768387.

[ankur22]

I'm unable to get the Meta and Alt modifier keys to work with any of the existing functions e.g.

		kb.Press("Shift+KeyA", nil)
		kb.Press("Shift+b", nil)
		kb.Press("Shift+C", nil)

		metaKey := "Control"
		if runtime.GOOS == "darwin" {
			metaKey = "Meta"
		}
		kb.Press(metaKey+"+A", nil)
		kb.Press("Delete", nil)

OR

		kb.Press("Shift+KeyA", nil)
		kb.Press("Shift+b", nil)
		kb.Press("Shift+C", nil)

		kb.Press("Alt+ArrowLeft", nil)
		kb.Press("Delete", nil)

This should delete the text that has been written into the input field. To continue to track and resolve this a new ticket has been opened: grafana/k6#4440

[inancgumus]

I'll continue looking into this tomorrow, but it failed on my machine as follows:

--- FAIL: TestPageInputSpecialCharacters (0.68s)
    page_test.go:341: 
                Error Trace:    page_test.go:341
                Error:          Not equal: 
                                expected: "[email protected]"
                                actual  : "test@k^>io"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                [email protected]
                                +test@k^>io
                Test:           TestPageInputSpecialCharacters
    page_test.go:341: 
                Error Trace:    page_test.go:341
                Error:          Not equal: 
                                expected: "<hello WoRlD \\/>"
                                actual  : "<hello WoRlD |?>"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -<hello WoRlD \/>
                                +<hello WoRlD |?>
                Test:           TestPageInputSpecialCharacters
    page_test.go:341: 
                Error Trace:    page_test.go:341
                Error:          Not equal: 
                                expected: "¯\\_(ツ)_/¯"
                                actual  : "¯|_(ツ)_?¯"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -¯\_(ツ)_/¯
                                +¯|_(ツ)_?¯
                Test:           TestPageInputSpecialCharacters

Update: CI has reported the failure as well: https://github.com/grafana/xk6-browser/runs/7114936484?check_suite_focus=true

[inancgumus]

Nit: No need to put an empty line here, but your call :)

[inancgumus]

Is there a reason why this is not a range as the above down loop?

[ankur22]

It's going backwards:

a->b->c
c->b->a

[inancgumus]

Ah, OK :) It's the same, but what about?

for i := range kk {
	// kk[len(kk)-i-1]
}

[ankur22]

Yeah sure, nice little trick 🙂

[imiric]

That trick is not intuitive at all IMO. The previous C-style for was clear and straightforward.

Please let's not point out style issues in reviews (we agreed on this before Ankur joined), unless the reviewer can explain an objective benefit of the change.

No need to change this back now, I don't want to spend more time on this, but just for future reference.

[inancgumus]

I looked more into this. Here are my new findings.

[inancgumus]

The linter is right. It's better to convert the if-else chain into a switch.

for _, k := range keys {
	switch {
	case k == '+' && ss != "":
		...
...

And, you can use strings.Builder to append runes without converting them to string.

[ankur22]

Nice, I hadn't realised that it was idiomatic to write them as switch statements. Source: https://go.dev/doc/effective_go#switch

[imiric]

Nice work 👍 Only the test comment seems like a blocker to me, the others are minor suggestions.

It's strange that meta keys don't seem to work. That's a pretty important issue, but we can tackle it later.

Also, remember to mark the PR "Ready for review" when it's done being a draft.

[imiric]

LGTM, nice work 👍

I would just move split() to common/helpers.go and its tests to helpers_test.go. On second thought, it might be a uniquely keyboard helper function, so it makes sense where it is. Up to you.

The CI failures don't seem to be related to these changes, though that TestPageGotoWaitUntilDOMContentLoaded is a new one I haven't seen before. 😕 Please squash this into a single commit and push it so the tests run again before merging.

[ankur22]

I would just move split() to common/helpers.go and its tests to helpers_test.go. On second thought, it might be a uniquely keyboard helper function, so it makes sense where it is. Up to you.

Decided to not move it to the helper file since it feels like it might only be for the keyboard.

The CI failures don't seem to be related to these changes, though that TestPageGotoWaitUntilDOMContentLoaded is a new one I haven't seen before. 😕 Please squash this into a single commit and push it so the tests run again before merging.

Squashed the commits, and the build failed with the usual errors we've seen before. I reran the CI tests and it passed.