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

Offset: Increase search depth when finding the 'real' offset parent #4861

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

Minimaximize
Copy link
Contributor

@Minimaximize Minimaximize commented Mar 20, 2021

Summary

Continue searching for the next positioned offsetParent (or the document) when calculating position().

This fixes the seemingly inconsistent behaviour that occurred when calling position() on elements inside tables; for which the offsetParent changes based on its position css property. e.g

<div id="positioned-container" style="position: relative;">
    <table>
        <tr>
            <td>
                <span id="static"></span>
                <span id="relative" style="position: relative;"></span>
                <span id="absolute" style="position: absolute;"></span>
            </td>
        </tr>
    </table>
</div>

The offsetParent of #static is td
The offsetParent of #relative and #absolute is #positioned-container

Previously $('#static').position() was returning the position relative to the containing <td> element, while $('#relative').position() was returning the position relative to #positioned-container

Now, both elements return their position relative to the containing #positioned-container

Checklist

@Minimaximize
Copy link
Contributor Author

Whoops, I thought I had fixed that failing test 😓😓

@Minimaximize
Copy link
Contributor Author

Finally tracked down the docs describing why the licence check was indeterminate - Fixed the erroneous author attributions for my commits

@mgol
Copy link
Member

mgol commented Sep 17, 2021

Closing & re-opening the PR to trigger the EasyCLA check...

@mgol mgol closed this Sep 17, 2021
@mgol mgol reopened this Sep 17, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 17, 2021

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Thanks, this is good!

}
if ( offsetParent && offsetParent !== elem && offsetParent.nodeType === 1 ) {
if ( offsetParent && offsetParent !== elem && offsetParent.nodeType === 1 &&
jQuery.css( offsetParent, "position" ) !== "static" ) {

// Incorporate borders into its offset, since they are outside its content origin
parentOffset = jQuery( offsetParent ).offset();
Copy link
Member

Choose a reason for hiding this comment

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

While we're here making changes, we could also account for scrolling.

-				// Incorporate borders into its offset, since they are outside its content origin
+				// The origin against which we will be returning a relative position is the absolute offset
+				// of offsetParent, plus the top/left width of its borders (since they are outside offsetParent's
+				// content origin), minus its top/left scroll position (which has already affected element
+				// absolute offset and should not be counted twice)
 				parentOffset = jQuery( offsetParent ).offset();
-				parentOffset.top += jQuery.css( offsetParent, "borderTopWidth", true );
-				parentOffset.left += jQuery.css( offsetParent, "borderLeftWidth", true );
+				parentOffset.top += jQuery.css( offsetParent, "borderTopWidth", true ) - offsetParent.scrollTop;
+				parentOffset.left += jQuery.css( offsetParent, "borderLeftWidth", true ) - offsetParent.scrollLeft;

Copy link
Member

Choose a reason for hiding this comment

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

I guess we should add some tests for this part of the behavior then as well.

Copy link
Member

Choose a reason for hiding this comment

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

Extracted to #5468. @gibson042 if you have nothing against, I'd merge this PR as-is and track the remaining changes via #5468. Let me know what you think.

@mgol mgol added the Offset label Mar 25, 2024
@mgol mgol added this to the 4.0.0 milestone Mar 25, 2024
@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Mar 25, 2024
@mgol mgol changed the title Offset: increase search depth when finding the 'real' offset parent Offset: Increase search depth when finding the 'real' offset parent Apr 19, 2024
@mgol mgol merged commit 556eaf4 into jquery:main Apr 19, 2024
13 checks passed
@mgol
Copy link
Member

mgol commented Apr 19, 2024

Followup work extracted to #5468 per the last team meeting.

@mgol mgol removed Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. Needs review labels Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants