Test: ensure position/offset return mutable objects #3695
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmethvin
reviewed
Jun 20, 2017
| @@ -75,10 +75,11 @@ QUnit.test( "disconnected element", function( assert ) { | |||
| assert.equal( result.top, 0, "Retrieving offset on disconnected elements returns zeros (gh-2310)" ); | |||
| assert.equal( result.left, 0, "Retrieving offset on disconnected elements returns zeros (gh-2310)" ); | |||
| assert.equal( Object.keys( result ).length, 2, "Retrieving offset on disconnected elements returns offset object (gh-3167)" ); | |||
| assert.equal( jQuery.isPlainObject( result ), true, "Retrieving offset on disconnected elements returns plain object (gh-3612)" ); | |||
There was a problem hiding this comment.
Should we be actually trying to mutate the object here? The jQuery.isPlainObject() test is a bit of an indirect feature test and technically we could be returning a plain object with read-only properties.
There was a problem hiding this comment.
I guess I was just ensuring it wasn't a ClientRect. I could create a helper that checks both isPlainObject and ensures it is modifiable. WDYT?
|
Agreed, revert 3befe59, and good to land. |
|
Was there any discussion about the use of |
|
I've added the revert of 3befe59 since it reduces size: |
timmywil
approved these changes
Jul 6, 2017
jbedard
added a commit
to jbedard/jquery
that referenced
this pull request
Jul 9, 2017
gibson042
reviewed
Jul 10, 2017
| left: parentOffset.left + jQuery.css( offsetParent, "borderLeftWidth", true ) | ||
| }; | ||
| parentOffset.top += jQuery.css( offsetParent[ 0 ], "borderTopWidth", true ); | ||
| parentOffset.left += jQuery.css( offsetParent[ 0 ], "borderLeftWidth", true ); |
There was a problem hiding this comment.
I think these need to be offsetParent, not offsetParent[ 0 ].
jbedard
added a commit
to jbedard/jquery
that referenced
this pull request
Jul 10, 2017
jbedard
added a commit
that referenced
this pull request
Jul 10, 2017
ornicar
added a commit
to ornicar/jquery
that referenced
this pull request
Jul 30, 2017
* 'master' of git://github.com/jquery/jquery: (92 commits) Ajax: add an ontimeout handler to all requests Tests: Account for TestSwarm focus issues Tests: Simulate events when CI hinders use of native ones Tests: Reduce the abort timeout for simple focus testing Tests: Abort focus tests when the environment doesn't cooperate Tests: Try extra hard to control focus Support: Properly check for IE9 absolute scrollbox mishandling Dimensions: Detect and account for content-box dimension mishandling Offset: fix error from bad merge in jquery#3695 Revert "Offset: Resolve strict mode ClientRect "no setter" exception" Test: ensure position/offset return mutable objects Core: Deprecate jQuery.isWindow Event: `stopPropagation()` on native event-handler Build: fix uglify options for uglify update Build: Update sinon, husky, and qunitjs Build: update node dependencies; commit package-lock.json Deferred: fix memory leak of promise callbacks Dimensions: Include scroll gutter in "padding" box Tests: minor typos Build: Test on Node.js 8, stop testing on Node.js 7 ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds tests verifying #3612. Could probably add more but this seemed to cover most cases?
Could potentially also revert 3befe59