Awesomebar loses correct tab selection

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 23
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
FF mobile 19 ...

Start browser ... tap awesomebar ... tap bookmarks tab ... notice curved tab selection indicates bookmarks tab

tap awesomebar again and delete all text ... selected tab still indicates bookmarks tab, but its the top sites tab ...

swipe screen left once or twice to observe
(Assignee)

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Depends on: 837815
(Assignee)

Comment 1

6 years ago
Created attachment 722867 [details] [diff] [review]
Patch (v1)

This could be done as a smaller patch without being dependent on bug 837815 ... but I took the ooportunity to tighten up some related code ... hence |feedback?| vs |review?| at this point ...
Attachment #722867 - Flags: feedback?(wjohnston)
Comment on attachment 722867 [details] [diff] [review]
Patch (v1)

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

::: mobile/android/base/AwesomeBarTabs.java
@@ +155,5 @@
> +        AllPagesTab allPages = getAllPagesTab();
> +        int tab = getTabByTag(allPages.getTag());
> +        mViewPager.setCurrentItem(tab);
> +        tabWidget.setCurrentTab(tab);
> +        styleSelectedTab();

It make sense to me, if we have to do this in multiple places, to pull setTabByTag() into its own method. Is there some reason you don't want to do it?

@@ +200,5 @@
>      public void onLightweightThemeReset() {
>          styleSelectedTab();
>      }
>  
> +    public int getTabByTag(String tag) {

Since this returns an id and not a tag, maybe we should call get it getTabIdByTag()

@@ -316,5 @@
>          // Restore normal focus behavior on tab host
>          setDescendantFocusability(ViewGroup.FOCUS_AFTER_DESCENDANTS);
>  
> -        // The tabs should only be visible if there's no on-going search
> -        mSearching = searchTerm.length() != 0;

mSearching is used to determine if we allow you to swipe through screens.
Attachment #722867 - Flags: feedback?(wjohnston)
(Assignee)

Comment 3

6 years ago
Created attachment 722976 [details] [diff] [review]
Patch (v2)


Assuming my initial testing hasn't overlooked anything further |mSearching fail :(|, this Patch (v2) fixes this bug (observing your suggestions), also fixes bug 837815, and avoids Bug 847475.

Minor issue: The call to open the correct bookmark tab from the reading list will call |styleSelectedTab()| one time more than required under the covers ...

Minor issue: The logic in the |filter()| function while simplified will still contain a magic number unless I keep something like the |getTabIdByTag()| function from my first Patch (v1).

I'm still testing, and can post the build if it helps you or a Q/A type to review it.
Attachment #722976 - Flags: feedback?(wjohnston)
(Assignee)

Comment 4

6 years ago
Oh... I'd removed the set/restore DescendantFocusability stuff in filter() during testing as I didn't understand the requirement, and haven't observed an effect. Is this for a11y support?
(Assignee)

Comment 5

6 years ago
Created attachment 723869 [details] [diff] [review]
patch (v3)

Refactored / rebased
Attachment #722867 - Attachment is obsolete: true
Attachment #722976 - Attachment is obsolete: true
Attachment #722976 - Flags: feedback?(wjohnston)
Attachment #723869 - Flags: review?(wjohnston)
Comment on attachment 723869 [details] [diff] [review]
patch (v3)

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

I'm a little confused about this, but maybe I'm missing something. The issue is that we're not calling styleSelectedTab() in filter(), but you basically added a styleSelectedTab call to setTabByTag. So why are we using a magic number. Clearing review so you can explain this magic to me (and maybe add a comment explaining it as well?)

::: mobile/android/base/AwesomeBarTabs.java
@@ -306,5 @@
>      }
>  
>      public void filter(String searchTerm) {
> -        // Don't let the tab's content steal focus on tab switch
> -        setDescendantFocusability(ViewGroup.FOCUS_BLOCK_DESCENDANTS);

In reply to your comment, no. I think this is in place to ensure that focus stays in the urlbar when we do this switch.

@@ +311,4 @@
>  
> +        // Ensure the 'All Pages' tab is styled
> +        getTabWidget().setCurrentTab(0);
> +        styleSelectedTab();

I'm not sure what the advatage of using this over setTabByTag() is? In fact, don't we need to set the tab to all pages and not just the indicator?
Attachment #723869 - Flags: review?(wjohnston)
(Assignee)

Comment 7

6 years ago
Created attachment 724614 [details] [diff] [review]
Patch (simplest)

Ok, I took the long way around to do this. The simplest patch to fix this is attached, and it points out interesting things that I then "tightened up" (previous patch) but knew I'd then have to explain :D

The original call in filter() to setCurrentTabByTag() (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBarTabs.java#314) doesn't actually seem to do anything (the problem this patch is fixing).

Replacing it with a call to our new setTabByTag() doesn't work. It's attempt to set mViewPager.setCurrentItem(i); doesn't do anything useful, and actually loses the focus (keyboard dissapears) in a way that the bookmarked set / restore DescendantFocusability() calls in filter() don't prevent.

What's left as |required| is in the patch, the combination 
getTabWidget().setCurrentTab(0); and styleSelectedTab(); calls, and that's where I didn't know if it was worth going further to do a getTabIdByTag() method like we were tinkering with in comment 2 or just live with a simplest case change that leaves the magic number |0|.

Maybe IRC might have been a better way to discuss all this?
(Assignee)

Updated

6 years ago
Attachment #724614 - Attachment is patch: true
(Assignee)

Comment 8

6 years ago
Comment on attachment 724614 [details] [diff] [review]
Patch (simplest)

Forgot to flag ...
Attachment #724614 - Flags: feedback?(wjohnston)
(Assignee)

Comment 9

6 years ago
Nag ping :)
(In reply to Mark Capella [:capella] from comment #7)
> Replacing it with a call to our new setTabByTag() doesn't work. It's attempt
> to set mViewPager.setCurrentItem(i); doesn't do anything useful

Sorry. I keep avoiding this because I don't quite understand. Why doesn't this work? Is it a mystery to you too?
(Assignee)

Comment 11

6 years ago
Created attachment 732215 [details] [diff] [review]
Patch (v5)

I've been over this several times again and believe this does it. Posting the patch without a review request for now, as I want to peek at the splinter review diff real quick, and I'm pushing it to TRY so I can have someone check the build, quick Q/A style.
(Assignee)

Comment 12

6 years ago
wesj: Here's the TRY build ... 

/me might want to find Q/A type to test this a bit ... aaronmt perhaps?

https://tbpl.mozilla.org/?tree=Try&rev=2f76d640b8cf
Try build works for me; the user is positioned back on 'Top Sites' after deleting the current selection instead of seeing the wrong view under Bookmarks.
(Assignee)

Comment 14

6 years ago
Comment on attachment 732215 [details] [diff] [review]
Patch (v5)

I'd better get in this in line :P
Attachment #732215 - Flags: review?(wjohnston)
Comment on attachment 732215 [details] [diff] [review]
Patch (v5)

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

Looks good! I don't love the NPOS thing and would just return -1. But otherwise, seems good! Lets have lucas sign off on reming the setDescendantFocusability hack. I don't understand it (we're keeping focus in the textbox, but hiding the keyboard so you can't type?)

::: mobile/android/base/AwesomeBarTabs.java
@@ +211,3 @@
>              }
>          }
> +        return NPOS;

