Page MenuHomePhabricator
Create Task
Maniphest T319295

"Uncaught TypeError: event is undefined" using Phonos with keyboard on Firefox
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT
Actions

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.

Details

SubjectRepoBranchLines +/-
PhonosButton.js: Connect `playHandler` to click eventmediawiki/extensions/Phonosmaster+5 -8
Stop disabling the button in case of errormediawiki/extensions/Phonosmaster+15 -15
Define the keypress event functionmediawiki/extensions/Phonosmaster+5 -0
Use parent click handlers for button clickmediawiki/extensions/Phonosmaster+14 -12
Clean up frontend eventsmediawiki/extensions/Phonosmaster+5 -9
Customize query in gerrit

Related Objects

Event Timeline

dom_walden created this task.Oct 4 2022, 1:11 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 4 2022, 1:11 PM
dmaza moved this task from New & TBD Tickets to Needs Discussion on the Community-Tech board.Oct 5 2022, 2:51 PM
dmaza set the point value for this task to 3.Oct 5 2022, 2:53 PM
TheresNoTime moved this task from Needs Discussion to Up Next on the Community-Tech board.Oct 10 2022, 1:54 PM
MusikAnimal subscribed.Nov 3 2022, 1:32 AM

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.

• JMcLeod_WMF moved this task from Up Next to CommTech-Sprint-38 on the Community-Tech board.Dec 1 2022, 6:31 PM
• JMcLeod_WMF edited projects, added Community-Tech (CommTech-Sprint-38); removed Community-Tech.
• JMcLeod_WMF moved this task from Backlog to 🌟Top Priority on the MediaWiki-extensions-Phonos board.Dec 1 2022, 6:33 PM
Nardog subscribed.EditedDec 16 2022, 2:48 PM

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).

gerritbot added a comment.Dec 25 2022, 4:54 AM

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

gerritbot added a project: Patch-For-Review.Dec 25 2022, 4:54 AM
Nardog added a comment.Dec 25 2022, 8:10 AM

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.

PatchDemoBot added a comment.Dec 25 2022, 9:09 PM

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

TheresNoTime subscribed.Dec 25 2022, 9:11 PM
• JMcLeod_WMF edited projects, added Community-Tech (CommTech-Sprint-39); removed Community-Tech (CommTech-Sprint-38).Jan 3 2023, 6:37 PM
dmaza moved this task from Ready 🎬 to Review/Feedback 💬 on the Community-Tech (CommTech-Sprint-39) board.Jan 3 2023, 10:04 PM
gerritbot added a comment.Jan 4 2023, 6:02 PM

Change 871287 merged by jenkins-bot:

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

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

Nardog mentioned this in rEPHNa6a4d93498bb: Clean up frontend events.Jan 4 2023, 6:03 PM
MusikAnimal assigned this task to Nardog.Jan 4 2023, 6:15 PM
MusikAnimal moved this task from Review/Feedback 💬 to QA 🐛 on the Community-Tech (CommTech-Sprint-39) board.
Maintenance_bot removed a project: Patch-For-Review.Jan 4 2023, 6:30 PM
ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.18; 2023-01-09).Jan 4 2023, 7:00 PM
dom_walden moved this task from QA 🐛 to In Development 💻 on the Community-Tech (CommTech-Sprint-39) board.EditedJan 5 2023, 8:25 AM

@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.

gerritbot added a comment.Jan 5 2023, 9:35 AM

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

gerritbot added a project: Patch-For-Review.Jan 5 2023, 9:35 AM
Nardog added a subscriber: Samwilson.EditedJan 5 2023, 9:40 AM

@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.

Samwilson added a comment.Jan 5 2023, 9:46 AM

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?

Nardog added a comment.Jan 5 2023, 9:58 AM

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.

gerritbot added a comment.Jan 5 2023, 10:15 AM

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

Samwilson added a comment.Jan 5 2023, 11:07 AM

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.

Nardog added a comment.EditedJan 5 2023, 11:44 AM

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.

Nardog added a comment.Jan 5 2023, 11:50 AM

Overriding onAction() is another possibility.

PatchDemoBot added a comment.Jan 5 2023, 8:54 PM

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

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

gerritbot added a comment.Jan 5 2023, 9:08 PM

Change 875809 merged by jenkins-bot:

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

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

Samwilson mentioned this in rEPHN56806e8442f7: Use parent click handlers for button click.Jan 5 2023, 9:09 PM
gerritbot added a comment.Jan 5 2023, 11:40 PM

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

HMonroy subscribed.EditedJan 5 2023, 11:43 PM

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.

• JMcLeod_WMF removed Nardog as the assignee of this task.Jan 11 2023, 8:05 PM
• JMcLeod_WMF moved this task from In Development 💻 to Ready 🎬 on the Community-Tech (CommTech-Sprint-39) board.
gerritbot added a comment.Jan 11 2023, 8:21 PM

Change 876029 abandoned by HMonroy:

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

Reason:

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

HMonroy added a comment.Jan 11 2023, 8:23 PM
In T319295#8503473, @HMonroy wrote:

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.

Nardog added a comment.Jan 12 2023, 12:31 AM
This comment was removed by Nardog.
Nardog mentioned this in T326828: Indicate progress during playback.Jan 12 2023, 5:12 PM
• JMcLeod_WMF edited projects, added Community-Tech (CommTech-Sprint-40); removed Community-Tech (CommTech-Sprint-39).Jan 16 2023, 8:00 PM
TheresNoTime claimed this task.Feb 16 2023, 8:06 PM
TheresNoTime moved this task from Ready 🎬 to In Development 💻 on the Community-Tech (CommTech-Sprint-40) board.
gerritbot added a comment.Feb 16 2023, 8:32 PM

Change 875813 merged by jenkins-bot:

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

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

TheresNoTime mentioned this in rEPHNa240da9a3c6b: Stop disabling the button in case of error.Feb 16 2023, 8:33 PM
ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.24; 2023-02-20); removed MW-1.40-notes (1.40.0-wmf.18; 2023-01-09).Feb 16 2023, 9:00 PM
Maintenance_bot removed a project: Patch-For-Review.Feb 16 2023, 9:10 PM
gerritbot added a comment.Feb 16 2023, 9:16 PM

Change 889866 had a related patch set uploaded (by Samtar; author: Samtar):

[mediawiki/extensions/Phonos@master] PhonosButton.js: Add keyDown handler, split playHandler

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

gerritbot added a project: Patch-For-Review.Feb 16 2023, 9:16 PM
PatchDemoBot added a comment.Feb 16 2023, 9:25 PM

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

PatchDemoBot added a comment.Feb 16 2023, 9:28 PM

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

https://patchdemo.wmflabs.org/wikis/a7c882b1cc/w/

PatchDemoBot added a comment.Feb 16 2023, 9:36 PM

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

TheresNoTime moved this task from In Development 💻 to Review/Feedback 💬 on the Community-Tech (CommTech-Sprint-40) board.Feb 16 2023, 9:38 PM
TheresNoTime moved this task from Review/Feedback 💬 to In Development 💻 on the Community-Tech (CommTech-Sprint-40) board.Feb 20 2023, 10:10 AM
TheresNoTime moved this task from In Development 💻 to Review/Feedback 💬 on the Community-Tech (CommTech-Sprint-40) board.Feb 20 2023, 1:16 PM
gerritbot added a comment.Feb 21 2023, 2:14 AM

Change 889866 merged by jenkins-bot:

[mediawiki/extensions/Phonos@master] PhonosButton.js: Connect `playHandler` to click event

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

Samwilson mentioned this in rEPHN3dc8031efd4e: PhonosButton.js: Connect `playHandler` to click event.Feb 21 2023, 2:14 AM
Maintenance_bot removed a project: Patch-For-Review.Feb 21 2023, 2:29 AM
Samwilson moved this task from Review/Feedback 💬 to QA 🐛 on the Community-Tech (CommTech-Sprint-40) board.Feb 21 2023, 2:40 AM
ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.25; 2023-02-27); removed MW-1.40-notes (1.40.0-wmf.24; 2023-02-20).Feb 21 2023, 3:00 AM
dom_walden closed this task as Resolved.Feb 22 2023, 9:12 AM
dom_walden moved this task from QA 🐛 to Done 🏁 on the Community-Tech (CommTech-Sprint-40) board.

I tested that play, pause and replay and showing and dismissing error popups could work with the keyboard (pressing either enter or space) and the mouse on various browsers (see below).

I also checked that the events being passed to statsv were the same when I clicked and when I pressed enter or space.

I briefly tested different skins (Vector 2022, Vector 2010, Minerva, MonoBook, Timeless) on Firefox, although I don't think this will make a difference.

Test environment: https://en.wikipedia.beta.wmflabs.org Phonos 0.1.0 (3dc8031) 02:11, 21 February 2023.
Browsers/platforms tested:

  • Firefox 91 Debian Bullseye
  • Firefox 102 Debian Bullseye
  • Chromium 110 Debian Bullseye
  • Firefox 110 Windows 11 (emulated)
  • Edge 103 Windows 11 (emulated)
  • IE11 Windows 10 (emulated)
  • Chrome 109 Windows 8.1 (emulated)
  • Firefox 95 Windows 8.1 (emulated)
  • Chrome 110 Mac Ventura (emulated)
  • Safari 16 Mac Ventura (emulated)
  • Edge 107 Mac Ventura (emulated)
  • iPhone 14 Chrome (emulated)
  • iPhone 14 Safari (emulated)

Platforms I could not test:

  • Android (Browserstack does not pass audio through. The animation suggested that it was playing.)
  • Firefox on Mac (There seems to be a bug in Browserstack which stops the Phonos buttons playing. We confirmed that they could play on a non-emulated Mac.)
PatchDemoBot added a comment.May 24 2023, 4:51 PM

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

https://patchdemo.wmflabs.org/wikis/abbc54d96d/w/

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL