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.

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

dependency: Update jquery to 3.4.1 #29837

Merged
merged 12 commits into from
Jul 12, 2024
Merged

dependency: Update jquery to 3.4.1 #29837

merged 12 commits into from
Jul 12, 2024

Conversation

jennifer-shehane
Copy link
Member

@jennifer-shehane jennifer-shehane commented Jul 11, 2024

  • Closes Upgrade JQuery to 3.2.0 #29822
  • Resolves CVE-2019-11358

    jQuery from 1.1.4 until 3.4.0, as used in Drupal, Backdrop CMS, and other products, mishandles jQuery.extend(true, {}, ...) because of Object.prototype pollution. If an unsanitized source object contained an enumerable proto property, it could extend the native Object.prototype.

Additional details

This bumps our jQuery version and the version included on Cypress.$ to 3.4.1.

In jQuery 3.2.0 a change was introduced and further iterated on with the calculation of .width() and .height(). Previously .width() and .height() would return the width and height of elements including scrollbars. In jQuery 3.2.0, they updated these methods to more closely match the CSS box model by returning the width and height excluding the scrollbars.

This would introduce a breaking change to users using the Cypress.$ API.

// jQuery 3.1.1 would pass, but in 3.2.0 it would equal 85 (minus scrollbars)
expect(Cypress.$('#scroll-to-both').height()).to.equal(100)

Additionally this change in width/height calculation changes our 'scrollTo' behavior since that relies on the width/height calculated by jQuery.

Screenshot 2024-07-11 at 9 43 38 AM

While technically the height/width calculations and scrollTo behavior are more correct, we don't want to introduce breaking changes in a non-breaking change release. Consequently, I have patched jQuery 3.4.1 to overwrite the height/width getters to use the old calculations. This could result in some confusion with jQuery.height and jQuery.width returning different results than Cypress, but I guess that's already the case today and no one has raised that.

jQuery 3.5.0 introduces some changes that broke more driver tests, so that would need further investigation.


Commits between jQuery 3.3.1 and 3.4.1

Features:

Deprecations:

  • jQuery.isArray
  • holdReady
  • jQuery.nodeName
  • jQuery.isWindow
  • jQuery.now
  • jQuery.proxy
  • jQuery.isFunction
  • jQuery.isNumeric
  • jQuery.type

Bugfixes:

  • Fix offset for zero sized elements
  • .contents() supporting HTMLTemplateElement
  • a bunch of more stuff

Steps to test

All of the previous tests should pass.

How has the user experience changed?

Users should see more correct behavior from jQuery fixes and have access to new jQuery features like .css()

PR Tasks

@jennifer-shehane jennifer-shehane changed the title Update jquery to 3.4.1 dependency: Update jquery to 3.4.1 Jul 11, 2024
Copy link

cypress bot commented Jul 11, 2024

8 flaky tests on run #56120 ↗︎

0 29207 1328 0 Flakiness 8

Details:

Merge branch 'develop' into jquery-update
Project: cypress Commit: fdeea9d86e
Status: Passed Duration: 22:43 💡
Started: Jul 12, 2024 7:14 PM Ended: Jul 12, 2024 7:36 PM
Flakiness  specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e

View Output

Test Artifacts
App/Cloud Integration - Latest runs and Average duration > when not using a branch > shows placeholders for all visible specs Test Replay Screenshots
Flakiness  e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-electron

View Output

Test Artifacts
... > throws when foo cannot resolve Test Replay
Flakiness  cypress/proxy-logging.cy.ts • 1 flaky test • 5x-driver-electron

View Output

Test Artifacts
Proxy Logging > request logging > fetch log shows resource type, url, method, and status code and has expected snapshots and consoleProps Test Replay
Flakiness  commands/waiting.cy.js • 1 flaky test • 5x-driver-chrome

View Output

Test Artifacts
... > errors > throws when waiting for 2nd response to route Test Replay
Flakiness  e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-chrome

View Output

Test Artifacts
... > throws when foo cannot resolve Test Replay

The first 5 flaky specs are shown, see all 6 specs in Cypress Cloud.

Review all test suite changes for PR #29837 ↗︎

@jennifer-shehane jennifer-shehane self-assigned this Jul 11, 2024
Comment on lines -4106 to -4107
layerX: 492,
layerY: 215,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layerX and layerY properties are non-standard. We already had these mouse props commented out for Firefox because it was returning odd results. I'm removing the tests for these non-standard properties on all browsers. https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/layerX

Non-standard: This feature is non-standard and is not on a standards track. Do not use it on production sites facing the Web: it will not work for every user. There may also be large incompatibilities between implementations and the behavior may change in the future.


// width/height of scrollable container - width of parent viewport (minux scrollbars) / 2 to get the center
// browsers round up the pixel value so we need to round it
this.halfScrollPixels = Math.round((500 - 100) / 2)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just moving this calc to one place with some explanation because it took me a while to figure out these values from thin air.

Comment on lines +4107 to +4109
// pageY is 220.5 in headless Electron
// since updating to jquery 3.2+....why...
// pageY: 215,
Copy link
Member Author

@jennifer-shehane jennifer-shehane Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pageY value returns 220.5 in headless Electron. It is 215 in headed, it is 215 in Chrome, it is 215 in the version of Chromium 118 that I could find. I commented this out. I have no earthly idea why a change in jQuery would result in this pageY value being slightly different when clicking from 1 div to another div in Electron headless. I'm not sure that this quirk is worth further investigation, but open for debate.

Copy link
Contributor

@cacieprins cacieprins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - can we file an issue to resolve when we do our next major release to revert the jquery patch?

@jennifer-shehane
Copy link
Member Author

@cacieprins Opened a followup issue here: #29846

@jennifer-shehane jennifer-shehane merged commit 17fd597 into develop Jul 12, 2024
80 of 82 checks passed
@jennifer-shehane jennifer-shehane deleted the jquery-update branch July 12, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade JQuery to 3.2.0
3 participants