Lets just return -1; no need for a const here.
Attachment #732215 - Flags: feedback?(lucasr.at.mozilla)
(In reply to Mark Capella [:capella] from comment #4)
> Oh... I'd removed the set/restore DescendantFocusability stuff in filter()
> during testing as I didn't understand the requirement, and haven't observed
> an effect. Is this for a11y support?

This was to avoid getting the focus moved to the results list when you started typing on the entry (back when we were using TabHost). This might not be necessary anymore. To confirm you're not regressing anything, follow these steps:

1. Open awesome screen
2. Switch to bookmarks tab
3. Tap on the entry and type something

The focus should remain on the entry as expected.
Comment on attachment 732215 [details] [diff] [review]
Patch (v5)

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

Assuming you test the steps to reproduce I mentioned above, this patch looks fine. I haven't reviewed the whole patch btw. I'm just giving my input on the setDescendantFocusability() part.
Attachment #732215 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 18

6 years ago
Correct

Tap on awesome screen
Tap on bookmarks tab, "about:home" is selected
Tap in middle of "about:home" entry, cursor is inserted, KBD opens
Type backspace on KBD several times, focus stays in awesomebar, KBD stay open, screen changes as appropriate
(Assignee)

Comment 19

6 years ago
Created attachment 735994 [details] [diff] [review]
Patch (v6) Final w/o NPOS const

Final version w/o NPOS const ...
Attachment #723869 - Attachment is obsolete: true
Attachment #724614 - Attachment is obsolete: true
Attachment #724614 - Flags: feedback?(wjohnston)
Comment on attachment 732215 [details] [diff] [review]
Patch (v5)

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

Looks good! I don't love the NPOS thing and would just return -1. But otherwise, seems good! Lets have lucas sign off on reming the setDescendantFocusability hack. I don't understand it (we're keeping focus in the textbox, but hiding the keyboard so you can't type?)

::: mobile/android/base/AwesomeBarTabs.java
@@ +211,3 @@
>              }
>          }
> +        return NPOS;

Lets just return -1; no need for a const here.
Attachment #732215 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 21

6 years ago
back to TRY with the final version
https://tbpl.mozilla.org/?tree=Try&rev=1a4f4bb329b3
https://hg.mozilla.org/mozilla-central/rev/334da067051e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.