Closed Bug 730192 Opened 12 years ago Closed 8 years ago

Headset playback controls (media keys) do not work while Firefox is open

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(firefox14 affected, firefox15 affected, firefox47 fixed, blocking-fennec1.0 -, fennec-)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox14 --- affected
firefox15 --- affected
firefox47 --- fixed
blocking-fennec1.0 --- -
fennec - ---

People

(Reporter: ravi_n+mozilla, Assigned: bob+bmo)

References

Details

Attachments

(2 files, 6 obsolete files)

logcat
144.72 KB, text/plain
Details
allow headset controls to function by passing media keyevents through to Android OS
16.53 KB, patch
jchen
: review+
Details | Diff | Splinter Review 
The summary pretty-much covers it. None of my Bluetooth headset playback controls (track forward, track backwards, play/pause) work when Firefox is open, unlike most other applications.

Even though the submission limits the bug to Fennec Native, it is actually more general than that. I've observed it with:

- pretty much every Android release from Firefox 4 on (when I started using Firefox on Android), including recent downloads of Aurora
- on devices with Froyo, Gingerbread, Honeycomb and Ice Cream Sandwich
- with at least 3 different models of Bluetooth headset
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: Other → ARM
Version: unspecified → Trunk
I had remembered this differently (so it might have changed at some point), but the playback controls on my ASUS Transformer keyboard dock also don't work when Firefox is open. 

This is less irritating because the controls from Google Music's (or Hive Player's) playback notification work fine, but I thought this was worth noting.
Mozilla QA will try and get a bluetooth headset to see if we can reproduce this
Dclarke has a bluetooth headset.  I've asked him to help take a look.
I was able to reproduce this, on Nexus S 2.3.6. 

Some things I noticed, the bluetooth service was not killed during this time. 

Possibly this is to do with some of the bluetooth service.
Actually I was thinking some of this could be webapi related, but I am unsure, as the specific components I was thinking about are compiled with B2g, and shouldn't have anything to do with FF4 and up.
blocking-fennec1.0: --- → ?
This is not something we'd want to ship with, but we shouldn't be doing anything with those events. Fennec blocking+.

Sicking, can I give this to you to look at/reassign?
Assignee: nobody → jonas
blocking-fennec1.0: ? → +
I have no idea who this should go to. None of the webapi work for bluetooth is being built on Fennec, and none of it goes as far back as FF4.

Sounds like a generic Fennec problem to me. Sending over to Brad for further triage.
Assignee: jonas → blassey.bugs
I also have no idea who this should go to. Since this isn't a regression in native fennec, I have a hard time believing we'd stop the ship for it. Moving to a soft blocker.
blocking-fennec1.0: + → soft
(In reply to dclarke@mozilla.com from comment #4)
> I was able to reproduce this, on Nexus S 2.3.6. 
> 
> Some things I noticed, the bluetooth service was not killed during this
> time. 
> 
> Possibly this is to do with some of the bluetooth service.

david, can you comment if playback controls was running the background or the foreground when you reproduced?
Keywords: qawanted
I didn't have an issue with Plantronics Marque M155 and Samsung Galaxy S II.

I was able to bring the Voice Control system for Samsung Galaxy S II up while Fennec was running with today's nightly build.  

This was while I was on youtube having a flash play through the headset while also having music playing through the headset.

(ie playback controls were in the background, Fennec was in the foreground when I tested; this brought the playback controls on the foreground and fennec in the background)
Nexus S 4.03
Headset: LG HBS700
Fennec: Aurora (13)/ Nightly(14).. 

Now I think we have a slight disagreement as to what playback controls are. 

On my bluetooth headset I have controls that allow me to move forward / backward to the previous / next song, and control the volume. 

When I am in Google Music, I can advance to the next song, by just pressing the button. Similarly If I am on the home screen I can advance to the next song. 
google Talk, calculator, I can all continue to advance / retreat my playlist. 

When I load firefox, this stops working.
What I had imagined is firefox was registering to receive bluetooth events in some manner, and some of these messages are broadcast to all listeners, and some can only be received by one application target. 

If someone can give guarantees that there is no bluetooth code that is being used by fennec, or rogue code that is being loaded when it should only be loaded on B2g.
I'm not sure if we're using the bluetooth api at all in our java code?
http://developer.android.com/guide/topics/wireless/bluetooth.html
(In reply to Naoki Hirata :nhirata from comment #13)
> I'm not sure if we're using the bluetooth api at all in our java code?
> http://developer.android.com/guide/topics/wireless/bluetooth.html


maybe not in java code, but possibly in gecko.  bug 713116, bug 729991.

removing qawanted.
Keywords: qawanted
It is probably worth mentioning that this may not be Bluetooth-specific issue (even though that is how I first noticed it). There are two reasons I think that:
1. The same thing happens with the playback controls on my Transformer keyboard dock (though I suppose those might be pretending to be a Bluetooth device at some level)
2. I've seen similar behavior from apps that hook into the volume controls for things like page scrolling (though, IIRC, these issues usually went away after an update and/or disabling the volume controls in the app).
my best guess is that this is a dupe of bug 746696 which is now fixed. qawanted to re-test and mark as a dupe if it works now.
Keywords: qawanted
We do not have access to bluetooth headset on SV side. Can someone else please test this on the latest Nightly and add the info.
(In reply to adrian tamas from comment #17)
> We do not have access to bluetooth headset on SV side. Can someone else
> please test this on the latest Nightly and add the info.

Dclarke: do you still have your BT headset?  if so, can you try on the latest version of nightly?  (bug 746696 is supposedly fixed)
I've tried with the latest Nightly I could find (2012-05-15) and this isn't fixed. 

It may have improved: When I first open Nightly the controls still work, it is only after I go to a few web pages that they stop working (usually, but not always the first page - going to Firefox Support is enough, for example, but a Google search might not be).
(In reply to ravi_n+mozilla from comment #19)
> I've tried with the latest Nightly I could find (2012-05-15) and this isn't
> fixed. 
> 
> It may have improved: When I first open Nightly the controls still work, it
> is only after I go to a few web pages that they stop working (usually, but
> not always the first page - going to Firefox Support is enough, for example,
> but a Google search might not be).

how are you testing? can you define not working? Also, can you provide a log for when they stop working?
My test script is the following:

1. Start music playing over my headset
2. Try playback controls (track forward, track back, play, pause) to make sure they work
3. Open Nightly
4. Try playback controls again - these usually work (affects the music playing)
5. Go to a web page (Firefox Support is handy on the opening screen)
6. Wait until the page is done loading
7. Try playback controls again - this usually fails (pressing buttons with no impact on the music playing)

In terms of a log, are you looking for a logcat of that sequence or something else?
(In reply to ravi_n+mozilla from comment #21)
> My test script is the following:
> 
> 1. Start music playing over my headset
> 2. Try playback controls (track forward, track back, play, pause) to make
> sure they work
> 3. Open Nightly
> 4. Try playback controls again - these usually work (affects the music
> playing)
> 5. Go to a web page (Firefox Support is handy on the opening screen)
> 6. Wait until the page is done loading
> 7. Try playback controls again - this usually fails (pressing buttons with
> no impact on the music playing)
> 
> In terms of a log, are you looking for a logcat of that sequence or
> something else?

Yep, logcat would be helpful.
(In reply to Tony Chung [:tchung] from comment #18)
> (In reply to adrian tamas from comment #17)
> > We do not have access to bluetooth headset on SV side. Can someone else
> > please test this on the latest Nightly and add the info.
> 
> Dclarke: do you still have your BT headset?  if so, can you try on the
> latest version of nightly?  (bug 746696 is supposedly fixed)

I don't have a BT headset right now, I misplaced it.  I will order a new one.
awaiting on more logging feedback from anyone that has a headset to test this.
Attached file logcat — 
Bought a LG HBS-700 Bluetooth Stereo Headset to test, since the bluetooth device I had before didn't have a play button.  This one has a push to call, button as well as a play, forward, backwards button and volumes button.

The play, forward and backwards button do not work when Fennec is in the foreground.  If the screen lock is on, then it will work (without pushing fennec to the background).

The phone, and volume buttons will work when Fennec is in the foreground or not.

Basically Fennec is taking in those bluetooth signals, when it should not be taking in the signals, it should be passing them to the music app; which is what it does with the default browser as well as when there's no app in the foreground.
removing QA wanted.  Handed off bluetooth headset to cpeterson to further investigate the issue.
Keywords: qawanted
Assignee: blassey.bugs → cpeterson
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-fennec: --- → 15+
blocking-fennec1.0: soft → -
Priority: -- → P3
Do we still care about this bug for Firefox 15? This bug does not directly affect our browser behavior, just the audio playback of other apps.
tracking-fennec: 15+ → ?
The likely problem is that dom/plugins/base/android/ANPAudio.cpp is retaining an android.media.AudioTrack object when Firefox is paused to the background.
tracking-fennec: ? → -
Assignee: cpeterson → nobody
I found that a key event of media buttons is firing but a MEDIA_BUTTON intent isn't broadcasted.
Overring dispatchKeyEvent() in GeckoView (or LayerView) may works.

--
@Override
public boolean dispatchKeyEvent(KeyEvent event) {
    if (interceptMediaKey(event)) {
        return true;
    }
    return super.dispatchKeyEvent(event);
}

And the implementation of interceptMediaKey() will be as bellow. (Unfortunately, it depends on API level)

 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/2.3_r1/com/android/internal/policy/impl/KeyguardViewBase.java/?v=source
 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.1_r1/com/android/internal/policy/impl/KeyguardViewBase.java/?v=source
 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.4_r1/com/android/keyguard/KeyguardViewBase.java#KeyguardViewBase/?v=source

Is there any better solution?
Still occurs on Firefox 44.

As #c15 suggested, this is not Bluetooth-specific. It also occurs with standard TRRS headsets, e.g. most handsfree headsets.

I've seen this happening on several devices (Samsung Galaxy S2, Samsung Galaxy S4, LG G4) with a range of Android OS versions (4.0, 4.3, 5.1). It only occurs when Firefox is in the foreground; no other app exhibits this behaviour.
I've done some digging, and I think I've found it... though I've got absolutely zero experience with Android so this might all be wrong.

It's all in /mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoInputConnection.java, used by GeckoView.java.

onKeyDown and onKeyUp there both call processKey (GeckoInputConnection.java#849). processKey calls shouldProcessKey (GeckoInputConnection.java#806) - if that returns true, then the event is *always* swallowed (no non-true return from processKey after that point).

shouldProcessKey: http://lxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoInputConnection.java#806

> 806     private boolean shouldProcessKey(int keyCode, KeyEvent event) {
> 807         switch (keyCode) {
> 808             case KeyEvent.KEYCODE_MENU:
> 809             case KeyEvent.KEYCODE_BACK:
> 810             case KeyEvent.KEYCODE_VOLUME_UP:
> 811             case KeyEvent.KEYCODE_VOLUME_DOWN:
> 812             case KeyEvent.KEYCODE_SEARCH:
> 813                 return false;
> 814         }
> 815         return true;
> 816     }

Notice that it specifically ignores volume up/down, which do work.

I would suggest that all of the KEYCODE_MEDIA_* keys are added to this list, since it appears that Firefox has no other use for them. Additionally, there are a number of other keys that might be worth ignoring - or, better yet, this should be converted to a whitelist of keys Firefox actually handles, if possible. At the very least, KEYCODE_TV_*, KEYCODE_SLEEP and KEYCODE_POWER sound like good candidates. KEYCODE_VOLUME_MUTE might be handled by Firefox (?), but if it isn't then it should at least be passed on.

Hopefully someone more familiar with Android dev and the Firefox codebase can check if this is correct.
Ok, looks like they *are* passed on to the webpage. That complicates matters.

http://lxr.mozilla.org/mozilla-central/source/widget/NativeKeyToDOMKeyName.h#914

Interestingly, volume up/down are also documented as passed, yet they are on the "do not process" list.

Still looks like the key is always swallowed even when there is no handler on the webpage.

IMO, the easiest way to satisfy the most common case is to just ignore those keys. The best way is to detect if they're handled by JS. I have no idea how to do the latter. Another possibility is to pass the events to JS but also pass them on to Android (i.e. don't suppress the event).

The current situation defies user expectations for negligible benefit - I can't remember a single time I've used media keys to control a webpage, but I've run into this issue almost daily while listening to music - and other comments here plus the duplicate reports suggest this is a somewhat common occurrence.

I'll test to make sure my guess is correct as soon as I can dig up a desktop *nix environment, but I'd appreciate it if someone else could double-check.
The keys cannot be ignored. This would disable the adjustment of the volume of media playback in the browser.
(In reply to Kevin Brosnan [:kbrosnan] from comment #36)
> The keys cannot be ignored. This would disable the adjustment of the volume
> of media playback in the browser.

But... the volume keys already *are* ignored, AFAICT. They're ignored by the browser and passed on to Android, which then controls the media playback volume (?). That works as expected. However, the play/pause, next, back buttons do not work - and it appears to be because Firefox always swallows them, even when they have no handler. As before, the best solution would be to only suppress the event if a handler actually exists (e.g. if there's media playing in the browser) but pass it on otherwise.
Should also be noted that a video playing on YouTube does not respond to play/pause anyway.

Also, using the JS Key Event test (https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html):

* Volume up/down is not detected at all (which lends credence to my current guess that they are completely ignored already, and handled by Android rather than Firefox).
* Play/pause is detected (events fire) but the key cannot be identified (charCode=0, keyCode=0, which=0, key='Unidentified', code=undefined, location=0, repeat=false, data=undefined)

Tested on an LG G4 running Android 5.1.

So, as it is, media play/pause is already unusable.

Note that Chrome for Android does not fire the events at all! So Firefox behaviour already differs from Chrome, and is also completely broken with play/pause.

Considering all this, ignoring the events entirely seems like it'll have no downsides - the current implementation is broken enough that I don't think there's any way any existing website uses it - or *can* use it - while it also has significant negative impact on normal Android operation/expectations.
Forgot to test, and a correction to my previous comment:

Chrome for Android does apply media play/pause to <video>.

Firefox for Android does *not* currently apply media/play pause to <video>.

Tested at http://camendesign.com/code/video_for_everybody/test.html

Again, ignoring entirely is not worse in any way than what it currently does, but might want to consider also fixing it to work for when <video> (or <audio>, I guess) exists on page. I'd still suggest ignoring the event for now as a quick fix while a more complete fix is considered (which should still not suppress the event when no video exists on the page).
Just got someone else to confirm, they get the same behaviour with a wired headset.

> OK so confirmed it does exactly as you describe with wired headset.
> Anytime firefox window is active, play/pause on the headset doesn't work
> Any time it's backgrounded, hidden, behind lockscreen, etc. it works fine.
> Volume keys always works
> Latest Firefox 44.0 on Android 5.0 by the way
> Confirmed exact same behaviour via standard Bluetooth headset.
> Incidentally, the call/redial button does work

On a Galaxy S5.

https://chat.stackexchange.com/transcript/message/27460722#27460722

Could not repro with a smartwatch, though we suspect might be because the smartwatch doesn't use standard AVRCP keypresses - since they communicate with an app that lets you specify a target media player.

In light of this info, could someone with editbugs please edit the summary to remove "Bluetooth"? Multiple people have confirmed that this does occur with wired headsets too.
Ok, I've tried the fix proposed in comment 34. It works :)

Tested on Android 5.1 on a LG G4, with both a wired headset and a Bluetooth speaker, using Spotify as the audio player. The wired headset actually fires KEYCODE_HEADSETHOOK for all its play/pause/next/prev actions - this is the one that reported 'Unidentified' in comment 38.

Unfortunately, it turns out that the Bluetooth speaker actually fires MediaPlay/MediaPause/MediaTrackNext/MediaTrackPrevious JS keypress events. These are not handled by Firefox itself as far as I can tell (video does not pause, unlike Chrome), but I suppose some webpages might be using it.

Chrome's behaviour: it passes these events through unless preventDefault is called on the keyup (not the keydown).

My proposal: best case, Firefox mimics Chrome's behaviour. It makes sense. If not, then Firefox should *always* pass the event through. The easy way would also hide these events from the webpage, but it's only marginally more difficult to continue letting them go to the webpage (with the caveat that preventDefault is impossible).

Note that none of these proposed changes affect the *volume* controls, which are already always passed through and never exposed to JS.
Sample patch implementing the easy first option from comment 41 (side-effect of hiding the events from the webpage) 

I suggest this at least as a temporary measure because it is intuitive behaviour to me, and almost certainly the more common use-case. Lots of people listening to music while browsing. Not so many people listening to music or watching videos inside a mobile browser, and fewer yet who'll use media controls while doing so. And even then the webpage would have to implement them - YouTube does not seem to do so.
Flags: needinfo?(cpeterson)
Having had another look, it actually might be possible to just check the return value of keyListener.onKeyUp to see if it was preventDefault'd.  I'll test it later today. If that's true, then it might not be so hard to mimic Chrome behaviour.

Looks like they actually redispatch it if it wasn't handled: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java&l=453

Should Firefox do the same?
Comment on attachment 8718072 [details] [diff] [review]
allow headset controls to function by passing keyevents through to Android OS

Thanks for the patch, Bob!

Jim, can you please take a look at Bob's fix? I no longer work on Firefox for Android. :)
Flags: needinfo?(cpeterson)
Attachment #8718072 - Flags: feedback?(nchen)
/me assigning bug to Bob.
Assignee: nobody → bob+bmo
Digging a bit more, TextKeyListener.onKeyDown/onKeyUp looks like it always returns false on non-character keys. Then we get into GeckoEditable.sendKeyEvent ... and run into a wall. The native code that calls into simply does not provide any form of notification of whether preventDefault was called. nsWindow::GeckoViewSupport::OnKeyEvent would need to be modified to pass back the status. This seems to be a relatively significant change.

I have a vague idea of how to implement it, and might try when time permits, but I'm not familiar with the Mozilla patch process - would this be better off split into one (or more) other bugs?
Flags: needinfo?(nchen)
Thanks for investigating! Passing back the status could be tricky, because this whole process is asynchronous. You would end up in situations where default-prevented keys are processed out-of-order w.r.t. non-default-prevented keys, and I'm not sure what kind of implications that has. In any case, I think it's worth a try. Doing that in this bug is fine.

You could pass the original KeyEvent reference to nsWindow::GeckoViewSupport::OnKeyEvent, and check the status. If it's non-default-prevented, then pass that reference back to GeckoEditable.
Flags: needinfo?(nchen)
Comment on attachment 8718072 [details] [diff] [review]
allow headset controls to function by passing keyevents through to Android OS

Review of attachment 8718072 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense as a workaround.
Attachment #8718072 - Flags: feedback?(nchen) → review+
Jim, are you going to land Bob's patch?

Bob, do you have permission to run automated tests on the Try Server? If not, then perhaps Jim can run one so that your patch can be checked in. If you add the "checkin-needed" keyword to the bug report, the patch will be checked in by a "tree sheriff", but only if there is a link to a Try Server run.

https://wiki.mozilla.org/ReleaseEngineering/TryServer
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #47)
> Thanks for investigating! Passing back the status could be tricky, because
> this whole process is asynchronous. You would end up in situations where
> default-prevented keys are processed out-of-order w.r.t.
> non-default-prevented keys, and I'm not sure what kind of implications that
> has. In any case, I think it's worth a try. Doing that in this bug is fine.

Ah, I missed that the call from UI to IC thread is async. But everything from there seems to be sync. So it's not possible to pass the original event on, but we can generate a new event and send that. I'll see if I can get a working patch.

> You could pass the original KeyEvent reference to
> nsWindow::GeckoViewSupport::OnKeyEvent, and check the status. If it's
> non-default-prevented, then pass that reference back to GeckoEditable.

I was thinking of modifying nsWindow::GeckoViewSupport::OnKeyEvent to return either the raw nsEventStatus value or a bool in the same way Android onKeyDown/Up works. Bool is easier; I'll try that first.

(In reply to Chris Peterson [:cpeterson] from comment #49)
> Jim, are you going to land Bob's patch?

Probably best to see if the other suggested method works first.
(In reply to Bob from comment #50)
> I was thinking of modifying nsWindow::GeckoViewSupport::OnKeyEvent to return
> either the raw nsEventStatus value or a bool in the same way Android
> onKeyDown/Up works. Bool is easier; I'll try that first.

Unfortunately you won't be able to return a value from OnKeyEvent, because the call is made asynchronously. You'd have to add a new method to GeckoEditable like "onDefaultKeyEvent" and call that from OnKeyEvent, if the status is not prevent-default.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #51)
> Unfortunately you won't be able to return a value from OnKeyEvent, because
> the call is made asynchronously. You'd have to add a new method to
> GeckoEditable like "onDefaultKeyEvent" and call that from OnKeyEvent, if the
> status is not prevent-default.

Got it, thanks. Didn't realise the JNI wrapper itself made async calls.

Ok, I've implemented and tested the alternative approach.

* Key events fire correctly in JS.
* Key events also affect Android OS correctly (tested: media play, pause, next, previous). preventDefault on keyup stops the Android action, as Chrome does.
* The only works when the webpage renderer is focused! If other parts of the UI, e.g. the top sites panel, url bar, etc., are focused, the event is still swallowed! Might need to properly skip these events in other places too...
* The only place this deviates from Chrome behaviour is the handling of KEYCODE_HEADSETHOOK (wired headset key). Chrome doesn't seem to pass it on to the webpage at all, so its behaviour there is more like the first fix. Also, the way it's done in this latest patch does not correctly launch the voice search when the key is held down.

If all else looks good with this patch, I might make some minor adjustments (completely ignore HEADSETHOOK, maybe figure out where the non-gecko views handle keypresses and implement the original passthrough patch on those) and leave it at that.

Out-of-order events don't seem to be a problem, thankfully. But that might change on a heavier webpage.

---

Interesting note: whether play or pause is received actually varies based on whether there's media playing at the moment. If you block the event then you'll get the same (play or pause) forever.
Attachment #8718787 - Flags: review?(nchen)
Attachment #8718787 - Flags: review?(nchen) → feedback?(nchen)
Sorry, correction to comment 52. Seems to work when on other views too - though it isn't 100% reliable (some presses are missed).
Also, for reference, Chromium also explicitly ignores KEYCODE_HEADSETHOOK (amongst others): https://code.google.com/p/chromium/codesearch#chromium/src/content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java&l=177
Comment on attachment 8718787 [details] [diff] [review]
allow headset controls to function by passing media keyevents through to Android OS

Review of attachment 8718787 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Looks great!

I think you should change GeckoEditable.onKeyEvent to add a KeyEvent parameter, where you pass in the original KeyEvent. Then you can make onDefaultKeyEvent accept a single KeyEvent parameter. Finally inside GeckoViewSupport::OnKeyEvent, you can simply pass back the KeyEvent reference back to OnDefaultKeyEvent. That way you don't have to recreate the KeyEvent inside onDefaultKeyEvent.

::: widget/android/nsWindow.cpp
@@ +2424,5 @@
> +    bool defaultPrevented = (status == nsEventStatus_eConsumeNoDefault);
> +    
> +    if (!defaultPrevented) {
> +        mEditable->OnDefaultKeyEvent(aAction, aKeyCode, aMetaState, aTime, aRepeatCount);
> +    }

You need to put this block inside the "else {...}" block above. The other case is for "synthetic keys" that are not relevant to normal key handling.
Attachment #8718787 - Flags: feedback?(nchen) → feedback+
Attachment #8718787 - Attachment is obsolete: true
(In reply to Jim Chen [:jchen] [:darchons] from comment #56)
> I think you should change GeckoEditable.onKeyEvent to add a KeyEvent
> parameter, where you pass in the original KeyEvent. Then you can make
> onDefaultKeyEvent accept a single KeyEvent parameter. Finally inside
> GeckoViewSupport::OnKeyEvent, you can simply pass back the KeyEvent
> reference back to OnDefaultKeyEvent. That way you don't have to recreate the
> KeyEvent inside onDefaultKeyEvent.

Done. I used a jni::Object::Param on the native side - is that correct?

> You need to put this block inside the "else {...}" block above. The other
> case is for "synthetic keys" that are not relevant to normal key handling.

Done.

Also added KEYCODE_HEADSETHOOK to the completely ignored list as proposed in comment 52, matching Chromium. So now wired headsets are completely ignored and will *not* trigger events, and holding them down to show search works correctly.

The reliability issue in comment 54 - on further testing it seems to be primarily a bit of lag if the button is pressed multiple times in quick succession, resulting in some missed events. Not sure what can be done there - they're not even reaching the webpage JS, let alone the new default handler. Not sure what can be done there but it seems relatively minor.
Attachment #8720297 - Flags: review?(nchen)
Comment on attachment 8718072 [details] [diff] [review]
allow headset controls to function by passing keyevents through to Android OS

We seem to be going the default event path, so I'll obsolete the original patch.
Attachment #8718072 - Attachment is obsolete: true
Comment on attachment 8720297 [details] [diff] [review]
allow headset controls to function by passing media keyevents through to Android OS

Review of attachment 8720297 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect! Thanks!

::: mobile/android/base/java/org/mozilla/gecko/GeckoInputConnection.java
@@ +859,5 @@
>          }
>          return event;
>      }
>  
> +    private void performDefaultKeyAction(KeyEvent event) {

Change to package-private and add a comment (i.e. change 'private' to '/* package */'), because this is called within an anonymous class.

@@ +879,5 @@
> +                // Forward media keypresses to the registered handler so headset controls work
> +                // Does the same thing as Chromium
> +                // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java&l=453
> +                // These are all the keys dispatchMediaKeyEvent supports.
> +                if (Build.VERSION.SDK_INT >= 19) {

Use 'if (AppConstants.Versions.feature19Plus) {', instead of using Build directly.
Attachment #8720297 - Flags: review?(nchen) → review+
Attachment #8720297 - Attachment is obsolete: true
Attachment #8723932 - Flags: review?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #60)

> Change to package-private and add a comment (i.e. change 'private' to '/*
> package */'), because this is called within an anonymous class.
> 
> Use 'if (AppConstants.Versions.feature19Plus) {', instead of using Build
> directly.

Made those changes. Thanks for the review.

Also updated the link in the comment to a more stable one (hopefully).

If there are no other changes required, can we get a Try Server run?
Comment on attachment 8723932 [details] [diff] [review]
allow headset controls to function by passing media keyevents through to Android OS

Actually, looks like there were a lot of recent changes in this area. I'll need to update the patch ... soon as I figure out how to use vimdiff, or set up a local copy.
Attachment #8723932 - Flags: review?(nchen)
Attachment #8725201 - Attachment is obsolete: true
Attachment #8725209 - Attachment is obsolete: true
Attachment #8723932 - Attachment is obsolete: true
Comment on attachment 8725211 [details] [diff] [review]
allow headset controls to function by passing media keyevents through to Android OS

Sorry about the spam. Had some teething problems with hg bookmarks.

This should now be the correct updated version.

The earlier mentioned lag/missed presses seem to be gone, possibly as a result of bug 1248047.
Attachment #8725211 - Flags: review?(nchen)
Comment on attachment 8725211 [details] [diff] [review]
allow headset controls to function by passing media keyevents through to Android OS

Review of attachment 8725211 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks! Do you need any help with landing this?
Attachment #8725211 - Flags: review?(nchen) → review+
Summary: Bluetooth headset playback controls do not work while Firefox is open → Headset playback controls (media keys) do not work while Firefox is open
(In reply to Jim Chen [:jchen] [:darchons] from comment #68)
> Do you need any help with landing this?

Thanks for the review.


Yes, I would appreciate some help landing the patch, thanks.

Was the next step a Try Server run? I don't have commit access - could you help me submit it there?
Flags: needinfo?(nchen)
Ok, I pushed a try run of your patch [1]. I'll check back tomorrow.

Also, feel free to request commit access if you'd like. I can vouch for you.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a1eeef9804f
Flags: needinfo?(nchen)
Try run looks good so I'm going to check in the patch.
(In reply to Jim Chen [:jchen] [:darchons] from comment #71)
> Try run looks good so I'm going to check in the patch.

Thanks!

Is there anything else I should do?
https://hg.mozilla.org/mozilla-central/rev/ab2a70ab5684
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(In reply to Bob from comment #73)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #71)
> > Try run looks good so I'm going to check in the patch.
> 
> Thanks!
> 
> Is there anything else I should do?

Everything looks okay. Thanks for your work!
Bob, thanks for fixing this bug. It had lingered for four year. It was even assigned to me a couple years ago, but I never found the time to fix it. <:)

If you're looking to hack on another bug, check out the "Bugs Ahoy" site to search Bugzilla for bugs with mentors. You might be interested in bug 656101 ("Lower or mute audio volume when Android asks").

http://www.joshmatthews.net/bugsahoy/?internals-android=1
(In reply to Chris Peterson [:cpeterson] from comment #76)
> If you're looking to hack on another bug, check out the "Bugs Ahoy" site to
> search Bugzilla for bugs with mentors. You might be interested in bug 656101
> ("Lower or mute audio volume when Android asks").
> 
> http://www.joshmatthews.net/bugsahoy/?internals-android=1

Thanks. I'll take a look.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: