1199556 - Add shortcut functions for moving tabs and putting focus in search/url box

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Whitaker]

desktop shortcuts for tabs and search do not exist

Comment 1

[Kevin Whitaker]

Attachment: shortcuts.patch

Comment 2

[Kevin Whitaker]

I've been using Firefox for Android on a tablet with a keyboard(http://www.jide.com/en/remix). Compared to Firefox on the desktop, I felt less able to interact with my browser. I added two shortcuts that should give more keyboard access to the browser on android.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Would either of you be able to review this?

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8653874 [details] [diff] [review]
shortcuts.patch

I'll take this.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8653874 [details] [diff] [review]
shortcuts.patch

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

Nice, good work here! Sorry this has taken so long :( but if you send it back, I promise I won't take as long!

A handful of comments, mostly nits.

::: mobile/android/base/BrowserApp.java
@@ +663,5 @@
>  
>          // Check if this was a shortcut. Meta keys exists only on 11+.
>          final Tab tab = Tabs.getInstance().getSelectedTab();
>          if (Versions.feature11Plus && tab != null && event.isCtrlPressed()) {
> +            if (event.isShiftPressed()) {

You should move this to the KEYCODE_TAB case.

You can make a single call and just pass in whether the shift key is pressed.

final boolean isForward = !event.isShiftPressed();
Tabs.getInstance()...getNextTabCircular(tab, isForward)...

@@ +698,5 @@
>  
>                  case KeyEvent.KEYCODE_F:
>                      mFindInPageBar.show();
> +                    return true;
> +                    

Nit: Remove trailing spaces.

@@ +703,5 @@
> +                case KeyEvent.KEYCODE_L:
> +                case KeyEvent.KEYCODE_K:
> +                    enterEditingMode();
> +                    return true;
> +                

Nit: trailing whitespace

@@ +705,5 @@
> +                    enterEditingMode();
> +                    return true;
> +                
> +                case KeyEvent.KEYCODE_TAB:
> +                    Tabs.getInstance().selectTab(Tabs.getInstance().getNextTabCircular(tab,true).getId());

Nit: space between arguments of getNextTabCircular - (tab, true).

::: mobile/android/base/Tabs.java
@@ +434,5 @@
>                  return parent;
>          }
>          return nextTab;
>      }
> +    

Nit: Trailing spaces.

@@ +435,5 @@
>          }
>          return nextTab;
>      }
> +    
> +    /** Return the tab that is next in the tab loop, like on desktop. Direction true is forwards and false is backwards. */

Nit: Break this comment so it's not a single long line.

