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

Add polyfill for making MediaQueryList extend the EventTarget interface #1071

Merged

Conversation

santialbo
Copy link
Contributor

As discussed in #1069. I separated the implementation as a new polyfill so it can be included independently.
I have several concerns:

  • I dind't include the implementation of dispatchEvent. If the implementation is not the one in the matchMedia polyfill there's is no way that I know of accessing the list of listeners.
  • I'm not sure the path I chose is the correct. Please let me know if you would put it somewhere else and I'll make the change right away.
  • I'm not sure I've got the config.toml right.

@santialbo santialbo requested a review from a team as a code owner July 20, 2021 09:42
@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ Jul 20, 2021
@github-actions github-actions bot added the library Relates to an Origami library label Jul 20, 2021
@santialbo santialbo force-pushed the media-query-list-event-target branch from 486c133 to 43c98af Compare July 20, 2021 09:43
@romainmenke
Copy link
Collaborator

see : santialbo#1

@JakeChampion To really test event listeners on MediaQueryList I needed to have iframes that are polyfilled.
To achieve this I extended test/polyfills.

In short :

  • new endpoint in the test server : iframe.html
  • uses createEndpoint so doesn't add more complexity in server.js
  • test-iframe.handlebars mirrors test-runner.handlebars but only loads the polyfill javascript and nothing else
  • test-runner.handlebars has a <meta> tag to expose the correct URL for the URL to tests.

in tests :

var iframe = document.createElement("iframe");
iframe.src = document.getElementById('test-iframe-src').getAttribute('content');

The result is a same origin iframe with all polyfills applied.

Feel free to shoot this down :)
Maybe there is another way to test this polyfill without modifying the test server?

@JakeChampion
Copy link
Owner

I don't see another way, I like this approach 👍

@santialbo
Copy link
Contributor Author

This is now updated with your changes @romainmenke

@JakeChampion
Copy link
Owner

The current changes in this pull-request would likely be breaking changes due to the removal of two feature names:

  • MediaQueryList.prototype.EventTarget
  • MediaQueryList.prototype.dispatchEvent

If we wanted to keep this change backwards compatible, we should add those two feature names as aliases to the file polyfills/MediaQueryList/prototype/addEventListener/config.toml

@romainmenke
Copy link
Collaborator

@JakeChampion Is it possible you were looking at New changes since you last viewed ?
I removed those two aliases from this PR because the features are not implemented (or not fully).
They were however new additions, not existing aliases on the main branch.

@JakeChampion
Copy link
Owner

@JakeChampion Is it possible you were looking at New changes since you last viewed ?
I removed those two aliases from this PR because the features are not implemented (or not fully).
They were however new additions, not existing aliases on the main branch.

Yep 🤦 - good work noticing what I did there 😅

@JakeChampion JakeChampion enabled auto-merge (rebase) August 2, 2021 14:35
Origami ✨ automation moved this from incoming to active Aug 2, 2021
@JakeChampion JakeChampion merged commit 16b6c16 into JakeChampion:master Aug 2, 2021
Origami ✨ automation moved this from active to complete Aug 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2022
@robertboulton robertboulton removed this from Done in Origami ✨ Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library Relates to an Origami library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants