Page MenuHomePhabricator

"Uncaught TypeError: event is undefined" using Phonos with keyboard on Firefox
Open, Needs TriagePublic3 Estimated Story PointsBUG REPORT

Description

What is the problem?

When I use the keyboard to listen to Phonos templates (i.e. pressing enter or space) on Firefox the audio does not play and I get an error in the browser console:

Uncaught TypeError: event is undefined
    onClick http://localhost:8081/wiki/Phonos_Natural line 10 > injectedScript:191
    emit http://localhost:8081/wiki/Phonos_Natural line 10 > injectedScript:200
    jQuery 3
Phonos_Natural line 10 > injectedScript:191:202

It does work on Chromium.

Steps to reproduce problem
  1. Go to a page with a Phonos template
  2. Tab until the template has focus
  3. Press enter

Expected behavior: You hear the audio
Observed behavior: You don't hear anything. Nothing appears to happen. If you have your browser console open, you see the above error.

Environment

Browser: Firefox 91, 102.
Wiki: local docker Phonos 0.1.0 (496ad09) 12:54, 3 October 2022.

Event Timeline

dmaza set the point value for this task to 3.Oct 5 2022, 2:53 PM

I looked into it and I'm a bit baffled! Firefox by itself doesn't seem to have any problems passing the event object to a click handler when using the enter key: https://jsfiddle.net/sh8zu7rd/ I couldn't find much on StackOverflow either, so I'm starting to believe it's something with OOUI.

ButtonWidget doesn't pass an argument to a click callback. Since it is simply an <a> element with no href, there is no "default" to "prevent" (and if there is href, attaching an OOUI click event alone blocks navigation).

Change 871287 had a related patch set uploaded (by Nardog; author: Nardog):

[mediawiki/extensions/Phonos@master] Clean up frontend events

https://gerrit.wikimedia.org/r/871287

Turns out it's because the event handler is named onClick, which has the effect of overriding OO.ui.mixin.ButtonElement.prototype.onClick, which is attached to jQuery events and returns false; hence the inconsistency between mouse click and Enter/Space. The handler should be renamed, but that restores PopupButtonWidget's default click handler so we need to suppress it.

Test wiki created on Patch demo by TheresNoTime using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/4fa39069f6/w

Change 871287 merged by jenkins-bot:

[mediawiki/extensions/Phonos@master] Clean up frontend events

https://gerrit.wikimedia.org/r/871287

@Nardog After this commit, error message popups do not appear when clicking Phonos tags.

For example, click any of the tags here https://en.wikipedia.beta.wmflabs.org/wiki/Phonos_Whitespace.

I tested this on Firefox and Chromium.

Locally I checked out the commit before this (6c6425e17c472d8548516134ee20b39a45e90312) and the error messages did work.

Change 875809 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/Phonos@master] Use parent click handlers for button click

https://gerrit.wikimedia.org/r/875809

@Samwilson This is not necessarily mutually exclusive with your solution, but I think we should simply stop disabling the button in case of an error, which appears to be the culprit of the issue @dom_walden reported. Disabling the button sets tabindex to -1, so currently keyboard users can't read the error message.

That's a good point! It's not disabled, really, given that it can still be clicked to get the popup.

(And sorry, I didn't mean to leap in with a hasty solution above, I just saw the onHtmlClick hangover and got interested.)

If we remove the disabling, I think we can still go with overriding the parent onClick(), which makes things a little bit cleaner. What do you think?

Wouldn't overriding onClick() make .on(), .emit(), .connect(), etc. not work? It could also stop working when the OOUI core is changed upstream. The mixin method is marked as protected. It doesn't strike me as scalable/future-proof.

Change 875813 had a related patch set uploaded (by Nardog; author: Nardog):

[mediawiki/extensions/Phonos@master] Stop disabling the button in case of error

https://gerrit.wikimedia.org/r/875813

Wouldn't overriding onClick() make .on(), .emit(), .connect(), etc. not work? It could also stop working when the OOUI core is changed upstream. The mixin method is marked as protected. It doesn't strike me as scalable/future-proof.

No, because we still call the parent method. And because this is a subclass, it's okay to call a protected method. To me, it feels like the right way to do it (i.e. we're modifying what happens when a user clicks, and this method is there for doing that), but it's totally fine to have a separate event handler instead.

Well, your patch seems to bring back the issue described in this task. Note keypress is handled by another method.

I don't know if my patch is the right way either so take it as a suggestion. Now I'm wondering if it should be a flag rather than just an HTML class.

Overriding onAction() is another possibility.

Test wiki on Patch demo by TheresNoTime using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/4fa39069f6/w/

Change 875809 merged by jenkins-bot:

[mediawiki/extensions/Phonos@master] Use parent click handlers for button click

https://gerrit.wikimedia.org/r/875809

Change 876029 had a related patch set uploaded (by HMonroy; author: HMonroy):

[mediawiki/extensions/Phonos@master] Define the keypress event function

https://gerrit.wikimedia.org/r/876029

Not sure if my patch is the best solution, but when testing this out I learned that Chrome handles the keypress as expected and Firefox didn't know what to do with this event at the parent level. This patch seems to solve the issue.

Change 876029 abandoned by HMonroy:

[mediawiki/extensions/Phonos@master] Define the keypress event function

Reason:

https://gerrit.wikimedia.org/r/876029

Not sure if my patch is the best solution, but when testing this out I learned that Chrome handles the keypress as expected and Firefox didn't know what to do with this event at the parent level. This patch seems to solve the issue.

Did not fully resolve the issue, I abandoned the patch.

This comment was removed by Nardog.