@@ +436,5 @@
>          return nextTab;
>      }
> +    
> +    /** Return the tab that is next in the tab loop, like on desktop. Direction true is forwards and false is backwards. */
> +    public Tab getNextTabCircular(Tab tab, boolean direction) {

Rename the direction argument to be something that is actually a boolean - perhaps isForwardDirection.

I might also add a check for if there is only one private or one normal tab, to just return the tab and not go through all this later logic.

@@ +437,5 @@
>      }
> +    
> +    /** Return the tab that is next in the tab loop, like on desktop. Direction true is forwards and false is backwards. */
> +    public Tab getNextTabCircular(Tab tab, boolean direction) {
> +        boolean getPrivate = tab.isPrivate();

Nit: make this final.

@@ +441,5 @@
> +        boolean getPrivate = tab.isPrivate();
> +        Tab nextTab = tab;
> +        if (direction == true) {
> +	    nextTab = getNextTabFrom(tab, getPrivate);
> +	    //Check if at end of list and circle to the front.

Nit: Space between // and comment.

@@ +443,5 @@
> +        if (direction == true) {
> +	    nextTab = getNextTabFrom(tab, getPrivate);
> +	    //Check if at end of list and circle to the front.
> +	    if (nextTab == null && getTab(0) != null && getTab(0).isPrivate() != getPrivate) {
> +		nextTab = getNextTabFrom(getTab(0),getPrivate);

Nit: space between arguments.

@@ +445,5 @@
> +	    //Check if at end of list and circle to the front.
> +	    if (nextTab == null && getTab(0) != null && getTab(0).isPrivate() != getPrivate) {
> +		nextTab = getNextTabFrom(getTab(0),getPrivate);
> +	    } else if (nextTab == null && getTab(0) != null) {
> +	        nextTab = getTab(0);

Instead of having two if/else if statements that are almost the same, I would consider just nesting two ifs, something like:

if (nextTab == null && getTab(0) != null) {
    nextTab = getTab(0);
    if (nextTab.isPrivate() != getPrivate) {
        nextTab = getNextTabFrom(nextTab, getPrivate);
    }
} else {
...

@@ +449,5 @@
> +	        nextTab = getTab(0);
> +	    }
> +        } else {
> +            nextTab = getPreviousTabFrom(tab, getPrivate);
> +            //Check if at start of list and circle to the back.

Nit: space after //

@@ +450,5 @@
> +	    }
> +        } else {
> +            nextTab = getPreviousTabFrom(tab, getPrivate);
> +            //Check if at start of list and circle to the back.
> +	    if (nextTab == null && getTab(mOrder.size()-1) != null && getTab(mOrder.size()-1).isPrivate() != getPrivate) {

Nit: Space around operators (like '-' )

@@ +451,5 @@
> +        } else {
> +            nextTab = getPreviousTabFrom(tab, getPrivate);
> +            //Check if at start of list and circle to the back.
> +	    if (nextTab == null && getTab(mOrder.size()-1) != null && getTab(mOrder.size()-1).isPrivate() != getPrivate) {
> +		nextTab = getPreviousTabFrom(getTab(mOrder.size()-1),getPrivate);

Also consider my earlier comment about if statements - whatever you decide to do, they should match.

Comment 6

[Kevin Whitaker]

Attachment: shortcuts++.patch

Additions to first patch(8653874) for cleanup.

Comment 7

[Kevin Whitaker]

Is the patch obsolete if both are needed for the full changes?

Comment 8

[Chenxia Liu [:liuche]]

Oh, that's my bad - would you fold the patches together into one? I was just going to comment that the patch doesn't apply for me :P

Also, update the patch message to be of the form
Bug <bug-number> - <bug-title>. r=<reviewer>

with me as the reviewer (liuche).

Thanks!

Comment 9

[Chenxia Liu [:liuche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f5ae1aaad7

Comment 10

[Kevin Whitaker]

Attachment: shortcuts-full.patch

Both patches now in one files.

Comment 11

[Kevin Whitaker]

When you say patch message, do you mean the commit messages or the bug issue title?

Comment 12

[Kevin Whitaker]

Attachment: shortcuts-fixed.patch

After looking around at some docs, you most likely meant the commit name(or else why mention putting the bug title in message). This should be more like you wanted than last one.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8659647 [details] [diff] [review]
shortcuts-fixed.patch

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

One last change! And then this is ready to land :)

Thanks, Kevin, both for the really nice patch, and being really responsive.

Feel free to drop into #mobile (usually during PST) if you were looking for more bugs/see what we're working on!

::: mobile/android/base/Tabs.java
@@ +448,5 @@
> +            // Check if at end of list and circle to the front.
> +            if (nextTab == null && getTab(0) != null) {
> +                nextTab = getTab(0);
> +                if(nextTab.isPrivate() != getPrivate) {
> +                    nextTab = getNextTabFrom(getTab(0), getPrivate);

You can just use nextTab instead of getTab(0), I believe. That saves us one more getTab() call.

@@ +457,5 @@
> +            // Check if at start of list and circle to the back.
> +            if (nextTab == null && getTab(mOrder.size() - 1) != null) {
> +                nextTab = getTab(mOrder.size() - 1);
> +                if(getTab(mOrder.size() - 1).isPrivate() != getPrivate) {
> +                    nextTab = getPreviousTabFrom(getTab(mOrder.size() - 1), getPrivate);

Same as the previous comment.

Comment 14

[Kevin Whitaker]

You are right. Didn't check for that when I refactored it before.

Comment 15

[Kevin Whitaker]

Attachment: shortcuts-fixed.patch

Here is the fixed patch.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8659647 [details] [diff] [review]
shortcuts-fixed.patch

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

::: mobile/android/base/BrowserApp.java
@@ -690,5 @@
>                      return true;
>  
>                  case KeyEvent.KEYCODE_F:
>                      mFindInPageBar.show();
> -                return true;

It looks like this was just returning true always, which was weird. However, I did just notice that after this patch, the ctrl+w, ctrl+t, etc don't work unless you open a new tab. Are you seeing this as well?

e.g.,

- Open a new tab, urlbar is highlighted.
- Tap somewhere on the page content, to unhighlight the urlbar
- Use ctrl+w shortcut

I don't see the tab being closed anymore, which looks like a regression.

Comment 17

[Kevin Whitaker]

(In reply to Chenxia Liu [:liuche] from comment #16)
> Comment on attachment 8659647 [details] [diff] [review]
> shortcuts-fixed.patch
> 
> Review of attachment 8659647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ -690,5 @@
> >                      return true;
> >  
> >                  case KeyEvent.KEYCODE_F:
> >                      mFindInPageBar.show();
> > -                return true;
> 
> It looks like this was just returning true always, which was weird. However,
> I did just notice that after this patch, the ctrl+w, ctrl+t, etc don't work
> unless you open a new tab. Are you seeing this as well?
> 
> e.g.,
> 
> - Open a new tab, urlbar is highlighted.
> - Tap somewhere on the page content, to unhighlight the urlbar
> - Use ctrl+w shortcut
> 
> I don't see the tab being closed anymore, which looks like a regression.

I am not getting any different behaviour on my build vs firefox stable for this. Also, neither firefox stable nor my build automatically highlight the url bar when making a new tab. If I do highlight it, I have to hit back twice to get back to the browser mode that accepts shortcuts. While I can see this not being ideal, it doesn't look like a regression.

Comment 18

[Chenxia Liu [:liuche]]

Okay, now that you point it out, with our current behavior, we do need two back presses to be able to handle keyboard shortcuts again. I'll file a bug for that, no need to worry about that for this bug.

One thing that I did notice is that the number of tabs a tab knows about can get stale.

1. Open 2 tabs and cycle between them by tabbing
2. Open two more tabs, and then close the _next to last one_ (#3)
3. Go back to tab #2 and tab backwards through the tabs

I wonder if it's mOrder.size() getting out of sync, because mOrder is a CopyOnWrite List? Now that each tab is handling the tablist, mOrder needs be a CopyOnRead List. For example, if you open or close tabs while on tab Y, and then switch to tab X and start cycling through tabs, tab X will not have the right mOrder to call size() on.

Comment 19

[Kevin Whitaker]

I don't see a CopyOnReadArrayList in java. Would the best way to handle that situation be to instead use a synchronized ArrayList?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Kevin – sorry about the delay! In the future, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.

Comment 21

[Chenxia Liu [:liuche]]

I looked at this a little bit and I'm pretty sure this is a deeper problem for why these list sizes are not sync, but I haven't had the time to figure out what it is. Keeping the needinfo flag, but I haven't figured out why this is crashing.

Comment 22

[Kevin Whitaker]

Thanks for still looking into this issue.

Comment 23

[Paul [:pmac] McLanahan]

I just got an android tablet as well and am experiencing this issue. Google has a list of keyboard shortcuts that work on their official tablet (Pixel C)[0], so hopefully Firefox can support some of them as well. This patch is a good start, but some of the others in the list seem useful as well.

[0] https://support.google.com/pixel/answer/6326992?hl=en

Thanks all for your work on this! Just hoping it lands soon!

Comment 25

[mjonsson1986]

any progress? latest nightly but no shortcuts

Comment 26

[robsmith11]

Is anyone still looking at this?  Keyboard shortcuts are pretty important when using a tablet/phone with a bluetooth keyboard and no mouse.

Comment 27

[austin.soplata]

Has there been any developments for this? I'm trying to use a USB keyboard (Kinesis Advantage) connected to a ChromeOS ASUS Chromebook Flip 101pa-db02, and neither the internal or USB keyboard work. (I'll add that when using Firefox desktop on a crouton-Debian install on the Chromebook, everything works fine, so it's probably not the hardware).

I think Chrome OS may be overloading the shortcuts, since I get interesting behavior with the USB keyboard (not the internal keyboard): if my Firefox Android is on a not-Firefox-Home/"empty" tab, and I hit ctrl+t, it does spawn a new tab, but then if I ctrl+t again, the computer switches to Chrome and opens a new tab *there*. Similarly, in Firefox Android if I ctrl+w, instead of closing just a tab, it quits the whole program. I think that either ChromeOS may be capturing certain keychords and preventing them from going to Firefox, or Firefox Android is misinterpreting key symbols (and/or Android/ChromeOS has weird/nonstandard key symbols).

I'll also add in a vote that maybe increasing amounts of people are going to do serious, even developer, job-work on Android/smaller OS's.

Comment 28

[Sam Tuke]

Disappointing to find these shortcuts still aren't available 8 years after an initial bug report.

Comment 29

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.