(Note: originally filed as b/118348157 )

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be2b45dd817799382e343bd255208db8a32e5803

commit be2b45dd817799382e343bd255208db8a32e5803
Author: Trent Apted <tapted@chromium.org>
Date: Fri Oct 26 01:14:11 2018

CrOS: Consolidate keydown handlers in the audio player.

Currently media keys break in tablet mode, but a 'space' keypress in
the same mode works fine. Media keys are in a separate handler that
probably lost its plumbing when shadow dom was introduced in r592709.

Merge the two key handlers into the one that works, and add a test
for 'MediaPlayPause'.

Adds a mapping for '[' and ']' to control tracks since its easier to
test and there's no prior mapping. (Cmd+Shift+][ are next/previous
tab in Chrome Mac).

Bug:  899094 
Change-Id: Ie57b9b0d497b1996566ea3a1842309f0191d8bff
Reviewed-on: https://chromium-review.googlesource.com/c/1298820
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602965}
[modify] https://crrev.com/be2b45dd817799382e343bd255208db8a32e5803/ui/file_manager/audio_player/elements/audio_player.js
[modify] https://crrev.com/be2b45dd817799382e343bd255208db8a32e5803/ui/file_manager/audio_player/js/audio_player.js
[modify] https://crrev.com/be2b45dd817799382e343bd255208db8a32e5803/ui/file_manager/integration_tests/audio_player/click_control_buttons.js

This needs a merge to m71.

The change is in 72.0.3593.0 which is still pushing to devices, so I haven't verified this on hardware yet. I'll do that when able.

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

+beccahughes - do you know anything that may have changed around media key handling in m71?

I'm now stumped. r602965 has a test that failed without the fix, and passes with it. That suggested that the cause had been identified.

The 'MediaPlayPause' keybinding is now identical to what the video player does. The binding there works in tablet mode, but the one in the audio player still seems to be broken.

I don't think so, everything we are doing is behind a flag.

Thanks for following up!

I dug into this pretty much all day.

These break in tablet mode because of r155769, which landed 6 years ago.

In tablet mode, the app list is always visible, which wasn't a thing back then. r155769 was to fix  Issue 142067  - "Ctrl+Space not working when the applist was visible" - the cause:  https://crbug.com/142067#c2  -- the old applist gave focus to a button first.

But the new app list focuses the text entry field properly, so I think this no longer applies and r155769 can just be reverted.


There's more to it, and one part will be impacted by kMediaSessionAccelerators.

Specifically this bit:

void MediaController::HandleMediaPlayPause() {
  // If media session media key handling is enabled. Toggle play pause using the
  // media session service.
  if (base::FeatureList::IsEnabled(features::kMediaSessionAccelerators)) {
    if (GetMediaSessionController())
      GetMediaSessionController()->ToggleSuspendResume();
    return;
  }

  if (client_)
    client_->HandleMediaPlayPause();
}

That early return is going to skip the bit that does

  extensions::MediaPlayerAPI::Get(ProfileManager::GetActiveUserProfile())
      ->media_player_event_router()
      ->NotifyNextTrack();


This is how the audio player *should* be hooking in, but isn't doing it properly.

this diff -- https://chromium-review.googlesource.com/c/chromium/src/+/1312439/1/ui/file_manager/audio_player/js/background.js -- fixes that. But I'm going to try reverting r155769 first since other things are probably broken in tablet mode because of this "app list is always visible" thing.

Oh, also worth noting that r602965 _did_ fix a bug where the media keys didn't work when the audio player was first launched. It just didn't do anything to help tablet mode :(

For the kMediaSessionAccelerators feature this should still be okay as the MediaPlayer app is a Chrome app. In this case the Media Session service will be able to see the playback from the app and trigger play/pause directly on the underlying player. Next and Previous track can be hooked through the Media Session API when we enable it on Chrome OS: https://wicg.github.io/mediasession/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ecf5031139305dadb75c8b1a2b5ac7e080592a5

commit 8ecf5031139305dadb75c8b1a2b5ac7e080592a5
Author: Trent Apted <tapted@chromium.org>
Date: Mon Nov 05 01:18:56 2018

Ash: Dispatch accelerators correctly in Tablet mode.

This is basically a "revert" of r155769, which landed 6 years ago.

Problem: The hardware play/pause key currently stops working in the CrOS
Audio Player in tablet mode. The event is not dispatched to the web
contents. Note an external keyboard or USB headset can send this key
event in tablet mode, even though events from a convertible's regular
keyboard are suppressed.

r155769 introduced logic that causes all accelerators to be processed as
though they have the same priority as, e.g., Alt+Tab when the AppList is
visible. Now, in tablet mode, the AppList _always_ declares itself
visible. So currently no key combinations bound to Ash accelerators are
dispatched to webcontents when in tablet mode. This is bad.

r155769 was to fix  https://crbug.com/142067  - "Ctrl+Space unable to
switch keyboard layouts in the AppList". This occurred because an
AppList button had focus, not the text field. To retain current
behaviour, instead update the two Button subclasses to return false for
SkipDefaultKeyEventProcessing(), thereby prioritizing accelerators over
their 'space' activation. There are other buttons in the app list, but
they do not currently forward space to the search box (See
https://crbug.com/901245). These button types disappear or change focus
to the search box when searching anyway - only SearchResultBaseView
seems to really benefit from being able to handle Ctrl+Space.

Bug:  899094 
Test: AudioPlayerBrowserTest.NativeMediaKey
Change-Id: I1cb585d88d57a775fdfa15ecfdce197ae52dfcdd
Reviewed-on: https://chromium-review.googlesource.com/c/1312439
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605219}
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/accelerators/pre_target_accelerator_handler.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/app_list/views/app_list_item_view.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/app_list/views/app_list_item_view.h
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/app_list/views/search_result_base_view.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/app_list/views/search_result_base_view.h
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ash/shelf/back_button_unittest.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/chrome/browser/chromeos/file_manager/audio_player_browsertest.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
[modify] https://crrev.com/8ecf5031139305dadb75c8b1a2b5ac7e080592a5/ui/file_manager/integration_tests/audio_player/click_control_buttons.js

I'm calling this fixed, but removing labels. The flakiness fixed in #c2 may have been a regression, but it's unlikely that the fix in #c10 was for a regression, and that CL is not nice to merge.

I think the intersection of users relying on this functionality (i.e. those (a) with a ChromeOS tablet, (b) with a USB-C headset with play/pause, that (c) use the CrOS audio player) is too small to justify the risk.