1794664 - Feature: Keyboard shortcuts

Status: ASSIGNED

(Firefox for Android :: General, task, P2)

Description

[Christian Sadilek [:csadilek]]

From github: https://github.com/mozilla-mobile/android-components/issues/1978.

This came up for the reference browser here:
https://github.com/mozilla-mobile/reference-browser/issues/575

Since most of those shortcuts would probably invoke UseCases (or indirectly perform things on the SessionManager or Session).. maybe we could just wrap that in a component and with that provide those keyboard shortcuts to all our apps easily.

Related code in Fennec:
https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#471-567

https://searchfox.org/mozilla-esr68/rev/13df4aebd54ee9ba73924dd558af289ef515a490/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#534

---

• [ ] New tab
• [ ] Navigate forward
• [ ] Navigate back
• [ ] Reload
• [ ] Stop loading
• [ ] Private tab
• [ ] Close tab
• [ ] Find in page

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 1

[edwin]

Would love to have this feature back.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:cpeterson, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[David Balch]

Some work was done on this, but needs help from a more experienced developer - see comments in:

• https://github.com/mozilla-mobile/android-components/issues/1978
• https://github.com/mozilla-mobile/android-components/pull/9948

Comment 5

[Christian Sadilek [:csadilek]]

See also: https://github.com/mozilla-mobile/fenix/issues/3729

Comment 6

[czlucius]

Maybe we can have something like this (UML diagram attached)?
https://user-images.githubusercontent.com/58442255/224989808-3c256caf-8738-4e9d-808c-f728ff4981d0.png

From @gaul's code (attached GitHub issue), we can have a generic abstract DispatchKeyEvent (is that alr in Fenix?), that would be extended to implement a single shortcut for every subclass (for extensibility, if not all shortcut code must be added to only one file) - similar to what is done for Feature(I think?)
We can then have an ArrayList<DispatchKeyEvent> in the GeckoEngineView, then in the BaseBrowserFragment, or appropriate locations, we add the shortcuts one by one with addShortcut().

Also, if needed, I can also add a field for the name and description of the shortcut, so users can discover is more quickly. We should use the OS keyboard shortcuts menu to advertise available shortcuts, probably in HomeActivity. Although undocumented, it is a feature of AOSP since Android Nougat, and I've discovered how to do this and posted my answer here: https://stackoverflow.com/a/71062698/12204281.

How does this sound?
Thanks.

Comment 7

[Christian Sadilek [:csadilek]]

Reading the original discussion on Github, I agree that's roughly the right shape.

The missing piece here is the Feature class, which would allow registering specific shortcut handlers in applications e.g., Fenix. The feature class would be bound to the view (as most of our features), have access to an EngineView and listen for key events. It would then invoke the corresponding shortcut handler in response to those events.

Comment 8

[czlucius]

I think View.OnKeyListener and the corresponding setOnKeyListener should be more appropriate for this use case.

Comment 9

[czlucius]

(In reply to czlucius from comment #8)

I think View.OnKeyListener and the corresponding setOnKeyListener should be more appropriate for this use case.
Seems to be what Fennec uses as well, although Fennec hard-codes it. I think a feature implementation will do well for this.

https://hg.mozilla.org/mozilla-central/file/add212ee1118aff550a5fc05420282556fb3ade9/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#l573

Comment 10

[czlucius]

Seems like onKeyDown, onKeyUp and onKeyPreIme is overriden in GeckoView.java, hence nothing is received with setOnKeyListener.
Thus, an override for dispatchKeyEvent is needed.

Comment 11

[czlucius]

Attachment: photo_2023-03-23_22-05-56.jpg

I've started working on this, got the reload shortcut to work with the suggested modular approach, implemented with a feature component.
Will push a PR when it is ready.

Comment 12

[gitta.hubner]

How are things going? I would very much enjoy to be able to use Firefox on the tablet.

Comment 13

[gustavo]

Hello there,

Just want to add some extra context as reading here makes it look like a relativeky new problem, but this user request dates back 12 years now, if we count it all the way to the Fennec issue that was fixed shortly before the transition to Fenix, which still has the bug to this day:

https://bugzilla.mozilla.org/show_bug.cgi?id=726716

This was also discussed to some length in GitHub for over 4 years, before the repository was retired:

https://github.com/mozilla-mobile/fenix/issues/3729

That latter issue is linked from user comments all around the web, mainly as people find that they are stuck with other browsers whenever they want to use Android with a keyboard, which is more common nowadays as there are larger tablet screens.

It's great to see some action on this per earlier comments above, but considering it's been over 8 months now, hopefully this didn't get unintendedly missed again as other items moved up the list.

Thanks for the great work you all do.

Comment 14

[christoph]

(In reply to czlucius from comment #11)

Created attachment 9324679 [details]
photo_2023-03-23_22-05-56.jpg

I've started working on this, got the reload shortcut to work with the suggested modular approach, implemented with a feature component.
Will push a PR when it is ready.

Hi czlucius, your screenshot looks great! Did you have a chance to move this topic forward in the past few months?

The missing support for keyboard shortcuts on Android is a real annoyance. It often leads me to to go back to Chrome on my Samsung Tab S9 tablet with the keyboard cover.

Comment 15

[Thomas.Heinrich.Schmidt]

Is there any hope that keyboard shortcuts will be implemented for Android Firefox?
Maybe someone with the required superpower can review Priority: Not set Severity: N/A.
Otherwise, this issue probably has the last place in the ranking.

Comment 16

[gitta.hubner]

This is such an unbelievably low hanging fruit. Yet it was not fixed for over years, preventing a percent or so of users from using firefox. Is the bugtracking system dysfunctional in prioritizing? Everybody please just make this happen, karma nirvana is all yours.

Comment 17

[Thomas.Heinrich.Schmidt]

@czlucius Have you made any progress with your work?

Comment 18

[Veddu]

I just came here to see what the status of this request was. Had to go back to vivaldi since I neither have tabs nor keyboard shortcuts when using samsung dex..

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:boek, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Comment 20

[czlucius]

Hi there.
I have been quite busy the past year, but have found some time on this of late. .

From what I can see from the attempted PR and Fennec, they all handle dispatchKeyEvent, and I think the ideal place is in GeckoEngineView.
I have made slight progress in the keyboard feature in AC, to plug into a GeckoEngineView for capturing dispatched KeyEvents.
Some of the keyboard shortcuts can be written in AC in the Feature itself, but some (like new tab) have to be written in Fenix as it is very dependent on navigation.

But before this can be solved, there is 1 problem that will be blocking it (in which I can file another issue), if not even if keyboard shortcuts are implemented the experience would not be according to standard:
Some websites may call event.preventDefault() to capture the shortcut from the browser, and we should support this behaviour (see https://stackoverflow.com/a/3680946/12204281). It seems like for GeckoView's processing keyevent code which leads to GeckoEditable.java (in the fn processKey, around line 2551), just returns true (hence capturing the keyevent), for all valid keyboard shortcuts (see snippet below).
This means all keyevents including our keyboard shortcuts is captured by Gecko, and its either all (we capture all shortcuts and disallow any website-override), or nothing (the browser does not get any keyevent at all because it is captured by Gecko).

Hence I think we need to find a solution to get Gecko(View) to return us whether it has captured the keyevent (i.e. the website has overriden it), which will be the return value of processKey hence onKeyDown (in GeckoView.java, line 880). I don't have much experience with Gecko code, it seems like afterwards it is being passed to native code in C++ which I cannot trace. But the chain of functions all return void which is going to be a problem (in addition to the callback-based sendKeyEvent as seen below)? Maybe someone from the Fenix/GeckoView team can advice on how we can proceed with this.

Thanks!
Let me know if you need any clarification.

Snippet for processKey of GeckoEditable:

  private boolean processKey(
      final @Nullable View view,
      final int action,
      final int keyCode,
      final @NonNull KeyEvent event) {
    if (keyCode > KeyEvent.getMaxKeyCode() || !shouldProcessKey(keyCode, event)) {
      return false;
    }

    postToInputConnection(
        new Runnable() {
          @Override
          public void run() {
            sendKeyEvent(view, action, event);
          }
        });
    return true;
  }

Comment 21

[czlucius]

The function can be found here:
https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java#2551

Comment 22

[Thomas.Heinrich.Schmidt]

I am very, very satisfied. After all these years, you have dealt with the subject seriously, so that I can have some hope again.

Comment 25

[[:owlish] 🦉 PST]

Hey Makoto, is this something that needs to be done in GV layer?

Comment 26

[Makoto Kato [:m_kato]]

Attachment: WIP: Bug 1794664 - Part 1. Add feature-keyboardshortcut to perform keyboard shortcut.

Comment 27

[Makoto Kato [:m_kato]]

Attachment: WIP: Bug 1794664 - Part 2. Use feature-keyboardshortcut in Fenix.

Comment 28

[Makoto Kato [:m_kato]]

(In reply to [:owlish] 🦉 PST from comment #25)

Hey Makoto, is this something that needs to be done in GV layer?

Although I am consider this implementation for 2 days, I think that new GeckoView API may be unnecessary. Actually, since GeckoView.onKey* calls super.onKey at first, we can override keyboard handling by View.setOnKeyListener.

I attach concept code / WIP as feature-keyboardshortcut to perform keyboard shortcut.

Comment 29

[Makoto Kato [:m_kato]]

As shortcut, there is both browser level and content level. Browser level is handed by this, but I need to investigate content level.

Comment 30

[Makoto Kato [:m_kato]]

(WIP is just imported from legacy Fennec code. It uses setOnKeyListener for View)

Comment 31

[Makoto Kato [:m_kato]]

We don't want to block any events by UI thread, so onKey* should return immediately. If you handle content level shortcut, I guess that it is better to add an API such as GeckoSession.setPostOnKeyListerer(View.OnKeyListener)...

Comment 32

[czlucius]

Thanks!
@m_kato I actually have some code that handles keyboard shortcuts also using Features, in a polymorphic approach. I've written a few subclasses to handle refresh, and some other commands.
Was waiting for the GV fix because if not the implementation wouldn't be good as websites won't be able to override the shortcuts.
How do I push this up as a draft as you did?
Thanks!

Comment 33

[czlucius]

I'll go test this out.
@m_kato does the WIP code (i.e. session?.webView?.setOnKeyListener(listener) ) successfully prevent the browser from handling the shortcut if already handled by the webpage?
Thanks.

Comment 34

[u764163]

Please add this to the "See also" field (in order for you to remember to update Mozilla Connect when this feature is completed): https://connect.mozilla.org/t5/ideas/android-firefox-shortcuts-keys/idi-p/57847

Comment 35

[Thomas.Heinrich.Schmidt]

I'm delighted. Now things are really starting to move, thank you very much!

Comment 36

[Makoto Kato [:m_kato]]

(In reply to czlucius from comment #33)

I'll go test this out.
@m_kato does the WIP code (i.e. session?.webView?.setOnKeyListener(listener) ) successfully prevent the browser from handling the shortcut if already handled by the webpage?
Thanks.

We will add new GeckoView API (see Bug 1921161 Comment 12) to handle more key event things.

Comment 37

[Cathy Lu [:calu]]

Hey Makoto, are there any updates on this feature? How much work is left and is it prioritized?

Comment 38

[Makoto Kato [:m_kato]]

It will be ready for review next week.

Comment 39

[Makoto Kato [:m_kato]]

GeckoView dispatches key event by 2 path. One is dispatched by parent process, and another is dispatched by content process via Binder. When editable element doesn't has focus, it is by parent. Child process case is most by IME related. Desktop version dispatch from parent only. (Unit tests might be able to dispatch from content process). I am re-thinking good way for 2 patterns.

Also, for media key support, GeckoEditable has internal listener, but this is broken now. So I also consider to fix it.

Comment 40

[Thomas.Heinrich.Schmidt]

Wonderful, I'm really excited, and finally we turn into the home straight.

Comment 41

[Delta]

(In reply to Thomas.Heinrich.Schmidt from comment #40)

Wonderful, I'm really excited, and finally we turn into the home straight.

Yeah it's been one of the sore spots of attempting to use Firefox on a tablet for some time now.

(In reply to Makoto Kato [:m_kato] from comment #39)

GeckoView dispatches key event by 2 path. One is dispatched by parent process, and another is dispatched by content process via Binder. When editable element doesn't has focus, it is by parent. Child process case is most by IME related. Desktop version dispatch from parent only. (Unit tests might be able to dispatch from content process). I am re-thinking good way for 2 patterns.

Also, for media key support, GeckoEditable has internal listener, but this is broken now. So I also consider to fix it.

Are you considering unifying these paths, or perhaps refining the conditions under which each is used?

Comment 42

[Makoto Kato [:m_kato]]

I will file some related bugs to add APIs.

Comment 43

[Thomas.Heinrich.Schmidt]

Are we getting closer? Is someone working on the APIs?

Comment 44

[Thomas.Heinrich.Schmidt]

🎉🎉🎉 we are indeed getting closer as dependent bug 1974148 has been fixed!

Comment 45

[gitta.hubner]

It's been three years. Is this going anywhere?

Everytime a new version comes out I test it. But still nothing after all this time.

Comment 46

[mousa.w.abbasi]

Hey, is there any updates?