1167382 - Reset search engine bar scroll state when it is shown

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

STR:
1) Click the url bar
2) Type "a"
3) Scroll right in the search engine bar at the bottom
4) Hit the "X" to close the search screen
5) Click the url bar
6) Type "a"

Expected: The search engine bar is scrolled to the left
Actual: The search engine bar retains scrolled state (i.e. to the right)

The user can customize their search engine positions so we should show them their highest priority search engines.

Comment 1

[Prabhjyot Sodhi [:psd]]

Hey!
I would love to take a shot at this.

Comment 2

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

Sure, Prabhjyot - welcome back! :D

The search engine bar code can generally be found in SearchEngineBar.java and the browser_search.xml and search_engine_bar_item.xml files.

Let me know if you have any questions.

Comment 3

[Prabhjyot Sodhi [:psd]]

Hi Michael! Thanks :)

I did have a look at the code that you pointed to. But I am not sure as to how the lifecycle of activities in fennec work and whether the searchbar is a part of the same activity as our general browsing.
I thought I would initialize the scroll to default 0,0 position each time it was created but I need to understand when all is it triggered. Do you feel this is the right approach?

Are there some online docs for Fennec where I could read more about the various activities?
I hope I make sense.
Thanks again! :)

Comment 4

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

(In reply to Prabhjyot Sodhi [:psd] from comment #3)
> Hi Michael! Thanks :)
> 
> I did have a look at the code that you pointed to. But I am not sure as to
> how the lifecycle of activities in fennec work and whether the searchbar is
> a part of the same activity as our general browsing.

Our primary Activity is BrowserApp (which extends GeckoApp). When the url bar is clicked, we open the HomePager, which gets replaced with the BrowserSearch screen when the user starts to type. The BrowserSearch screen contains the SearchEngineBar, which is the scroll state we want to update.

As for specifics of when things are created/destroyed (as opposed to just being shown), I'd have to look it up so I'll leave it to you and you can ask me again if you have specific questions. :)

> I thought I would initialize the scroll to default 0,0 position each time it
> was created but I need to understand when all is it triggered. Do you feel
> this is the right approach?

I doubt we re-create the SearchEngineBar each time it is shown because that'd be expensive (and if we did do that, we wouldn't have to reset any scroll state because it'd automatically be initialized to 0).

I want to reset scroll state every time it's shown - look for BrowserApp.showBrowserSearch.

> Are there some online docs for Fennec where I could read more about the
> various activities?

Unfortunately, we have little documentation beyond what's in the code - the reason for that is that documentation often gets out of date - the code is the best documentation there is. :) That being said, we have a few Javadocs in code but probably not enough to form a comprehensive story, nor do I know if there's an easy way to generate this into a full html javadoc with our current setup (if you want to try, I'd recommend building and doing it on the output objdir). If you have specific questions about what some class/method/etc. does, feel free to ask.

Comment 5

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

FYI, Prabhjyot, I'd like to land this by the next merge (6/29) - will you be able to complete this by then?

[Tracking Requested - why for this release]: Should ship with the search engine bar (bug 1137483).

Comment 6

[Prabhjyot Sodhi [:psd]]

Hey Michael!
Give me time till the coming Monday! I should be able to come up with the first patch! :)

Comment 7

[Prabhjyot Sodhi [:psd]]

Hey Michael!

I was trying to scroll mSearchEngineBar in BrowserSearch.java to (0, 0) each time the showBrowserSearch() in BrowserApp.java is called. But I am repeatedly getting Null Pointer Exceptions. I also tried resetting the search engines(using: mSearchEngineBar.setSearchEngine(...) ), but again i got a Null pointer Exception. I am trying to figure out the reason of the null pointer exception. Do you understand what is happening here?

Thanks!

Comment 8

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

Can you post your work-in-progress patch, Prabhjyot?

Comment 9

[Prabhjyot Sodhi [:psd]]

Attachment: 1167382.patch

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41 as it makes sense to re-focus on user's higher priority search engines (left-most) rather than lower priority engines (right-most).

Comment 11

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

Comment on attachment 8623769 [details] [diff] [review]
1167382.patch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +513,4 @@
>          return null;
>      }
>  
> +    public void scrollTo(int x, int y) {

nit: I'd call this "resetScrollState" or similar – there's no need to have specific scroll values (and there are multiple scrolls to scroll here).

@@ +515,5 @@
>  
> +    public void scrollTo(int x, int y) {
> +        Log.d("CHECKTAG", "Attempting to scroll to 0 from browser search!");
> +
> +        mSearchEngineBar.scrollTo(x, y);

This gets initialized in onCreateView which I would assume doesn't need to get called until setUserVisibleHint(true) or setVisibility(View.VISIBLE) is called (which, notably, happens after scrollTo is called.

So mSearchEngineBar is probably null.

Comment 12

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

(Sorry for the delay, I was on PTO part of last week)

Comment 13

[Prabhjyot Sodhi [:psd]]

Attachment: 1167382.patch

Comment 14

[Prabhjyot Sodhi [:psd]]

Hey!
Don't bother the above patch. I thought I fixed the issue, but noticed trouble immediately after uploading.

(In reply to Michael Comella (:mcomella) from comment #11)
> nit: I'd call this "resetScrollState" or similar – there's no need to have
> specific scroll values (and there are multiple scrolls to scroll here).

What did you mean by 'multiple scrolls to scroll here'?
did you mean multiple functions other than scrollTo() that I could call in the resetScrollState function?

Comment 15

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

(In reply to Prabhjyot Sodhi [:psd] from comment #14)
> What did you mean by 'multiple scrolls to scroll here'?

Honestly, I'm not sure.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Michael, I am trying to follow up on the FF41+ tracked bugs. I noticed that this bug hasn't had much activity for over a month. Given, that we are almost half way through the Beta41 cycle, it would be nice if we can fix this soon and request a patch for uplift. Thanks!

Comment 17

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

This actually doesn't need to track 41.

Prabhjyot, are you still working on this?

Comment 18

[Prabhjyot Sodhi [:psd]]

Hey Michael!
I am back at it. I spent quite some time on this today.

I was using the scroll methods defined in the View class (and inherited by TwoWayView and hence SearchEngineBar). But they don't seem to be making any changes.
Before realizing that the View's functions were actually not making any changes, I tried calling the resetScroll function from 
1) onPause of Fragment
2) showBrowserSearch() in BrowserApp
3) hideBrowserSearch() in BrowserApp
but ended up with similar results in all cases.

Coming back to the idea of using methods other than the ones defined in View, like smoothScrollToPosition, I am going through [1] [2]

any suggestions? or any place you feel I am doing wrong.

Thanks for your patience :)

[1]: http://stackoverflow.com/questions/11431832/android-smoothscrolltoposition-not-working-correctly
[2]: https://github.com/lucasr/twoway-view/issues/53

Comment 19

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

(In reply to Prabhjyot Sodhi [:psd] from comment #18)
> I was using the scroll methods defined in the View class (and inherited by
> TwoWayView and hence SearchEngineBar). But they don't seem to be making any
> changes.

I wonder if TwoWayView doesn't properly support these "scroll" methods (e.g. [2] from comment 18!) and that's what the issue was. We recently moved SearchEngineBar over to RecyclerView (bug 1184211) – maybe you can try these methods again?

Make sure you pull the latest changes!

> [1]:
> http://stackoverflow.com/questions/11431832/android-smoothscrolltoposition-
> not-working-correctly
> [2]: https://github.com/lucasr/twoway-view/issues/53

Nice research! :)

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This bug does not seem to be a release blocker, setting FF41 status as "wontfix". Tracked for 42.

Comment 21

[Prabhjyot Sodhi [:psd]]

Attachment: 1167382.patch

Finally!
scrollToPosition(int position): (the recycler view method) worked just fine :)

Comment 22

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

Comment on attachment 8652797 [details] [diff] [review]
1167382.patch

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

Almost there!

The scroll state seems to reset on its own now but I'd guess that's bug 1186683. This seems correct so I wouldn't mind landing this for when bug 1186683 is fixed though. It'd technically be more correct if bug 1186683 was fixed first if you wanted more work, Prabhjyot. :P

::: mobile/android/base/home/BrowserSearch.java
@@ +246,5 @@
>      @Override
>      public void onPause() {
>          super.onPause();
>  
> +        this.resetScrollState();

I think this should be onStop() – the Fragment is still visible and can be restored from onPause – onStop should guarantee it's no longer visible.

Comment 23

[Prabhjyot Sodhi [:psd]]

Attachment: 1167382.patch

Hey mcomella!
I tried moving the resetScrollState() call to onStop as well as onPause but in vain.
I tested this on my device on top of my patch for bug 1186683[https://bugzilla.mozilla.org/show_bug.cgi?id=1186683] and it seems to work fine :)

Comment 24

[Prabhjyot Sodhi [:psd]]

oh sorry, ^ Forgot that bugzilla links up bug foofoo automatically

Comment 25

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

(In reply to Prabhjyot Sodhi [:psd] from comment #23)
> I tested this on my device on top of my patch for bug
> 1186683

Nice! If they need to land in this order, we should add the bug dependencies above (like so!).

Don't forget to comment about this when you add "checkin-needed"!

Comment 26

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

Comment on attachment 8657432 [details] [diff] [review]
1167382.patch

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

Awesome.

try (on top of bug 1186683): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b9ac665542b

Comment 27

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf87e3cec99

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b862f5b9b036

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b862f5b9b036

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Michael, do you think this should be uplifted to 43 & 42? Thanks

Comment 31

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #30)
> Michael, do you think this should be uplifted to 43 & 42? Thanks

No. iirc, this should only affect 44 as bug 1186683 only landed in 44. When I initially filed, I think I had the patches from bug 1186683 applied.