Feature: Add keyboard shortcut to toggle mute on expando videos

[FlaminSarge]

Relevant issue: N/A
Tested in browser: Chrome

This adds a keyboardNav bind (default 'm') to toggle mute/unmute on the current most visible video expando.

[FlaminSarge]

I'm seeing some inconsistency in behavior when pressing 'm' with different page states (just-after-reload, etc.), so this needs more testing, but I'm opening this PR as a draft now to get feedback.
I've fixed the inconsistency and I think this is good to go, outside of the fact that I have no idea how to write tests for RES. I'd appreciate some guidance on that.

[benmcgarry]

Don't think we need a test for this as its pretty straight forward behaviour.

[benmcgarry]

This clashes with the ModMail keyboard shortcut: https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/master/lib/modules/keyboardNav.js#L733-L739. M requires goMode to be enabled before actioning Modmail, With it turned off this PR would work, however with it enabled it errors out.

[FlaminSarge]

This clashes with the ModMail keyboard shortcut: master/lib/modules/keyboardNav.js#L733-L739. M requires goMode to be enabled before actioning Modmail, With it turned off this PR would work, however with it enabled it errors out.

Any suggestions on a solution? Should this shortcut only be allowed/enabled when goMode is enabled to avoid the clash? Or should it use another key by default? I'm hesitant to have it use shift-m/ctrl-m, as the ideal interaction is using J and K to scroll through posts and M to mute/unmute the current post, and adding a modifier kind of messes with that flow since shift-J would jump down a page instead if the user messes up their button order.

[FlaminSarge]

Updated to 'o' as that does not appear to be used at all.

[larsjohnsen]

I think it would be preferable to inline toggleMute. Also we might as well allow toggling mute on any video objects, not only RES's.

Probably something like this:

const video = downcast(getMostVisibleElementInThingByQuery('video', ASSERT), 'video');
video.muted = !video.muted;

[FlaminSarge]

I tried to do the mute-any-video handling in my initial impl but ran into a few hurdles involving how RES determines which entity is in focus; I wasn't too sure how to handle it, so if you have any further details that would be appreciated.

Also, would there be any benefit to leaving toggleMute in ShowImages rather than pulling it out to here? I was under the impression (from the rest of the file) that we tried to limit the amount of logic happening in keyboardNav.js and instead defer logic to the ShowImages module/etc, which is why my impl has it like that, but if it isn't needed, I'll pull the toggleMute logic up to here.

[larsjohnsen]

This clashes with the ModMail keyboard shortcut: https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/master/lib/modules/keyboardNav.js#L733-L739. M requires goMode to be enabled before actioning Modmail, With it turned off this PR would work, however with it enabled it errors out.

If I remember correctly, the user will be prompted whether m should be used for the modMail action or muting. Since probably relatively few users use the modmail shortcut, I think it will probably be okay to use m for toggling mute.

[FlaminSarge]

This clashes with the ModMail keyboard shortcut: master/lib/modules/keyboardNav.js#L733-L739. M requires goMode to be enabled before actioning Modmail, With it turned off this PR would work, however with it enabled it errors out.

If I remember correctly, the user will be prompted whether m should be used for the modMail action or muting. Since probably relatively few users use the modmail shortcut, I think it will probably be okay to use m for toggling mute.

Given that 'o' is unused entirely, it might just be fine to leave it on 'o' and let users rebind it to 'm' if they want. On a QWERTY layout it also lines up a little better for usage with J/K and [] for navigation, though it really isn't that much different from 'm' in that regard. Let me know which I should go with.

[larsjohnsen]

Thanks!

[FlaminSarge]

Thanks!

Ah, I was still in the process of seeing if I could get it working for any video element. I'll open up a followup PR for that, as well as for inlining instead of calling ShowImages if you think that's necessary as well.

[FlaminSarge]

@larsjohnsen @benmcgarry I could have sworn I set the keybind for this to 79 (o) but must have clobbered the change in a rebase at some point; do you folks have any opinions on changing it to o to remove the possibility of clashing with modmail (in a followup PR)?