1038354 - Switch to tab toast doesn't work in editing mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open a private browsing tab
2) Click the URL bar
3) Long press a top site thumbnail
4) Choose Open in New Tab or Open in Private Tab
5) Click the "Switch" button on the toast

After these STR, nothing happens. We should switch to the opened tab.

Comment 1

[Nick Alexander :nalexander [he/him]]

This is not actually a private-browsing interaction at all!  It is revealed there because when you open a fresh private browsing tab, the only thing to do is to focus the URL bar.  In a regular tab, you can interact with the home fragment before focusing the URL bar.  (That works.)

However, if you focus the URL bar, and then interact with the home fragment (from a regular or private tab), the tab switch fails.  There's some home fragment state to consider here.  Adding lucasr and margaret for their home fragment knowledge.

That is, the new STR are Brian's original, but it's not important if the tab is private.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Exit editing mode before quick-switching to tab.

So, this quick hack fixes the problem.  We could codify this by
extracting an interface (like OnUrlOpen...) and saving the BrowserApp
in HomeFragment.onViewCreated or similar.  I'm not sure what is the
right way to dismiss edit mode.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8458333 [details] [diff] [review]
Exit editing mode before quick-switching to tab.

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

Looks fine but the direct coupling between BrowserApp and HomeFragment is a bit unfortunate.

::: mobile/android/base/BrowserApp.java
@@ +1573,5 @@
> +        // mode exit) in lieu of the tab we are about to select.
> +        mTargetTabForEditingMode = null;
> +
> +        final Tabs tabs = Tabs.getInstance();
> +        tabs.selectTab(tabId);

nit: Tabs.getInstance().selectTab(tabId) to be more concise?

@@ +1575,5 @@
> +
> +        final Tabs tabs = Tabs.getInstance();
> +        tabs.selectTab(tabId);
> +
> +        mBrowserToolbar.cancelEdit();

I'd probably cancel edit mode before selecting the new tab as we use the edit mode state to handle about:home visibility when a new tab is selected, see:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#239

::: mobile/android/base/home/HomeFragment.java
@@ +206,5 @@
>                      R.drawable.switch_button_icon,
>                      new ButtonToast.ToastListener() {
>                          @Override
>                          public void onButtonClicked() {
> +                            ((BrowserApp) getActivity()).switchToTab(newTabId);

This is directly coupling HomeFragment with BrowserApp which is not ideal. But it seems some previous change has already coupled HomeFragment with GeckoApp :-/ I would probably have added some interface for handling this action (i.e. create the toast and handling the button click) outside HomeFragment. We can talk about this in a follow-up if you prefer.

Strictly speaking, the type cast above should actually use BrowserApp instead of GeckoApp because HomeFragment is a BrowserApp-specific component.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Exit editing mode before quick-switching to tab.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 8458333 [details] [diff] [review]
> Exit editing mode before quick-switching to tab.
> 
> Review of attachment 8458333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine but the direct coupling between BrowserApp and HomeFragment is a
> bit unfortunate.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1573,5 @@
> > +        // mode exit) in lieu of the tab we are about to select.
> > +        mTargetTabForEditingMode = null;
> > +
> > +        final Tabs tabs = Tabs.getInstance();
> > +        tabs.selectTab(tabId);
> 
> nit: Tabs.getInstance().selectTab(tabId) to be more concise?
> 
> @@ +1575,5 @@
> > +
> > +        final Tabs tabs = Tabs.getInstance();
> > +        tabs.selectTab(tabId);
> > +
> > +        mBrowserToolbar.cancelEdit();
> 
> I'd probably cancel edit mode before selecting the new tab as we use the
> edit mode state to handle about:home visibility when a new tab is selected,
> see:
> 
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#239

In this case we should update the maybeSwitchToTab logic as well:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1599

If we are adding a new helper method for switching tabs, we should probably just use it inside the maybeSwitchToTab implementation.

> ::: mobile/android/base/home/HomeFragment.java
> @@ +206,5 @@
> >                      R.drawable.switch_button_icon,
> >                      new ButtonToast.ToastListener() {
> >                          @Override
> >                          public void onButtonClicked() {
> > +                            ((BrowserApp) getActivity()).switchToTab(newTabId);
> 
> This is directly coupling HomeFragment with BrowserApp which is not ideal.
> But it seems some previous change has already coupled HomeFragment with
> GeckoApp :-/ I would probably have added some interface for handling this
> action (i.e. create the toast and handling the button click) outside
> HomeFragment. We can talk about this in a follow-up if you prefer.
> 
> Strictly speaking, the type cast above should actually use BrowserApp
> instead of GeckoApp because HomeFragment is a BrowserApp-specific component.

I agree with lucasr that this coupling of HomeFragment and BrowserApp isn't great. It looks like the only existing GeckoApp dependency comes from using the button toast, and it would be nice to see a better interface for accessing that (rather than directly getting the widget from GeckoApp), but we could address that in a follow-up.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Exit editing mode before quick-switching to tab. r=lucasr

This moves the relevant functionality into an interface (implemented
by BrowserApp), which helps reduce the coupling between the
HomeFragments and the rest of the App lifecycle.

I agonized about adding a new interface versus extending an existing
one, and finally decided that I am not the right person to extract the
unified interface (at least, not right now).

Finally, I will be PTO until mid-August.  If one of you can shepherd
this through landing, and then possibly uplift to Aurora, that would
be much appreciated.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8460406 [details] [diff] [review]
Exit editing mode before quick-switching to tab. r=lucasr

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

I wonder if we could do this by tracking background tabs being added directly in Tabs instead of tying this code specifically to HomePager?

Comment 8

[:Margaret Leibovic]

Lucas, do you want to help wrap up this patch, since Nick isn't going to be back until August 11?

I can also take it on, but you seem to have ideas about a different way to approach this :)

Comment 9

[Lucas Rocha (:lucasr)]

Ok, I'll have a look at it next week.

Comment 10

[Nick Alexander :nalexander [he/him]]

Oh dear, this slipped up to Beta.  I'll try to move on this.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Poke from triage.

Comment 13

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

I'm not exactly sure what the issue is regarding this landing, nor *all* the coupling issues, but I do want to note that editing mode does make a lot of assumptions that state will not change while it's open (likely including selected tab state). I will likely de-couple this in bug 1058902, but note that this is in no rush to land or be uplifted. That being said, after it lands, I'll have some more knowledge with which to work on this bug.

Comment 14

[Nick Alexander :nalexander [he/him]]

OK, I'll take this and see if I can get something moving.

Comment 15

[Nick Alexander :nalexander [he/him]]

Here's the plan: land the rebased original patch on Nightly, r=lucasr.  (No need for re-review, in my opinion.)  I've tested this locally, but we'll get a QA pass.

The Nightly patch applies, with offsets, to Aurora.  Beta requires some love, since refactorings landed; but I have it locally updated and will attach.  I built and locally tested Aurora and Beta, but we'll want QA here too.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: 1038354.aurora.patch

Approval Request Comment
[Feature/regressing bug #]: I think it's always been there.

[User impact if declined]: poor user experience when using a nice feature on the Home Panel.  Open in new [private] tab -> click switch icon will do nothing.

[Describe test coverage new/current, TBPL]: tested locally.

[Risks and why]: late in the cycle, stalled patch being landed late.  (Due to PTO and some pretty minor futzing with approach that wasn't worthwhile.)

[String/UUID change made/needed]: none.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: 1038354.beta.patch

Approval Request Comment
[Feature/regressing bug #]: I think it's always been there.

[User impact if declined]: poor user experience when using a nice feature on the Home Panel.  Open in new [private] tab -> click switch icon will do nothing.

[Describe test coverage new/current, TBPL]: tested locally.

[Risks and why]: late in the cycle, stalled patch being landed late.  (Due to PTO and some pretty minor futzing with approach that wasn't worthwhile.)  Some possibility of completely screwing the logic and throwing an exception or similar in weird edge cases that I can't predict.

[String/UUID change made/needed]: none.

Comment 18

[Nick Alexander :nalexander [he/him]]

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

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8498660 [details] [diff] [review]
1038354.beta.patch

We are so late in the cycle (beta 9 go to build is today and no mobile version), the patch is big and touches important files, we have this bug for a while, I think this should not be uplifted in beta but aurora could take it!

Comment 20

[u421692]

Tested on latest fx-team build:
* Issue does not reproduce anymore
* Performed different scenarios and Switch to tab functionality is working fine

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Mihai Pop from comment #20)
> Tested on latest fx-team build:
> * Issue does not reproduce anymore
> * Performed different scenarios and Switch to tab functionality is working
> fine

Thanks for such qa turn-around.  It's always appreciated.

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #22)
> (In reply to Mihai Pop from comment #20)
> > Tested on latest fx-team build:
> > * Issue does not reproduce anymore
> > * Performed different scenarios and Switch to tab functionality is working
> > fine
> 
> Thanks for such qa turn-around.  It's always appreciated.

Argh, *quick* qa turn-around.

Comment 24

[Flaviu Cos]

Verified as fixed in build 35.0a1 (2014-10-03);
Device: Nexus 7 (Android 4.4.4).

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Ping for the Aurora patch

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8498658 [details] [diff] [review]
1038354.aurora.patch

Aurora+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc93d8dfa1e9

Comment 28

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on
Build: Firefox for Android 34 Beta 1
Device: Asus Transformer Pad TF300T (Android 4.2.1)