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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
May 3, 2024
Merged

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

merged 5 commits into from
May 3, 2024

Conversation

weiz18
Copy link
Contributor

@weiz18 weiz18 commented Feb 24, 2023 •
edited
Loading

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 •
edited
Loading

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
misteroneill
misteroneill previously requested changes May 9, 2023
View reviewed changes
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 weiz18 mentioned this pull request Jun 2, 2023
Open
@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
mister-ben
mister-ben approved these changes Jul 6, 2023
View reviewed changes
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 one or more additional approvals 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
@kerasus kerasus mentioned this pull request Jun 21, 2024
Open
@Bitti09 Bitti09 mentioned this pull request Jun 21, 2024
Closed
@kerasus kerasus mentioned this pull request Jun 28, 2024
Open
@apiiro-snyk apiiro-snyk mentioned this pull request Jul 2, 2024
Open
@kerasus kerasus mentioned this pull request Jul 4, 2024
Open
@ChaosEngine ChaosEngine mentioned this pull request Jul 9, 2024
Open
@kerasus kerasus mentioned this pull request Jul 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mister-ben mister-ben mister-ben approved these changes

@adrums86 adrums86 adrums86 approved these changes

@gkatsev gkatsev Awaiting requested review from gkatsev

@misteroneill misteroneill Awaiting requested review from misteroneill

Assignees
No one assigned
Labels
needs: LGTM Needs one or more additional approvals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@weiz18 @misteroneill @mister-ben @adrums86