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

fix(dom): handle slotted parent transform position #8158

Merged
merged 5 commits into from May 3, 2024

Conversation

weiz18
Copy link
Contributor

@weiz18 weiz18 commented Feb 24, 2023

Description

getPointerPosition for IOS is not handling slot elements and nested web components due to the following:

  1. item.parentNode will stop at the first web component and any transform applied on the outer component won't be part of the calculation and translate.x will be wrong.
  2. item.parentNode will skip the slot parent wrapper entirely, so any transformation applied to the parent wrapper won't be counted.

It would be useful to support this as we have a use case where we are displaying a list of videos inside the slides web component.(https://ionicframework.com/docs/api/slides) and here is a simple reproduction sample where the pointer position is miscalculated with a slotted video having a wrapper with transform.
https://codepen.io/weiz18/pen/poOEjzJ?editors=1000

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.85%. Comparing base (ee07382) to head (bc7e49c).

❗ Current head bc7e49c differs from pull request most recent head 383493d. Consider uploading reports for the commit 383493d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8158   +/-   ##
=======================================
  Coverage   82.85%   82.85%           
=======================================
  Files         113      113           
  Lines        7647     7567   -80     
  Branches     1839     1820   -19     
=======================================
- Hits         6336     6270   -66     
+ Misses       1311     1297   -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@weiz18
Copy link
Contributor Author

weiz18 commented Apr 4, 2023

Could you review this one ? thanks! 🙏

@misteroneill misteroneill requested a review from gkatsev May 9, 2023 16:13
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

This would require a test of some kind to ensure code coverage, but it seems reasonable - I don't know much about "slotted" elements, though, personally so maybe @gkatsev might have some thoughts?

@weiz18
Copy link
Contributor Author

weiz18 commented Jul 6, 2023

Hey, i have updated the test to cover the change for default and slot element behaviour 🙏 Could you review it when you have time ? thanks !

@weiz18 weiz18 requested a review from misteroneill July 6, 2023 14:28
Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

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

I don't have any real experience of using , but since the if block ensures that this can't affect regular use cases and there's a well commented test, LGTM.

@mister-ben mister-ben added needs: LGTM Needs an additional approval and removed needs: updates labels Jul 6, 2023
@mister-ben mister-ben merged commit 9946a19 into videojs:main May 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: LGTM Needs an additional approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants