fix Safari 13 addEventListener on MediaQueryList

nikoPLP commented

2024-07-28 09:05:52 +02:00

(Migrated from github.com)

Please accept this PR that fixes an error on Safari 13 because addEventListener is not available yet on MediaQueryList.
see https://github.com/mdn/sprints/issues/858 for more description of the issue

Please accept this PR that fixes an error on Safari 13 because addEventListener is not available yet on MediaQueryList. see https://github.com/mdn/sprints/issues/858 for more description of the issue

marijnh commented

2024-07-28 09:12:12 +02:00

(Migrated from github.com)

onPrint actually looks at its event argument. I don't see any mention of such an object being passed to addListener callbacks in https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList/addListener . Are you sure the patch works the way you wrote it? (I don't have a Safari 13 around to test.)

`onPrint` actually looks at its event argument. I don't see any mention of such an object being passed to `addListener` callbacks in https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList/addListener . Are you sure the patch works the way you wrote it? (I don't have a Safari 13 around to test.)

nikoPLP commented

2024-07-28 09:35:33 +02:00

(Migrated from github.com)

That's the fastest PR review I have ever seen in the whole FOSS world!
I didn't test that it actually works when the print is triggered. Is it what this code is doing?
I was more concerned about having the code execute in general (the whole codemirror was broken because of that).
I have read the code though, and it should work.
I can see we get an argument that looks exactly the same as the one received in the newer API addEventListener:

interface MediaQueryListEvent extends Event {
    readonly matches: boolean;
    readonly media: string;
}

It has a matches boolean and inherits a type from Event.

And yes, the old API addListener doesn't take a type parameter like the new one does (with "change" value)

If you look at the commits of other people who solved this before us, they did exactly like what I propose.
https://github.com/h2oai/wave/pull/394/files

That's the fastest PR review I have ever seen in the whole FOSS world! I didn't test that it actually works when the print is triggered. Is it what this code is doing? I was more concerned about having the code execute in general (the whole codemirror was broken because of that). I have read the code though, and it should work. I can see we get an argument that looks exactly the same as the one received in the newer API `addEventListener`: ``` interface MediaQueryListEvent extends Event { readonly matches: boolean; readonly media: string; } ``` It has a `matches` boolean and inherits a `type` from Event. And yes, the old API `addListener` doesn't take a type parameter like the new one does (with "change" value) If you look at the commits of other people who solved this before us, they did exactly like what I propose. https://github.com/h2oai/wave/pull/394/files

nikoPLP commented

2024-07-28 09:56:28 +02:00

(Migrated from github.com)

I have checked with a console.log(event) at the beginning of onPrint and this is what I get:
when print is triggered:

{
  media: "print",
  matches: true
}

when the print is finished:

{
  media: "print",
  matches: false
}

so I believe that your guard if (event.type == "change" && !(event as MediaQueryListEvent).matches) return is never going to be useful on Safari 13, but tha's ok, right?
The print works fine.

I have checked with a `console.log(event)` at the beginning of `onPrint` and this is what I get: when print is triggered: ``` { media: "print", matches: true } ``` when the print is finished: ``` { media: "print", matches: false } ``` so I believe that your guard `if (event.type == "change" && !(event as MediaQueryListEvent).matches) return` is never going to be useful on Safari 13, but tha's ok, right? The print works fine.

nikoPLP commented

2024-07-28 10:22:47 +02:00

(Migrated from github.com)

BTW, you can also test on your end, as the old API is available on all browsers.
Some people have even solved this issue by always using the old API addListener here in this case of media queries, even if it is deprecated.

BTW, you can also test on your end, as the old API is available on all browsers. Some people have even solved this issue by always using the old API `addListener` here in this case of media queries, even if it is deprecated.

marijnh commented

2024-07-28 15:26:32 +02:00

(Migrated from github.com)

I've merged this and followed up with a patch that makes the listener ignore matches: false objects.

I've merged this and followed up with a patch that makes the listener ignore `matches: false` objects.

👍 1

nikoPLP commented

2024-07-28 15:27:49 +02:00

(Migrated from github.com)

very nice! do you know when the next release to npm will be?

marijnh commented

2024-07-29 12:37:40 +02:00