chore(webkit): driver updates for clicking/typing actions and related tests #23522

tbiethman · 2022-08-23T16:14:08Z

Related: Safari (WebKit) Support #6422

User facing changelog

N/A

Additional details

This PR re-enables driver tests related to clicking and typing for the WebKit browser. Included are some WebKit-specific tweaks to the driver for these actions. In-line doc is included where we deviate, please call out anything that is unclear or could otherwise be expounded on.

Steps to test

Review driver-integration-tests-webkit job results in CircleCI for the successful execution of the following specs:

type.cy.js
type_errors.cy.js
type_special_chars.cy.js
click.cy.js

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

Co-authored-by: Blue F <blue@cypress.io>

cypress-bot · 2022-08-23T16:14:11Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

tbiethman · 2022-08-23T16:23:22Z

packages/driver/cypress/e2e/commands/actions/click.cy.js

@@ -4064,12 +4062,28 @@ describe('mouse state', { browser: '!webkit' }, () => {
          y: 10,
        }

+        const coordsWebKit = {


The coordinates vary per-browser due to differences in the default stylesheet.

cypress · 2022-08-23T16:40:26Z

Test summary

47 • 0 • 11 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`31cb6fd`
Started	Aug 25, 2022 1:39 PM
Ended	Aug 25, 2022 1:50 PM
Duration	10:39 💡
OS	Linux Debian - 11.3
Browser	Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

tbiethman · 2022-08-23T16:50:47Z

packages/app/src/runner/logger.ts

-          return this._logTable({ table })
-        },
-      )
+  _logTables (consoleProps: any) {


We currently log tables for keyboard and mouse events, and we were creating slightly different data structures within the consoleProps for each. These differences made the existing logic complicated (and made parsing out the elements to avoid the WebKit hang difficult).

So I normalized the inputs and simplified the parsing here a bit, while ensuring that the elements don't make it into the table for WebKit. I'm sure we could simplify this further.

tbiethman · 2022-08-23T17:44:54Z

packages/driver/cypress/e2e/commands/actions/type.cy.js

-          6: { 'Details': '{ code: ControlLeft, which: 17 }', Typed: '{ctrl}', 'Events Fired': 'keyup', 'Active Modifiers': null, 'Prevented Default': null, 'Target Element': $input[0] },
-          7: { 'Details': '{ code: KeyI, which: 73 }', Typed: 'i', 'Events Fired': `keydown, keypress, beforeinput, textInput, input, keyup`, 'Active Modifiers': null, 'Prevented Default': null, 'Target Element': $input[0] },
-        }
+        const expectedTable = [


These were updated as part of the table log normalization mentioned above: https://github.com/cypress-io/cypress/pull/23522/files#r952882550

tbiethman · 2022-08-23T17:58:00Z

packages/driver/cypress/e2e/e2e/origin/commands/actions.cy.ts


        expect(KeyboardEventsTable.data[2]).to.have.property('Details').that.equals('{ code: KeyO, which: 79 }')
        expect(KeyboardEventsTable.data[2]).to.have.property('Typed').that.equals('o')
-
-        expect(KeyboardEventsTable.data[3]).to.have.property('Details').that.equals('{ code: KeyO, which: 79 }')
-        expect(KeyboardEventsTable.data[3]).to.have.property('Typed').that.equals('o')
      })


This difference is a result of switching from a number-keyed object (starting with key 1) with an array (indexed at 0).

Where is the source code you changed that led to the need to change this test? I don't see it.

The directly relevant change is here: https://github.com/cypress-io/cypress/pull/23522/files#diff-c0edb6eb8f7d6c9276eae18e47f83859d84a34dafaf05ec2a97b6fd5478983a5R91

For reference, this is how the Mouse/Keyboard Events tables are rendered in develop:

Notice that Mouse Events are indexed starting at 0, while Keyboard Events start at 1. I don't think this was intentional, or at least don't have a good reason as to why they'd be different.

This PR ensures both tables are rendered from arrays:

I didn't even know we could log a table like this, neat.

…tbiethman/webkit-tests

tbiethman · 2022-08-23T18:20:40Z

packages/driver/cypress/e2e/commands/actions/type_special_chars.cy.js

+      // WebKit does not round to the step value when calling stepUp/stepDown,
+      // as we do here for the ArrowUp handler.
+      // https://bugs.webkit.org/show_bug.cgi?id=244206
+      .should('have.value', Cypress.isBrowser('webkit') ? '14.34' : '14')


The bug I logged is getting a little traction, but I'm not sure how quick these things get turned around. Do we think it's worth adding our own logic here for WebKit to bring it to parity, or just documenting the issue as a current limitation? Current spec for the step attribute and stepUp/stepDown algorithms can be found here, if you're curious.

Option 3 ... Fix WebKt ourselves! But we'd still need something in the (probably long) interim.

If this is a WebKit bug, what you've done make sense to me - if a user runs a test expecting 14, it should fail - since it will fail in production for real users, too.

if a user runs a test expecting 14, it should fail - since it will fail in production for real users, too

The behavior we're simulating here is a user tapping the up/down arrows inside the input, which does work appropriately in WebKit. The problem is that the stepUp/stepDown functions we call to simulate those presses don't work the same way. So this would definitely need to be a documented limitation if we left things as-is.

I did write up the algorithm to handle the step logic ourselves. It's not too intensive, but it'll need a new suite of tests associated to it. I want to make sure we have other critical issues wrapped up before spending much more time on it (unless we consider this critical?).

The cross-section of users who use .type('{uparrow}') on a number input who will be using experimental WebKit is probably pretty small, I'd agree that focus should be put elsewhere. How large is the change to fix this?

The algorithm isn't very complicated, there are just a lot of bound checks/edge cases that I'd want to write thorough tests for. I'd hate to write a custom handler that introduces yet more bugs 🐛

I think we could wrap it up prior to the initial release if we start looking good on the other issues.

Sounds good, I say the sooner we ship this, the better - little fixes like this will come up organically, if we don;'t get to them (and we might have other edges cases we don't even know about yet).

This reverts commit 2c52293.

This reverts commit 47d1155.

tbiethman · 2022-08-24T04:07:59Z

packages/driver/cypress/e2e/commands/actions/type_special_chars.cy.js

      .focus()
+      .then(($input) => $input[0].setSelectionRange(0, 0))


Setting the selection range prior to focusing the input like we did previously will fail, due to the default selection Webkit performs (that we're working around here).

The ordering here for these commands seemed pretty arbitrary given what is asserted, and it generally seems appropriate to focus an input prior to setting a selection within it, so I didn't have too many concerns with this change. But it is technically a difference in behavior.

I did spend a bit of time trying to keep these tests passing as they are, but the resulting changes caused quite a few more focus events to be emitted (and quite a few more tests to fail as a result).

lmiller1990

No major blocker, just some questions.

lmiller1990 · 2022-08-24T04:15:17Z

packages/app/src/runner/logger.ts

-      return
-    }
+      if (Cypress.isBrowser('webkit')) {
+        // WebKit will hang when we attempt to log element references


Woah, major bug in webkit.

lmiller1990 · 2022-08-24T04:15:32Z

packages/app/src/runner/logger.ts


-    const table = this._getTable(consoleProps)
+        const getSimplifiedElementDisplay = (element: Element) => {


This seems similar to https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/dom/elements/utils.ts#L45 - or are they different enough not to warrant reuse?

They are pretty close. I wrote the current version to match what Chrome renders in its tables, the util would produce slightly different results. But we could reuse if there are strong feelings either way, it's exposed in app through Cypress.dom.stringify()

Up to you, I do not have strong feelings about this 💯

lmiller1990 · 2022-08-24T04:25:22Z

packages/driver/cypress/e2e/commands/actions/type_special_chars.cy.js

+      // WebKit does not round to the step value when calling stepUp/stepDown,
+      // as we do here for the ArrowUp handler.
+      // https://bugs.webkit.org/show_bug.cgi?id=244206
+      .should('have.value', Cypress.isBrowser('webkit') ? '14.34' : '14')


Option 3 ... Fix WebKt ourselves! But we'd still need something in the (probably long) interim.

If this is a WebKit bug, what you've done make sense to me - if a user runs a test expecting 14, it should fail - since it will fail in production for real users, too.

lmiller1990 · 2022-08-24T04:27:47Z

packages/driver/cypress/e2e/e2e/origin/commands/actions.cy.ts


        expect(KeyboardEventsTable.data[2]).to.have.property('Details').that.equals('{ code: KeyO, which: 79 }')
        expect(KeyboardEventsTable.data[2]).to.have.property('Typed').that.equals('o')
-
-        expect(KeyboardEventsTable.data[3]).to.have.property('Details').that.equals('{ code: KeyO, which: 79 }')
-        expect(KeyboardEventsTable.data[3]).to.have.property('Typed').that.equals('o')
      })


Where is the source code you changed that led to the need to change this test? I don't see it.

flotwig

Nice! I appreciate the thorough comments, these will help a lot down the road. Had a few Q's but LGTM.

packages/driver/cypress/e2e/commands/actions/type.cy.js

flotwig · 2022-08-24T15:48:25Z

packages/driver/cypress/e2e/commands/actions/type_special_chars.cy.js

+      // WebKit does not round to the step value when calling stepUp/stepDown,
+      // as we do here for the ArrowUp handler.
+      // https://bugs.webkit.org/show_bug.cgi?id=244206
+      .should('have.value', Cypress.isBrowser('webkit') ? '14.34' : '14')


The cross-section of users who use .type('{uparrow}') on a number input who will be using experimental WebKit is probably pretty small, I'd agree that focus should be put elsewhere. How large is the change to fix this?

flotwig · 2022-08-24T15:49:22Z

packages/driver/cypress/e2e/commands/actions/type_special_chars.cy.js

@@ -1736,7 +1745,8 @@ describe('src/cy/commands/actions/type - #type special chars', { browser: '!webk
        })
      })

-      it('will not submit the form', function (done) {
+      // WebKit still emits the submit event on the form in this configuration.


Is this just how WebKit works? It acts the same if a user presses "Enter" on a form with multiple submits?

Correct, I recreated this scenario outside of Cypress and found the form was still submitted in this case in WebKit, whereas its not in Chrome/FF.

flotwig · 2022-08-24T15:53:09Z

packages/driver/src/dom/selection.ts

+      // WebKit must use selectAllChildren to ensure selections are made in all use cases.
+      // Some content, like HTML markup, cannot be selected using a range when inside a contenteditable field.
+      if (Cypress.isBrowser('webkit')) {
+        selection.selectAllChildren(el)


Interesting, I wonder why we aren't just using selectAllChildren in all browsers, did you check if this works for Cr/FF?

Good call, it does work for the current versions of Chrome/Firefox and Chromium 74, which is the earliest I can grab. Minimum supported Chrome is 64, so there's still a range of supported versions I can't validate.

MDN says it's supported since uh.. Chrome... squints Chrome 1. https://developer.mozilla.org/en-US/docs/Web/API/Selection/selectAllChildren#browser_compatibility So it should be safe if the tests pass?

Wow, straight from the primordial soup. Well we know it won't throw at least! I'll get that change made and verified in CI.

…browsers

flotwig and others added 30 commits March 16, 2021 17:56

detect playwright-webkit browser

de8e6c3

fix: use stdio for CDP instead of TCP (#14348)

d0d1736

wip: begin launchin webkit

818c7de

Merge remote-tracking branch 'origin/develop' into cy-webkit

a7279e5

run mode works w webkit in 10.0

45b5286

reset previous cdp changes

ecd6f0f

run driver webkit tests

505c0dd

always detect webkit in non-prod

35b4039

fix version detection

ceba06f

actually run new job

68ebaca

Merge remote-tracking branch 'origin/develop' into cy-webkit

a5a0fc6

cleanup

f974e99

fix run

25b8fc9

try caching pw binary

411a2f1

npx install pw binary

83c58d5

install-deps

def14cc

add experimentalSessionAndOrigin wk tests

e85025c

wk experimentalSessionAndOrigin tests

53db500

browser icon

12b3cab

fix some tests

37de56a

reset browsers.ts change

32ed232

fix more tests

3acc19b

fix even more tests, skip driver CI for now

0681f0a

comma

67fdd2d

fix server-unit-test

58b2d39

fix websockets_spec

406d8ec

refactor wkautomation to initialize self from static async method

8fb785a

fix(proxy/prerequests): fix duplicate key behavior, fallthrough

b997ff0

Apply suggestions from code review

14270e1

Co-authored-by: Blue F <blue@cypress.io>

simpler name for StackMap

43b2f0a

Making WebKit checks more consistent

33cc866

tbiethman mentioned this pull request Aug 23, 2022

Safari (WebKit) Support #6422

Closed

21 tasks

tbiethman changed the title ~~fix: driver updates for clicking/typing actions and related tests for WebKit~~ chore(webkit): driver updates for clicking/typing actions and related tests for WebKit Aug 23, 2022

tbiethman changed the title ~~chore(webkit): driver updates for clicking/typing actions and related tests for WebKit~~ chore(webkit): driver updates for clicking/typing actions and related tests Aug 23, 2022

Don't expose webkit in public types

ef49430

tbiethman commented Aug 23, 2022

View reviewed changes

tbiethman added 2 commits August 23, 2022 13:12

Adding comments and doing a little cleanup

5bc01fd

Merge branch 'develop' of https://github.com/cypress-io/cypress into …

48d2010

…tbiethman/webkit-tests

tbiethman commented Aug 23, 2022

View reviewed changes

tbiethman and others added 6 commits August 23, 2022 17:37

PR updates

f082b2d

Simplifying workaround for webkit focus selection

47d1155

Removing unnecessary test

2c52293

Revert "Removing unnecessary test"

455ea3f

This reverts commit 2c52293.

Revert "Simplifying workaround for webkit focus selection"

822f4c0

This reverts commit 47d1155.

Merge branch 'develop' into tbiethman/webkit-tests

3468805

tbiethman marked this pull request as ready for review August 24, 2022 04:01

tbiethman requested a review from flotwig August 24, 2022 04:01

tbiethman commented Aug 24, 2022

View reviewed changes

lmiller1990 approved these changes Aug 24, 2022

View reviewed changes

Removing comment that is no longer applicable

b986f8e

flotwig approved these changes Aug 24, 2022

View reviewed changes

tbiethman and others added 2 commits August 24, 2022 16:10

Simplifying selection logic that is now functional for all supported …

a694163

…browsers

Merge branch 'develop' into tbiethman/webkit-tests

31cb6fd

tbiethman merged commit 2fcecbf into develop Aug 25, 2022

tbiethman deleted the tbiethman/webkit-tests branch August 25, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(webkit): driver updates for clicking/typing actions and related tests #23522

chore(webkit): driver updates for clicking/typing actions and related tests #23522

tbiethman commented Aug 23, 2022

cypress-bot bot commented Aug 23, 2022

tbiethman Aug 23, 2022

cypress bot commented Aug 23, 2022 •

edited

Loading

tbiethman Aug 23, 2022

tbiethman Aug 23, 2022

tbiethman Aug 23, 2022

lmiller1990 Aug 24, 2022

tbiethman Aug 24, 2022 •

edited

Loading

lmiller1990 Aug 24, 2022

tbiethman Aug 23, 2022 •

edited

Loading

lmiller1990 Aug 24, 2022

tbiethman Aug 24, 2022

flotwig Aug 24, 2022

tbiethman Aug 24, 2022

lmiller1990 Aug 24, 2022

tbiethman Aug 24, 2022 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Aug 24, 2022

lmiller1990 Aug 24, 2022

tbiethman Aug 24, 2022 •

edited

Loading

lmiller1990 Aug 24, 2022

lmiller1990 Aug 24, 2022

lmiller1990 Aug 24, 2022

flotwig left a comment

flotwig Aug 24, 2022

flotwig Aug 24, 2022

tbiethman Aug 24, 2022

flotwig Aug 24, 2022

tbiethman Aug 24, 2022

flotwig Aug 24, 2022

tbiethman Aug 24, 2022

		.focus()
		.then(($input) => $input[0].setSelectionRange(0, 0))


		const table = this._getTable(consoleProps)
		const getSimplifiedElementDisplay = (element: Element) => {

chore(webkit): driver updates for clicking/typing actions and related tests #23522

chore(webkit): driver updates for clicking/typing actions and related tests #23522

Conversation

tbiethman commented Aug 23, 2022

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 23, 2022

Choose a reason for hiding this comment

cypress bot commented Aug 23, 2022 • edited Loading

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Aug 23, 2022 •

edited

Loading

tbiethman Aug 24, 2022 •

edited

Loading

tbiethman Aug 23, 2022 •

edited

Loading

tbiethman Aug 24, 2022 •

edited

Loading

tbiethman Aug 24, 2022 •

edited

Loading